-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Upgrade TinkerPop version to 3.7.0 [cql-tests][tp-tests] #3914
Upgrade TinkerPop version to 3.7.0 [cql-tests][tp-tests] #3914
Conversation
This includes: 1. Upgraded maven dependency version 2. Fixed compilation errors caused by breaking changes 3. Fixed maven dependency convergence errors 4. Enabled one previously disabled test 4. Updated upgrade doc Signed-off-by: Boxuan Li <[email protected]>
2a82b83
to
e80e368
Compare
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.
LGTM. Thank you @li-boxuan !
@@ -14,4 +14,4 @@ | |||
|
|||
hosts: [localhost] | |||
port: 8182 | |||
serializer: { className: org.apache.tinkerpop.gremlin.driver.ser.GraphBinaryMessageSerializerV1, config: { serializeResultToString: true }} |
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.
I've just noticed that I'm now getting a warning if I try to use this config file with our latest Docker images:
gremlin> :remote connect tinkerpop.server conf/remote.yaml session
Sep 13, 2023 2:23:59 PM org.yaml.snakeyaml.internal.Logger warn
WARNING: Failed to find field for org.apache.tinkerpop.gremlin.driver.Settings.serializers
Do you know what could be causing this, @li-boxuan?
It however continues to work afterwards. I haven't tested however whether the serializer config is actually used or not.
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.
I didn't notice this warning - thanks for bringing it up. I am guessing it's caused by some dependency upgrade in TinkerPop, maybe they bumped the snakeyaml dependency and broke backward compatibility.
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.
// WARNING: Failed to find field for org.apache.tinkerpop.gremlin.driver.Settings.serializers
Shouldn't this be org.apache.tinkerpop.gremlin.server.Settings.serializers
?
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.
Shouldn't this be
org.apache.tinkerpop.gremlin.server.Settings.serializers
?
Yeah so the warning itself doesn't quite make sense. Somehow the code believes the driver should also have this serializers
setting, and since the driver doesn't, snakeyaml logs a warning message.
This includes:
Thank you for contributing to JanusGraph!
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
master
)?For code changes:
For documentation related changes: