You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/11/03 14:17:35 UTC
[GitHub] [beam] je-ik commented on a change in pull request #13240: [BEAM-11146] Add copyFaster option to Flink runner
je-ik commented on a change in pull request #13240:
URL: https://github.com/apache/beam/pull/13240#discussion_r516514156
##########
File path: runners/flink/1.8/src/main/java/org/apache/beam/runners/flink/translation/types/CoderTypeSerializer.java
##########
@@ -82,10 +81,14 @@ public T createInstance() {
@Override
public T copy(T t) {
Review comment:
Maybe rename to `copyIfNeeded`, as when the flag is on, it doesn't copy anything.
##########
File path: runners/flink/1.8/src/main/java/org/apache/beam/runners/flink/translation/types/CoderTypeSerializer.java
##########
@@ -49,20 +50,18 @@
* org.apache.beam.sdk.io.FileSystems} registration needed for {@link
* org.apache.beam.sdk.transforms.Reshuffle} translation.
*/
- @SuppressWarnings("unused")
- private final @Nullable SerializablePipelineOptions pipelineOptions;
+ private final SerializablePipelineOptions pipelineOptions;
- public CoderTypeSerializer(Coder<T> coder) {
- Preconditions.checkNotNull(coder);
- this.coder = coder;
- this.pipelineOptions = null;
- }
+ private final boolean fasterCopy;
Review comment:
Small suggestion, can we name this `zeroCopy`, as that is actually what it is.
##########
File path: runners/flink/1.8/src/main/java/org/apache/beam/runners/flink/translation/types/CoderTypeSerializer.java
##########
@@ -82,10 +81,14 @@ public T createInstance() {
@Override
public T copy(T t) {
Review comment:
Ah, I missed the `@Override` annotation. Understood.
##########
File path: runners/flink/1.8/src/main/java/org/apache/beam/runners/flink/translation/types/CoderTypeSerializer.java
##########
@@ -49,20 +50,18 @@
* org.apache.beam.sdk.io.FileSystems} registration needed for {@link
* org.apache.beam.sdk.transforms.Reshuffle} translation.
*/
- @SuppressWarnings("unused")
- private final @Nullable SerializablePipelineOptions pipelineOptions;
+ private final SerializablePipelineOptions pipelineOptions;
- public CoderTypeSerializer(Coder<T> coder) {
- Preconditions.checkNotNull(coder);
- this.coder = coder;
- this.pipelineOptions = null;
- }
+ private final boolean fasterCopy;
Review comment:
Okay, what about `disableValueClone`, I think that `fasterCopy` is a little misleading as well. Faster than what? And why it was slower before? :) It would be good for the flag to describe direct effect (disable, enable something), not a consequence (being faster).
##########
File path: runners/flink/1.8/src/main/java/org/apache/beam/runners/flink/translation/types/CoderTypeSerializer.java
##########
@@ -49,20 +50,18 @@
* org.apache.beam.sdk.io.FileSystems} registration needed for {@link
* org.apache.beam.sdk.transforms.Reshuffle} translation.
*/
- @SuppressWarnings("unused")
- private final @Nullable SerializablePipelineOptions pipelineOptions;
+ private final SerializablePipelineOptions pipelineOptions;
- public CoderTypeSerializer(Coder<T> coder) {
- Preconditions.checkNotNull(coder);
- this.coder = coder;
- this.pipelineOptions = null;
- }
+ private final boolean fasterCopy;
Review comment:
To make it clear - although I put my comment to the internal field, I was having in mind mostly the flag in PipelineOptions. These should be aligned.
##########
File path: runners/flink/1.8/src/main/java/org/apache/beam/runners/flink/translation/types/CoderTypeSerializer.java
##########
@@ -49,20 +50,18 @@
* org.apache.beam.sdk.io.FileSystems} registration needed for {@link
* org.apache.beam.sdk.transforms.Reshuffle} translation.
*/
- @SuppressWarnings("unused")
- private final @Nullable SerializablePipelineOptions pipelineOptions;
+ private final SerializablePipelineOptions pipelineOptions;
- public CoderTypeSerializer(Coder<T> coder) {
- Preconditions.checkNotNull(coder);
- this.coder = coder;
- this.pipelineOptions = null;
- }
+ private final boolean fasterCopy;
Review comment:
Agree that this is not 100% important, but if possible, we should make options passed in PipelineOptions the most self-explanatory as possible, because these options are user-facing.
----------------------------------------------------------------
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