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.

Sign up for GitHub

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
Merged

Introduce runtime version checks for flatbuffer compatibility #5583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion buildSrc/src/main/groovy/Classpaths.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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".


static final String DAGGER_GROUP = 'com.google.dagger'
static final String DAGGER_NAME = 'dagger'
Expand Down
74 changes: 74 additions & 0 deletions extensions/barrage/src/main/java/io/deephaven/extensions/barrage/util/BarrageUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
//
package io.deephaven.extensions.barrage.util;

import com.google.flatbuffers.Constants;
import com.google.flatbuffers.FlatBufferBuilder;
import com.google.protobuf.ByteString;
import com.google.protobuf.ByteStringAccess;
import com.google.rpc.Code;
import io.deephaven.UncheckedDeephavenException;
import io.deephaven.barrage.flatbuf.BarrageMessageWrapper;
import io.deephaven.base.ArrayUtil;
import io.deephaven.base.ClassUtil;
import io.deephaven.base.verify.Assert;
Expand Down Expand Up @@ -44,6 +46,7 @@
import io.deephaven.vector.Vector;
import io.grpc.stub.StreamObserver;
import org.apache.arrow.flatbuf.KeyValue;
import org.apache.arrow.flatbuf.Message;
import org.apache.arrow.util.Collections2;
import org.apache.arrow.vector.types.TimeUnit;
import org.apache.arrow.vector.types.Types;
Expand All @@ -56,6 +59,8 @@
import org.jetbrains.annotations.Nullable;

import java.lang.reflect.Array;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.time.Instant;
Expand All @@ -64,6 +69,8 @@
import java.time.ZonedDateTime;
import java.util.*;
import java.util.function.*;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -105,6 +112,73 @@ public class BarrageUtil {
private static final String ATTR_TYPE_TAG = "type";
private static final String ATTR_COMPONENT_TYPE_TAG = "componentType";

private static final boolean ENFORCE_FLATBUFFER_VERSION_CHECK =
Configuration.getInstance().getBooleanWithDefault("barrage.version.check", true);

static {
verifyFlatbufferCompatibility(Message.class);
verifyFlatbufferCompatibility(BarrageMessageWrapper.class);
}

private static void verifyFlatbufferCompatibility(Class<?> clazz) {
try {
clazz.getMethod("ValidateVersion").invoke(null);
} catch (InvocationTargetException e) {
Throwable targetException = e.getTargetException();
if (targetException instanceof NoSuchMethodError) {
// Caused when the reflective method is found and cannot be used because the flatbuffer version doesn't
// match
String requiredVersion = extractFlatBufferVersion(targetException.getMessage())
.orElseThrow(() -> new UncheckedDeephavenException(
"FlatBuffers version mismatch, can't read expected version", targetException));
Optional<String> foundVersion = Arrays.stream(Constants.class.getDeclaredMethods())
.map(Method::getName)
.map(BarrageUtil::extractFlatBufferVersion)
.filter(Optional::isPresent)
.map(Optional::get)
.findFirst();
Comment on lines +134 to +139
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.

String dependentLibrary = clazz.getPackage().getSpecificationTitle();
final String message;
if (foundVersion.isEmpty()) {
message = "Library '" + dependentLibrary + "' requires FlatBuffer " + requiredVersion
+ ", cannot detect present version";
} else {
message = "Library '" + dependentLibrary + "' requires FlatBuffer " + requiredVersion + ", found "
+ foundVersion.get();
}
if (ENFORCE_FLATBUFFER_VERSION_CHECK) {
throw new UncheckedDeephavenException(message);
} else {
log.warn().append(message).endl();
}
} else {
throw new UncheckedDeephavenException("Cannot validate flatbuffer compatibility, unexpected exception",
targetException);
}
} catch (IllegalAccessException e) {
throw new UncheckedDeephavenException(
"Cannot validate flatbuffer compatibility, " + clazz + "'s ValidateVersion() isn't accessible!", e);
} catch (NoSuchMethodException e) {
// Caused when the type isn't actually a flatbuffer Table (or the codegen format has changed)
throw new UncheckedDeephavenException(
"Cannot validate flatbuffer compatibility, " + clazz + " is not a flatbuffer table!", e);
}
}

private static Optional<String> extractFlatBufferVersion(String method) {
Matcher matcher = Pattern.compile("FLATBUFFERS_([0-9]+)_([0-9]+)_([0-9]+)").matcher(method);

if (matcher.find()) {
if (Integer.valueOf(matcher.group(1)) <= 2) {
// semver, third decimal doesn't matter
return Optional.of(matcher.group(1) + "." + matcher.group(2) + ".x");
}
// "date" version, all three components should be shown
return Optional.of(matcher.group(1) + "." + matcher.group(2) + "." + matcher.group(3));
}
return Optional.empty();
}

/**
* These are the types that get special encoding but are otherwise not primitives. TODO (core#58): add custom
* barrage serialization/deserialization support
Expand Down
Loading