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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
