You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "Abacn (via GitHub)" <gi...@apache.org> on 2023/03/08 15:01:41 UTC

[GitHub] [beam] Abacn opened a new pull request, #25763: LOG error for illegal ramBytes

Abacn opened a new pull request, #25763:
URL: https://github.com/apache/beam/pull/25763

   We have report that users pass in values like `withMinRam(1073741824 * 16)` crashes the pipeline after job starts. This is due to java literal int overflow: `1073741824 * 16 = 0`. While this can be avoided by setting up spotbug guard. It is nice to have a more obvious logging for this error.
   
   **Please** add a meaningful description for your change here
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
    - [ ] 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/get-started-contributing/#make-the-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Go tests](https://github.com/apache/beam/workflows/Go%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #25763:
URL: https://github.com/apache/beam/pull/25763#issuecomment-1466917616

   Run Java_Azure_IO_Direct PreCommit


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] robertwb commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1130276274


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   At the very least, let's log the error for anything <1G which is probably an unintentional overflow. (I have my doubts as to whether anyone will see this log, but let's at least drop a TODO to turn it into a hard error.)



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1130223042


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   Like what we usually do to introduce breaking change



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #25763:
URL: https://github.com/apache/beam/pull/25763#issuecomment-1466917151

   Run Java_GCP_IO_Direct PreCommit


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn merged pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn merged PR #25763:
URL: https://github.com/apache/beam/pull/25763


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1132558616


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   > will this surface as an error in Cloud Logging
   
   Yes it does



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] liferoad commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1130373530


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   Sounds good. BTW: if DataflowRunner is used, will this surface as an error in Cloud Logging?



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #25763: LOG error for illegal ramBytes

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25763:
URL: https://github.com/apache/beam/pull/25763#issuecomment-1460344571

   Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
   
   R: @kennknowles for label java.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review 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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1132803365


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,25 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes. A number greater than 1G
+   *     (1_000_000_000L) is typical.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      // TODO(yathu) ignore invalid value as of Beam v2.47.0. throw error in future version.
+      LOG.error(
+          "Encountered invalid non-positive minimum ram hint value {}.\n"

Review Comment:
   added hint in both LOG message



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #25763:
URL: https://github.com/apache/beam/pull/25763#issuecomment-1466917818

   Run Java_Pulsar_IO_Direct PreCommit


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] liferoad commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1130068841


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   Will raising an exception or forcing this to be at least 1G break the users's current 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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1130222764


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   Thanks for the comments.
   
   > Will raising an exception ... break the users's current jobs?
   
   Though in practical setting a small value does not make sense, formally it is still a breaking change. So I chose to use an "error" level logging for now and switch to checkArgument it in later versions



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] robertwb commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1132653173


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,25 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes. A number greater than 1G
+   *     (1_000_000_000L) is typical.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      // TODO(yathu) ignore invalid value as of Beam v2.47.0. throw error in future version.
+      LOG.error(
+          "Encountered invalid non-positive minimum ram hint value {}.\n"
+              + "The value is ignored. In the future, this will throw an IllegalArgumentException.",
+          ramBytes);
+      return this;
+    } else if (ramBytes < 1_000_000_000L) {

Review Comment:
   Integer.MAX_VALUE?



##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,25 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes. A number greater than 1G
+   *     (1_000_000_000L) is typical.
+   */
   public ResourceHints withMinRam(long ramBytes) {

Review Comment:
   What if we took a Long rather than a long? In this case, ints would not be accidentally promoted. (We'd want to drop a comment as to why.)



##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,25 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes. A number greater than 1G
+   *     (1_000_000_000L) is typical.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      // TODO(yathu) ignore invalid value as of Beam v2.47.0. throw error in future version.
+      LOG.error(
+          "Encountered invalid non-positive minimum ram hint value {}.\n"

Review Comment:
   For both of these errors, the likely cause is passing in an (overflowing) int expression. This may be non-obvious to the user, we should suggest that Longs are required here. 



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #25763:
URL: https://github.com/apache/beam/pull/25763#issuecomment-1466916952

   Run Java_Azure_IO_Direct PreCommit


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #25763:
URL: https://github.com/apache/beam/pull/25763#issuecomment-1466917395

   Run Java_Kafka_IO_Direct PreCommit


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] Abacn commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1132803952


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,25 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes. A number greater than 1G
+   *     (1_000_000_000L) is typical.
+   */
   public ResourceHints withMinRam(long ramBytes) {

Review Comment:
   Warned user for changing function signature as part of TODO



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] robertwb commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1129788604


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   I might even raise an exception. To be safe, we could require at least 1G here to ensure it's a long. (Anything less than that is not meaningfully actionable anyway.)



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] liferoad commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1129659572


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   error or warning?



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] liferoad commented on a diff in pull request #25763: LOG error for illegal ramBytes

Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #25763:
URL: https://github.com/apache/beam/pull/25763#discussion_r1132703000


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/resourcehints/ResourceHints.java:
##########
@@ -207,8 +210,19 @@ public int hashCode() {
     }
   }
 
-  /** Sets desired minimal available RAM size to have in transform's execution environment. */
+  /**
+   * Sets desired minimal available RAM size to have in transform's execution environment.
+   *
+   * @param ramBytes specifies a positive RAM size in bytes.
+   */
   public ResourceHints withMinRam(long ramBytes) {
+    if (ramBytes <= 0L) {
+      LOG.error(

Review Comment:
   SG.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org