You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "kenhuuu (via GitHub)" <gi...@apache.org> on 2023/05/06 04:49:12 UTC
[GitHub] [tinkerpop] kenhuuu opened a new pull request, #2056: TINKERPOP-2703 Build on JDK17
kenhuuu opened a new pull request, #2056:
URL: https://github.com/apache/tinkerpop/pull/2056
Fixes https://issues.apache.org/jira/browse/TINKERPOP-2703
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] xiazcy commented on pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#issuecomment-1555017033
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#issuecomment-1553480591
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] kenhuuu commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1195756645
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -146,6 +146,24 @@ See: link:https://issues.apache.org/jira/browse/TINKERPOP-2819[TINKERPOP-2819]
distributions. Any app which makes use of both `gremlin-driver` and `gremlin-server` will now need to directly
include both modules.
+==== Building and Running with JDK 17
Review Comment:
I added a small section to the developer documentation that mentioned running with various JDK versions.
> i suspect there are more places in the docs that make various proclamations about JDK versions. please search around for a bit and see what might need updating.
I looked around for this but most of the places that mention which Java runtime to use are generic and don't refer to a specific version. In the areas that specifically mention using Java 11 (like building documentation), I think it makes sense to keep them like that for now as I don't think JDK 17 will become our default choice yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] lyndonbauto commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "lyndonbauto (via GitHub)" <gi...@apache.org>.
lyndonbauto commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1187780194
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -103,9 +103,9 @@ public final class TinkerGraph implements Graph {
protected TinkerIndex<TinkerVertex> vertexIndex = null;
protected TinkerIndex<TinkerEdge> edgeIndex = null;
- protected final IdManager<?> vertexIdManager;
- protected final IdManager<?> edgeIdManager;
- protected final IdManager<?> vertexPropertyIdManager;
+ protected IdManager<?> vertexIdManager;
+ protected IdManager<?> edgeIdManager;
+ protected IdManager<?> vertexPropertyIdManager;
Review Comment:
Is the removal of `final` here required?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1189067462
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -146,6 +146,24 @@ See: link:https://issues.apache.org/jira/browse/TINKERPOP-2819[TINKERPOP-2819]
distributions. Any app which makes use of both `gremlin-driver` and `gremlin-server` will now need to directly
include both modules.
+==== Building and Running with JDK 17
Review Comment:
i suspect there are more places in the docs that make various proclamations about JDK versions. please search around for a bit and see what might need updating.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#issuecomment-1555093849
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] kenhuuu commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1188897552
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -103,9 +103,9 @@ public final class TinkerGraph implements Graph {
protected TinkerIndex<TinkerVertex> vertexIndex = null;
protected TinkerIndex<TinkerEdge> edgeIndex = null;
- protected final IdManager<?> vertexIdManager;
- protected final IdManager<?> edgeIdManager;
- protected final IdManager<?> vertexPropertyIdManager;
+ protected IdManager<?> vertexIdManager;
+ protected IdManager<?> edgeIdManager;
+ protected IdManager<?> vertexPropertyIdManager;
Review Comment:
I tried to explain this in the commit message.
> The final modifier was removed in some TinkerGraph manager fields as these were removed during testing with reflection. However, as of Java 18 there is no reasonable way to use reflection to do this. Also, the class is marked final which should reduce its impact.
Please let me know if you have further concerns about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] kenhuuu merged pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu merged PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] codecov-commenter commented on pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#issuecomment-1537051255
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2056?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2056](https://app.codecov.io/gh/apache/tinkerpop/pull/2056?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c7d4ba0) into [master](https://app.codecov.io/gh/apache/tinkerpop/commit/ef14f264426c30bbd20b970517483cbb063de0c7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ef14f26) will **decrease** coverage by `4.45%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #2056 +/- ##
============================================
- Coverage 68.96% 64.51% -4.45%
============================================
Files 855 25 -830
Lines 41543 3830 -37713
Branches 5630 0 -5630
============================================
- Hits 28650 2471 -26179
+ Misses 10906 1187 -9719
+ Partials 1987 172 -1815
```
[see 830 files with indirect coverage changes](https://app.codecov.io/gh/apache/tinkerpop/pull/2056/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1189064192
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/jsr223/ScriptEngineCacheTest.java:
##########
@@ -35,6 +37,9 @@ public void shouldBeUtilityClass() throws Exception {
@Test
public void shouldGetEngineFromCache() {
+ String javaVersion = System.getProperty("java.version");
Review Comment:
is there another way to test this that perhaps doesn't require us to use nashorn? that way we could avoid binding to a particular jdk version/type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1189065266
##########
pom.xml:
##########
@@ -198,6 +198,22 @@ limitations under the License.
<log4j-silent.properties>file:target/test-classes/log4j-silent.properties</log4j-silent.properties>
<muteTestLogs>false</muteTestLogs>
+ <jdk17JvmArgs>
Review Comment:
a comment with some details on why this is here and when/if all/any of it can be removed in the future would be nice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] kenhuuu commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1195759068
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/jsr223/ScriptEngineCacheTest.java:
##########
@@ -35,6 +37,9 @@ public void shouldBeUtilityClass() throws Exception {
@Test
public void shouldGetEngineFromCache() {
+ String javaVersion = System.getProperty("java.version");
Review Comment:
There doesn't seem to be any default ScriptEngines that ship with the JDK anymore. To actually test the current method as is we would probably need to add another ScriptEngineFactory that the service provider could load. There isn't an easy way to mock this and still test the actual implementation of the cache. I'm not sure if it is worth adding those other pieces to make the ScriptEngineManager load a ScriptEngine just to test this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] xiazcy commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1187609968
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -146,6 +146,24 @@ See: link:https://issues.apache.org/jira/browse/TINKERPOP-2819[TINKERPOP-2819]
distributions. Any app which makes use of both `gremlin-driver` and `gremlin-server` will now need to directly
include both modules.
+==== Building and Running with JDK 17
Review Comment:
Should we also add a section in the developer doc section for this?
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -146,6 +146,24 @@ See: link:https://issues.apache.org/jira/browse/TINKERPOP-2819[TINKERPOP-2819]
distributions. Any app which makes use of both `gremlin-driver` and `gremlin-server` will now need to directly
include both modules.
+==== Building and Running with JDK 17
Review Comment:
Should we also add a section in the developer doc for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] kenhuuu commented on a diff in pull request #2056: TINKERPOP-2703 Build on JDK17
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2056:
URL: https://github.com/apache/tinkerpop/pull/2056#discussion_r1188895278
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -146,6 +146,24 @@ See: link:https://issues.apache.org/jira/browse/TINKERPOP-2819[TINKERPOP-2819]
distributions. Any app which makes use of both `gremlin-driver` and `gremlin-server` will now need to directly
include both modules.
+==== Building and Running with JDK 17
Review Comment:
Yeah, that is a good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org