-
Notifications
You must be signed in to change notification settings - Fork 4.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
[BEAM-3617] Revert "Reinstate proto round trip in Java DirectRunner" #4609
Conversation
This reverts commit c0cb28c.
@@ -74,4 +76,10 @@ public Integer create(PipelineOptions options) { | |||
return Math.max(Runtime.getRuntime().availableProcessors(), MIN_PARALLELISM); | |||
} | |||
} | |||
|
|||
@Experimental(Kind.CORE_RUNNERS_ONLY) | |||
@Default.Boolean(false) |
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.
If the default is False, why is it causing performance degradation?
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.
(This is the revert; HEAD doesn't contain this flag, it always does the round trip)
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.
This is fine as release triage.
@@ -74,4 +76,10 @@ public Integer create(PipelineOptions options) { | |||
return Math.max(Runtime.getRuntime().availableProcessors(), MIN_PARALLELISM); | |||
} | |||
} | |||
|
|||
@Experimental(Kind.CORE_RUNNERS_ONLY) | |||
@Default.Boolean(false) |
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.
(This is the revert; HEAD doesn't contain this flag, it always does the round trip)
We should probably do the same for the Flink runner. I'm not certain, since runners mostly grab out the field and hang onto them, but it is possible that there's something round-tripped and repeatedly deserialized. |
The HBase failure is common across gradle and maven. The Kinesis failure is the one I am trying to sickbay. |
retest this please |
If HBase failure is on |
retest this please |
See #4627 as I think the failures are legitimate. |
This reverts commit c0cb28c.
This commit introduced a severe performance degradation (on some query pipelines, the direct runner is 10 times longer between version 2.2.0 and 2.3.0).
The idea is to revert this change for Beam 2.3.0 and give us time to investigate.
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue.mvn clean verify
to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.