Skip to content
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

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Jun 6, 2024

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.

@niloc132 niloc132 added this to the 3. May 2024 milestone Jun 6, 2024
@niloc132
Copy link
Member Author

niloc132 commented Jun 6, 2024

Example error (before changing the version in Classpaths):

        at io.deephaven.server.runner.scheduler.SchedulerModule$ThreadFactory.lambda$newThread$0(SchedulerModule.java:100)
        at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: io.deephaven.UncheckedDeephavenException: Library 'Arrow Format' requires FlatBuffer 1.12.x, found 2.0.x
        at io.deephaven.extensions.barrage.util.BarrageUtil.verifyFlatbufferCompatibility(BarrageUtil.java:142)
        at io.deephaven.extensions.barrage.util.BarrageUtil.<clinit>(BarrageUtil.java:119)
        ... 11 more
2024-06-06T21:19:03.752Z | Thread-0             |  WARN | d.u.p.ShutdownManagerImpl | Initiating shutdown processing
2024-06-06T21:19:03.753Z | Thread-0             |  WARN | d.u.p.ShutdownManagerImpl | Starting to invoke FIRST shutdown tasks
2024-06-06T21:19:03.763Z | Thread-0             |  WARN | d.u.p.ShutdownManagerImpl | Done invoking FIRST shutdown tasks

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?

@@ -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'
Copy link
Member

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?

Copy link
Member Author

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".

Comment on lines +134 to +139
Optional<String> foundVersion = Arrays.stream(Constants.class.getDeclaredMethods())
.map(Method::getName)
.map(BarrageUtil::extractFlatBufferVersion)
.filter(Optional::isPresent)
.map(Optional::get)
.findFirst();
Copy link
Member

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")

Copy link
Member Author

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.

@niloc132 niloc132 merged commit 29ce5ee into deephaven:main Jun 11, 2024
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants