gemini-code-assist[bot] commented on code in PR #38470: URL: https://github.com/apache/beam/pull/38470#discussion_r3227460632
########## sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java: ########## @@ -153,6 +153,13 @@ public interface BigQueryOptions void setStorageWriteMaxInflightBytes(Long value); + @Description( + "Maximum time in seconds a Storage Write API append request is allowed to wait in the " + + "request callback queue before timing out. Overrides Storage Write API default (5 min)") + Integer getStorageWriteApiMaxRequestCallbackWaitTimeSec(); + + void setStorageWriteApiMaxRequestCallbackWaitTimeSec(Integer value); Review Comment:  Consider using `Long` instead of `Integer` for consistency with other BigQuery options (like `storageWriteMaxInflightRequests`) and to match the parameter type of `java.time.Duration.ofSeconds()`. Additionally, adding a `@Default` value is recommended because this setting affects a global static variable in the underlying `StreamWriter` library. Providing a default ensures that every pipeline explicitly sets the state, preventing side effects between different pipelines running in the same JVM. ```suggestion @Description( "Maximum time in seconds a Storage Write API append request is allowed to wait in the " + "request callback queue before timing out. Defaults to 300 seconds (5 minutes).") @Default.Long(300) Long getStorageWriteApiMaxRequestCallbackWaitTimeSec(); void setStorageWriteApiMaxRequestCallbackWaitTimeSec(Long value); ``` ########## sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java: ########## @@ -1496,6 +1496,7 @@ public static class WriteStreamServiceImpl implements WriteStreamService { private final BigQueryWriteClient newWriteClient; private final long storageWriteMaxInflightRequests; private final long storageWriteMaxInflightBytes; + private final @Nullable Integer storageWriteApiMaxRequestCallbackWaitTimeSec; Review Comment:  Change the field type to `long` to match the updated `BigQueryOptions`. Since a default value will be provided in the options, this field no longer needs to be nullable. ```suggestion private final long storageWriteApiMaxRequestCallbackWaitTimeSec; ``` ########## sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java: ########## @@ -1578,6 +1583,11 @@ public StreamAppendClient getStreamAppendClient( options.as(BigQueryOptions.class).getMaxConnectionPoolConnections()) .build()); + if (storageWriteApiMaxRequestCallbackWaitTimeSec != null) { + StreamWriter.setMaxRequestCallbackWaitTime( + java.time.Duration.ofSeconds(storageWriteApiMaxRequestCallbackWaitTimeSec)); + } Review Comment:  With the move to a primitive `long` and a default value in the options, the null check is no longer necessary. It is also safer to ensure the value is positive before passing it to the library. ```suggestion if (storageWriteApiMaxRequestCallbackWaitTimeSec > 0) { StreamWriter.setMaxRequestCallbackWaitTime( java.time.Duration.ofSeconds(storageWriteApiMaxRequestCallbackWaitTimeSec)); } ``` -- 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]
