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/04/02 03:18:14 UTC
[GitHub] [beam] spoortikundargi opened a new pull request #11289:
[BEAM-9660]: Add an explicit check for integer overflow.
spoortikundargi opened a new pull request #11289: [BEAM-9660]: Add an explicit check for integer overflow.
URL: https://github.com/apache/beam/pull/11289
If `commitSize` is less than zero (due to overflow of integer serialized size), the existing code was setting it to `Integer.MAX_VALUE` and using the `estimatedCommitSize > byteLimit` check to throw an exception. However, in some cases in Dataflow Streaming Applicance, `byteLimit` is set to `Integer.MAX_VALUE` and so the check `estimatedCommitSize > byteLimit` fails to detect integer overflow.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
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] [beam] scwhittle commented on issue #11289: [BEAM-9660]: Add an
explicit check for integer overflow.
Posted by GitBox <gi...@apache.org>.
scwhittle commented on issue #11289: [BEAM-9660]: Add an explicit check for integer overflow.
URL: https://github.com/apache/beam/pull/11289#issuecomment-608207913
Thanks! LGTM
R: @reuvenlax for merging
----------------------------------------------------------------
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] [beam] reuvenlax merged pull request #11289: [BEAM-9660]: Add an
explicit check for integer overflow.
Posted by GitBox <gi...@apache.org>.
reuvenlax merged pull request #11289: [BEAM-9660]: Add an explicit check for integer overflow.
URL: https://github.com/apache/beam/pull/11289
----------------------------------------------------------------
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] [beam] scwhittle commented on a change in pull request #11289:
[BEAM-9660]: Add an explicit check for integer overflow.
Posted by GitBox <gi...@apache.org>.
scwhittle commented on a change in pull request #11289: [BEAM-9660]: Add an explicit check for integer overflow.
URL: https://github.com/apache/beam/pull/11289#discussion_r402466421
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java
##########
@@ -1372,11 +1372,13 @@ private void process(
WorkItemCommitRequest commitRequest = outputBuilder.build();
int byteLimit = maxWorkItemCommitBytes;
int commitSize = commitRequest.getSerializedSize();
- int estimatedCommitSize = commitSize < 0 ? Integer.MAX_VALUE : commitSize;
-
- // Detect overflow of integer serialized size or if the byte limit was exceeded.
- windmillMaxObservedWorkItemCommitBytes.addValue(estimatedCommitSize);
- if (estimatedCommitSize > byteLimit) {
+ // Detect overflow of integer serialized size.
+ if (commitSize < 0) {
+ throw KeyCommitTooLargeException.causedBy(computationId, byteLimit, commitRequest);
Review comment:
We still would prefer to send a truncation commit (from buildWorkItemTruncationRequest). That reports the commit as too large but sends the necessary info to the backend to attempt to truncate the work item for reprocessing.
----------------------------------------------------------------
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] [beam] spoortikundargi commented on a change in pull request
#11289: [BEAM-9660]: Add an explicit check for integer overflow.
Posted by GitBox <gi...@apache.org>.
spoortikundargi commented on a change in pull request #11289: [BEAM-9660]: Add an explicit check for integer overflow.
URL: https://github.com/apache/beam/pull/11289#discussion_r402693381
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java
##########
@@ -1372,11 +1372,13 @@ private void process(
WorkItemCommitRequest commitRequest = outputBuilder.build();
int byteLimit = maxWorkItemCommitBytes;
int commitSize = commitRequest.getSerializedSize();
- int estimatedCommitSize = commitSize < 0 ? Integer.MAX_VALUE : commitSize;
-
- // Detect overflow of integer serialized size or if the byte limit was exceeded.
- windmillMaxObservedWorkItemCommitBytes.addValue(estimatedCommitSize);
- if (estimatedCommitSize > byteLimit) {
+ // Detect overflow of integer serialized size.
+ if (commitSize < 0) {
+ throw KeyCommitTooLargeException.causedBy(computationId, byteLimit, commitRequest);
Review comment:
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] [beam] spoortikundargi commented on issue #11289: [BEAM-9660]: Add
an explicit check for integer overflow.
Posted by GitBox <gi...@apache.org>.
spoortikundargi commented on issue #11289: [BEAM-9660]: Add an explicit check for integer overflow.
URL: https://github.com/apache/beam/pull/11289#issuecomment-607598330
R: @scwhittle
----------------------------------------------------------------
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