Upgrade TinkerPop to 3.5.0 [tp-tests] #2619

li-boxuan · 2021-05-16T11:20:25Z

Bump TinkerPop dependency from 3.4.10 to 3.5.0 and resolve
dependency conflicts. Several jackson packages are ignored from
requireUpperBoundDeps maven enforce plugin now, because Spark 3.0.0
requires a lower version of jackson (2.10.0) while other libraries,
including CQL driver, require a higher version of jackson.
Support null value mutation semantics to comply with TinkerPop 3.5.0.
property(single, prop, null) removes the property while
property(list/set, prop, null) is simply ignored.
Introduce a workaround for traversal strategies which can be removed
once TINKERPOP-2568 is fixed.
Upgrade apache common configuration to configuration2 to comply
with the same TinkerPop breaking change. Now users won't experience the
problem reported at ConfigurationGraphFactory configuration with multiple storage.hostname not working #1447 anymore, since configuration2 by default does
not read comma delimited string as a list of values.
Other minor changes mostly caused by breaking changes in TinkerPop.

Closes #2611, #2624, #1447

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:

Is there an issue associated with this PR? Is it referenced in the commit message?
Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you written and/or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

...rg/janusgraph/graphdb/tinkerpop/optimize/strategy/JanusGraphLocalQueryOptimizerStrategy.java

li-boxuan · 2021-05-22T09:35:49Z

I cleaned up the settings parsing.

@farodin91 Do you want to create a separate PR for that clean-up?

farodin91 · 2021-05-22T09:37:53Z

I cleaned up the settings parsing.

@farodin91 Do you want to create a separate PR for that clean-up?

It requires tp 3.5. I can wait that this PR is merged and create a new PR. I think should be part of jg 0.6.

li-boxuan · 2021-05-22T09:38:44Z

It requires tp 3.5. I can wait that this PR is merged and create a new PR

Got it, I think that would be better. Just want to avoid unnecessary changes in this PR since it's already large.

farodin91

Could you update mkdocs.yml and docs/changelog.md?

farodin91

LGTM! Thank you for upgrading TP?

I just have some questions.

farodin91 · 2021-05-23T08:35:19Z

janusgraph-backend-testutils/src/main/java/org/janusgraph/diskstorage/SimpleScanJob.java

