You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@nemo.apache.org by GitBox <gi...@apache.org> on 2020/01/31 14:57:27 UTC
[GitHub] [incubator-nemo] wonook opened a new pull request #281: [NEMO-437]
Support Java version 11
wonook opened a new pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281
JIRA: [NEMO-437: Support Java version 11](https://issues.apache.org/jira/projects/NEMO/issues/NEMO-437)
**Major changes:**
- Bump java libraries to recent versions
- `beam` 2.11.0 --> 2.18.0
- `grpc` 1.7.0 --> 1.26.0
- `mockito` 2.13.0 --> 3.2.4
- `powermock` 2.0.0-beta --> 2.0.4
**Minor changes to note:**
- None
**Tests for the changes:**
- I've included new JDK requirements to the travis script
- Existing tests pass
**Other comments:**
- I've referred to the following PRs for upgrading Beam: [link1](https://github.com/apache/beam/pull/7635/files), [link2](https://github.com/apache/beam/pull/9275/files)
Closes #
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] johnyangk merged pull request #281: [NEMO-437]
Support Java version 11
Posted by GitBox <gi...@apache.org>.
johnyangk merged pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] johnyangk commented on a change in pull request
#281: [NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
johnyangk commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r373953701
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/PipelineTranslationContext.java
##########
@@ -69,6 +71,7 @@
this.pValueToTag = new HashMap<>();
this.loopVertexStack = new Stack<>();
this.pipelineOptions = pipelineOptions;
+ this.currentTransform = null;
Review comment:
Please avoid assigning null values.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] johnyangk commented on a change in pull request
#281: [NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
johnyangk commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r373954367
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/PipelineVisitor.java
##########
@@ -41,12 +41,16 @@ public PipelineVisitor(final Pipeline pipeline, final NemoPipelineOptions pipeli
@Override
public void visitPrimitiveTransform(final TransformHierarchy.Node node) {
+ context.setCurrentTransform(node);
pipelineTranslator.translatePrimitive(context, node);
+ context.setCurrentTransform(null);
Review comment:
Please avoid assigning nulls.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] johnyangk commented on a change in pull request
#281: [NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
johnyangk commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r373957675
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/PipelineTranslationContext.java
##########
@@ -55,6 +56,7 @@
private final Map<PValue, TupleTag<?>> pValueToTag;
private final Stack<LoopVertex> loopVertexStack;
private final Pipeline pipeline;
+ private AppliedPTransform<?, ?, ?> currentTransform;
Review comment:
Can you perhaps do without this? Something like this has not been needed, because the current transform that's being visited is explicitly provided through the Beam PipelineVisitor interface.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] wonook commented on a change in pull request #281:
[NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
wonook commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r374505450
##########
File path: README.md
##########
@@ -17,7 +17,7 @@ Please refer to the [Contribution guideline](.github/CONTRIBUTING.md) to contrib
## Nemo prerequisites and setup
### Prerequisites
-* Java 8
+* Java 11
Review comment:
Sure, it's done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] johnyangk commented on a change in pull request
#281: [NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
johnyangk commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r374487257
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/transform/CreateViewTransform.java
##########
@@ -137,6 +137,11 @@ public MultiView(final Iterable<T> iterable) {
this.iterable = iterable;
}
+ @Override
+ public Iterable<Void> get() {
+ return null;
Review comment:
A short comment on this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] johnyangk commented on a change in pull request
#281: [NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
johnyangk commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r374488308
##########
File path: README.md
##########
@@ -17,7 +17,7 @@ Please refer to the [Contribution guideline](.github/CONTRIBUTING.md) to contrib
## Nemo prerequisites and setup
### Prerequisites
-* Java 8
+* Java 11
Review comment:
Does Java 8 still work?
Maybe something like this, so 8/9/10 users first give it a try?:
Java 8 or higher (mostly tested on Java 11)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] wonook commented on issue #281: [NEMO-437] Support
Java version 11
Posted by GitBox <gi...@apache.org>.
wonook commented on issue #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#issuecomment-581774267
@johnyangk Thanks! I've resolved your comments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] wonook commented on a change in pull request #281:
[NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
wonook commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r374484224
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/PipelineVisitor.java
##########
@@ -41,12 +41,16 @@ public PipelineVisitor(final Pipeline pipeline, final NemoPipelineOptions pipeli
@Override
public void visitPrimitiveTransform(final TransformHierarchy.Node node) {
+ context.setCurrentTransform(node);
pipelineTranslator.translatePrimitive(context, node);
+ context.setCurrentTransform(null);
Review comment:
Handled.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] wonook commented on a change in pull request #281:
[NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
wonook commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r374058388
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/PipelineTranslationContext.java
##########
@@ -55,6 +56,7 @@
private final Map<PValue, TupleTag<?>> pValueToTag;
private final Stack<LoopVertex> loopVertexStack;
private final Pipeline pipeline;
+ private AppliedPTransform<?, ?, ?> currentTransform;
Review comment:
If you look at the [PR](https://github.com/apache/beam/pull/7635/files) it's been implemented this way, and that's why I've implemented this in the way described here, but I'll try to follow your comments as I think that could be a better approach.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] johnyangk commented on a change in pull request
#281: [NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
johnyangk commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r373954281
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/PipelineTranslator.java
##########
@@ -443,6 +442,12 @@ private static AbstractDoFnTransform createDoFnTransform(final PipelineTranslati
Iterables.getOnlyElement(TransformInputs.nonAdditionalInputs(pTransform));
final HasDisplayData displayData = (builder) -> builder.add(DisplayData.item("name", beamNode.getFullName()));
+ final DoFnSchemaInformation doFnSchemaInformation;
+ if (ctx.getCurrentTransform() != null) {
+ doFnSchemaInformation = ParDoTranslation.getSchemaInformation(ctx.getCurrentTransform());
+ } else {
+ doFnSchemaInformation = DoFnSchemaInformation.create();
Review comment:
It's not straightforward to me why it creates a new schema information when the current transform is null.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] wonook commented on issue #281: [NEMO-437] Support
Java version 11
Posted by GitBox <gi...@apache.org>.
wonook commented on issue #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#issuecomment-581752694
@johnyangk I've addressed your comments. Please check! Thanks :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] wonook commented on a change in pull request #281:
[NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
wonook commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r374484208
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/PipelineTranslator.java
##########
@@ -443,6 +442,12 @@ private static AbstractDoFnTransform createDoFnTransform(final PipelineTranslati
Iterables.getOnlyElement(TransformInputs.nonAdditionalInputs(pTransform));
final HasDisplayData displayData = (builder) -> builder.add(DisplayData.item("name", beamNode.getFullName()));
+ final DoFnSchemaInformation doFnSchemaInformation;
+ if (ctx.getCurrentTransform() != null) {
+ doFnSchemaInformation = ParDoTranslation.getSchemaInformation(ctx.getCurrentTransform());
+ } else {
+ doFnSchemaInformation = DoFnSchemaInformation.create();
Review comment:
Handled.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] wonook commented on a change in pull request #281:
[NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
wonook commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r374484210
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/PipelineTranslationContext.java
##########
@@ -69,6 +71,7 @@
this.pValueToTag = new HashMap<>();
this.loopVertexStack = new Stack<>();
this.pipelineOptions = pipelineOptions;
+ this.currentTransform = null;
Review comment:
Handled.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] johnyangk merged pull request #281: [NEMO-437]
Support Java version 11
Posted by GitBox <gi...@apache.org>.
johnyangk merged pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nemo] wonook commented on a change in pull request #281:
[NEMO-437] Support Java version 11
Posted by GitBox <gi...@apache.org>.
wonook commented on a change in pull request #281: [NEMO-437] Support Java version 11
URL: https://github.com/apache/incubator-nemo/pull/281#discussion_r374505475
##########
File path: compiler/frontend/beam/src/main/java/org/apache/nemo/compiler/frontend/beam/transform/CreateViewTransform.java
##########
@@ -137,6 +137,11 @@ public MultiView(final Iterable<T> iterable) {
this.iterable = iterable;
}
+ @Override
+ public Iterable<Void> get() {
+ return null;
Review comment:
👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services