-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce runtime version checks for flatbuffer compatibility #5583
Conversation
Example error (before changing the version in Classpaths):
Technically, 1.12.0 and 1.12.1 and compatible, as is 2.0.0 through 2.0.3 (and probably later, but I didn't check), but after 22.y.z, the version is based on a date rather than following semvers, and exact matches are required. We could drop the library name ("Arrow Format" above) and display the FQCN instead so that in the case of class relocations the user knows which to go after? It might also make sense to read the FQCN of com.google.flatbuffers.Constants and load it with Class.forName? |
fea49ce
to
7316923
Compare
@@ -52,7 +52,7 @@ class Classpaths { | |||
|
|||
static final String FLATBUFFER_GROUP = 'com.google.flatbuffers' | |||
static final String FLATBUFFER_NAME = 'flatbuffers-java' | |||
static final String FLATBUFFER_VERSION = '2.0.3' | |||
static final String FLATBUFFER_VERSION = '1.12.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got some non-obvious linked dependencies in other places; I try to have some comments to remind myself and others who may do version bumps in the future. Does it make sense to add a comment here and around arrow (or barrage?) dependencies that we need to keep them in-sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually only bringing the dependency in line with our transitive dependencies - before this, we saw that some classpaths had 1.12.0, and others had 1.12.0 -> 2.0.3, though none had 2.0.3 directly (except the two projects in dhc that actually used this property).
Rather than a comment, I'd like to get a little further into correct gradle dep management, and possibly a specific rule that says "if anything tries to disagree with this, blow up right away (so we correct it on a case-by-case basis".
Optional<String> foundVersion = Arrays.stream(Constants.class.getDeclaredMethods()) | ||
.map(Method::getName) | ||
.map(BarrageUtil::extractFlatBufferVersion) | ||
.filter(Optional::isPresent) | ||
.map(Optional::get) | ||
.findFirst(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the risk of being even more over engineered, do we care about java.lang.Package#getImplementationVersion
? (Fine if answer is "no")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work here unfortunately, it was the first thing I tried. Some subset of flatbuffers-java versions shipped with META-INF/METADATA.MF
files with no Implementation-Version
property. Instead other properties were set, but none that can be consistently read without a whole lot more code.
This approach let me introduce extractFlatBufferVersion and use it in two places. Previously, I just logged the missing method error and the implversion as you said, and let the user put the pieces together.
Downgrades our flatbuffers-java version to one that should actually be correctly compatible, and adds a runtime check that the appropriate version is in use.
This check is somewhat overengineered as it may be user-facing, giving the user as much detail as we can, no matter what environment they are running from - it should be resilient to relocating classes when shading/shadowing.