@@ -269,9 +270,11 @@ public static Configuration getJobConf(List<SliceQuery> queries, Long modulus) {
    }

    public static Configuration getJobConf(List<SliceQuery> queries, Long modulus, Long modVal) {
+        BaseConfiguration baseConfiguration = new BaseConfiguration();
+        baseConfiguration.setListDelimiterHandler(new DefaultListDelimiterHandler(','));


Why we need this setting?

To maintain backward compatibility. In the old version of commons-configuration, comma-separated values are parsed into a list of values, while in the new version (configuration2), commas are read as they are.

ref:

http://tinkerpop.apache.org/docs/3.5.0-SNAPSHOT/upgrade/#_versions_and_dependencies

https://commons.apache.org/proper/commons-configuration/userguide/upgradeto2_0.html

Do we want the old behavior?

If we don't do baseConfiguration.setListDelimiterHandler(new DefaultListDelimiterHandler(','));, then comma-delimited values, e.g. "host1,host2,host3" will be read as a single string, rather than a string array ["host1","host2","host3"] (which is the old behavior).

In theory, we can read it as a single string. When we need it, we split it into a string array. The drawback is, this string splitting might happen many times on runtime and thus inefficient. Another problem is, I am not sure if I could implement that change correctly and not miss any place.

janusgraph-dist/src/assembly/static/conf/gremlin-server/gremlin-server-berkeleyje.yaml

farodin91 · 2021-05-23T08:40:59Z

pom.xml

@@ -88,7 +88,8 @@
        <scylladb.version>1.7.1</scylladb.version>
        <!-- align with org.apache.hbase:hbase -->
        <jackson1.version>1.9.13</jackson1.version>
-        <jackson2.version>2.12.2</jackson2.version>
+        <!-- align with org.apache.spark:spark-core_2.12 -->


We should upgrade spark in a next pr to 3 or not?

Not sure if I got you. spark-gremlin 3.5.0 depends on org.apache.spark:spark-core_2.12 3.0.0: https://mvnrepository.com/artifact/org.apache.tinkerpop/spark-gremlin/3.5.0

2.12 for jackson or not?

The spark package name is org.apache.spark:spark-core_2.12. The version of it is 3.0.0. It requires Jackson 2.10.0, otherwise, it throws runtime exceptions.

It looks like the 2.12 is for the support scale version.

You mean scala right? Yes, the "2.12" suffix of "spark_core" is seemingly to match the scala version.

porunov

Thanks a lot @li-boxuan for this upgrade!
I have just some nitpicks mostly

janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphFactory.java

...ore/src/main/java/org/janusgraph/diskstorage/configuration/backend/CommonsConfiguration.java

...ph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphTraversalUtil.java

janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java

...ph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphTraversalUtil.java

janusgraph-core/src/main/java/org/janusgraph/graphdb/vertices/AbstractVertex.java

janusgraph-core/src/main/java/org/janusgraph/util/system/ConfigurationLint.java

1. Bump TinkerPop dependency from 3.4.10 to 3.5.0 and resolve dependency conflicts. Several jackson packages are ignored from requireUpperBoundDeps maven enforce plugin now, because Spark 3.0.0 requires a lower version of jackson (2.10.0) while other libraries, including CQL driver, require a higher version of jackson. 2. Support null value mutation semantics to comply with TinkerPop 3.5.0. property(single, prop, null) removes the property while property(list/set, prop, null) is simply ignored. 3. Introduce a workaround for traversal strategies which can be removed once TINKERPOP-2568 is fixed. 4. Upgrade apache common configuration to configuration2 to comply with the same TinkerPop breaking change. Now users won't experience the problem reported at JanusGraph#1447 anymore, since configuration2 by default does not read comma delimited string as a list of values. 5. Other minor changes mostly caused by breaking changes in TinkerPop. Closes JanusGraph#2611, JanusGraph#2624, JanusGraph#1447 Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>

...ph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphTraversalUtil.java

porunov

LGTM. Thank you @li-boxuan for this upgrade!

li-boxuan · 2021-05-27T16:38:43Z

Anyone who is still reviewing, or intends to review, please let me know within 24 hours. Otherwise, I'll merge this PR after that.

rngcntr · 2021-05-28T08:37:15Z

The version compatibility matrix should list TP 3.5.z instead of 3.4.z for JanusGraph 0.6, LGTM otherwise

porunov · 2021-05-28T11:59:18Z

The version compatibility matrix should list TP 3.5.z instead of 3.4.z for JanusGraph 0.6, LGTM otherwise

I think it makes sense to add a separate commit which won't trigger full tp tests because it's just a documentation change.
I would either add a separate commit to this PR or add a CTR commit after the merge

This adds test cases to demonstrate that comma-delimited values can be loaded by HadoopGraph in Spark usecase. Prior to TinkerPop 3.5.0 upgrade, if config file has `gremlin.graph=org.apache.tinkerpop.gremlin.hadoop.structure.HadoopGraph`, which is typically the case for MapReduce/Spark traversals, then `GraphFactory.open(filename)` would first read all comma delimited values into a list, and then only retain the last value. A workaround is to create a `PropertiesConfiguration` config object, disable comma delimiter, and then call `GraphFactory.open(config)`. This is auto-resolved by JanusGraph#2619 because since TinkerPop 3.5.0, comma-delimited values are by default loaded as they are. This commit adds some test cases to demonstrate this behaviour. The same tests fail without JanusGraph#2619. Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>

This adds test cases to demonstrate that comma-delimited values can be loaded by HadoopGraph in Spark usecase. Prior to TinkerPop 3.5.0 upgrade, if config file has `gremlin.graph=org.apache.tinkerpop.gremlin.hadoop.structure.HadoopGraph`, which is typically the case for MapReduce/Spark traversals, then `GraphFactory.open(filename)` would first read all comma delimited values into a list, and then only retain the last value. A workaround is to create a `PropertiesConfiguration` config object, disable comma delimiter, and then call `GraphFactory.open(config)`. This is auto-resolved by #2619 because since TinkerPop 3.5.0, comma-delimited values are by default loaded as they are. This commit adds some test cases to demonstrate this behaviour. The same tests fail without #2619. Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>

janusgraph-bot added the cla: external Externally-managed CLA label May 16, 2021

li-boxuan force-pushed the bump-tp-to-3.5.0 branch from 7c2780f to 014a5bc Compare May 16, 2021 11:40

li-boxuan marked this pull request as draft May 16, 2021 11:43

farodin91 added this to the Release v0.6.0 milestone May 16, 2021

li-boxuan commented May 16, 2021

View reviewed changes

...rg/janusgraph/graphdb/tinkerpop/optimize/strategy/JanusGraphLocalQueryOptimizerStrategy.java Show resolved Hide resolved

li-boxuan mentioned this pull request May 16, 2021

TINKERPOP-1568 Changed Strategy Application Methodology apache/tinkerpop#1211

Merged

li-boxuan force-pushed the bump-tp-to-3.5.0 branch 4 times, most recently from 3630c22 to 3eb6c6a Compare May 19, 2021 03:26

li-boxuan mentioned this pull request May 19, 2021

Usage of null deviates from TinkerPop 3.5.0 #2624

Closed

li-boxuan changed the title ~~Upgrade TinkerPop to 3.5.0 [tp-tests]~~ Upgrade TinkerPop to 3.5.0 May 19, 2021

li-boxuan force-pushed the bump-tp-to-3.5.0 branch 3 times, most recently from f89bbc9 to 63ce476 Compare May 21, 2021 07:52

li-boxuan changed the title ~~Upgrade TinkerPop to 3.5.0~~ Upgrade TinkerPop to 3.5.0 [tp-tests] May 21, 2021

li-boxuan force-pushed the bump-tp-to-3.5.0 branch 2 times, most recently from b4b8ffd to e8738f5 Compare May 22, 2021 08:14

This comment has been minimized.

Sign in to view

li-boxuan marked this pull request as ready for review May 22, 2021 09:34

farodin91 requested changes May 22, 2021

View reviewed changes

li-boxuan force-pushed the bump-tp-to-3.5.0 branch 2 times, most recently from 73a1e74 to 78323ec Compare May 22, 2021 17:29

farodin91 approved these changes May 23, 2021

View reviewed changes

porunov self-requested a review May 23, 2021 19:05

li-boxuan mentioned this pull request May 24, 2021

Add config option to use barrier size as batch size limit #2516

Merged

porunov reviewed May 24, 2021

View reviewed changes

li-boxuan force-pushed the bump-tp-to-3.5.0 branch 4 times, most recently from eac5087 to 1a6d8cc Compare May 26, 2021 16:27

li-boxuan force-pushed the bump-tp-to-3.5.0 branch from 1a6d8cc to e016e0d Compare May 26, 2021 16:35

porunov reviewed May 26, 2021

View reviewed changes

...ph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphTraversalUtil.java Show resolved Hide resolved

porunov approved these changes May 27, 2021

View reviewed changes

li-boxuan merged commit 3da125d into JanusGraph:master May 29, 2021

li-boxuan deleted the bump-tp-to-3.5.0 branch May 29, 2021 04:08

li-boxuan mentioned this pull request May 29, 2021

changelog: Update tinkerpop compatibility matrix #2632

Merged

9 tasks

porunov mentioned this pull request May 29, 2021

ConfigurationGraphFactory configuration with multiple storage.hostname not working #1447

Closed

li-boxuan mentioned this pull request Jun 1, 2021

Add test for HadoopGraph loading comma delimited values #2638

Merged

9 tasks

planetf1 mentioned this pull request Jun 2, 2021

gremlin 3.5 (dependabot proposal) causes breakage odpi/egeria#5297

Closed

porunov mentioned this pull request Jun 29, 2021

JanusGraph release v0.6.0 [cql-tests] [tp-tests] #2672

Merged

9 tasks

porunov mentioned this pull request Oct 18, 2023

Allow removal of configuration properties #4061

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade TinkerPop to 3.5.0 [tp-tests] #2619

Upgrade TinkerPop to 3.5.0 [tp-tests] #2619

li-boxuan commented May 16, 2021 •

edited

This comment has been minimized.

li-boxuan commented May 22, 2021

farodin91 commented May 22, 2021

li-boxuan commented May 22, 2021

farodin91 left a comment

farodin91 left a comment

farodin91 May 23, 2021

li-boxuan May 23, 2021

farodin91 May 24, 2021

li-boxuan May 25, 2021 •

edited

farodin91 May 23, 2021

li-boxuan May 23, 2021

farodin91 May 24, 2021

li-boxuan May 24, 2021

farodin91 May 24, 2021

li-boxuan May 25, 2021

porunov left a comment

porunov left a comment

li-boxuan commented May 27, 2021

rngcntr commented May 28, 2021

porunov commented May 28, 2021

Upgrade TinkerPop to 3.5.0 [tp-tests] #2619

Upgrade TinkerPop to 3.5.0 [tp-tests] #2619

Conversation

li-boxuan commented May 16, 2021 • edited

For all changes:

For code changes:

For documentation related changes:

This comment has been minimized.

li-boxuan commented May 22, 2021

farodin91 commented May 22, 2021

li-boxuan commented May 22, 2021

farodin91 left a comment

Choose a reason for hiding this comment

farodin91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

li-boxuan May 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porunov left a comment

Choose a reason for hiding this comment

porunov left a comment

Choose a reason for hiding this comment

li-boxuan commented May 27, 2021

rngcntr commented May 28, 2021

porunov commented May 28, 2021

li-boxuan commented May 16, 2021 •

edited

li-boxuan May 25, 2021 •

edited