gemini-code-assist[bot] commented on code in PR #36165:
URL: https://github.com/apache/beam/pull/36165#discussion_r2350321361
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/WindmillSink.java:
##########
@@ -71,15 +71,29 @@ class WindmillSink<T> extends Sink<WindowedValue<T>> {
this.context = context;
}
+ private static ByteString encodeMetadata(
+ ByteStringOutputStream stream,
+ Coder<Collection<? extends BoundedWindow>> windowsCoder,
+ Collection<? extends BoundedWindow> windows,
+ PaneInfo paneInfo)
+ throws IOException {
+ try {
+ PaneInfoCoder.INSTANCE.encode(paneInfo, stream);
+ windowsCoder.encode(windows, stream, Coder.Context.OUTER);
+ return stream.toByteStringAndReset();
+ } catch (Throwable e) {
+ stream.toByteStringAndReset();
+ throw e;
+ }
Review Comment:
Catching `Throwable` is generally discouraged as it includes `Error`s (like
`OutOfMemoryError`) from which the application is not expected to recover. It's
better to catch `Exception` instead. This would also be more consistent with
other similar `try-catch` blocks in this file, such as in the `encode` method.
```suggestion
} catch (Exception e) {
stream.toByteStringAndReset();
throw e;
}
```
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/state/WindmillStateUtil.java:
##########
@@ -26,22 +27,36 @@
class WindmillStateUtil {
+ private static final ThreadLocal<SoftReference<ByteStringOutputStream>>
threadLocalOutputStream =
+ new ThreadLocal<>();
+
/** Encodes the given namespace and address as {@code
<namespace>+<address>}. */
@VisibleForTesting
static ByteString encodeKey(StateNamespace namespace, StateTag<?> address) {
+ // Use ByteStringOutputStream rather than concatenation and String.format.
We build these keys
+ // a lot, and this leads to better performance results. See associated
benchmarks.
+ ByteStringOutputStream stream = getByteStringOutputStream();
try {
- // Use ByteStringOutputStream rather than concatenation and
String.format. We build these keys
- // a lot, and this leads to better performance results. See associated
benchmarks.
- ByteStringOutputStream stream = new ByteStringOutputStream();
// stringKey starts and ends with a slash. We separate it from the
// StateTag ID by a '+' (which is guaranteed not to be in the stringKey)
because the
// ID comes from the user.
namespace.appendTo(stream);
stream.append('+');
address.appendTo(stream);
- return stream.toByteString();
- } catch (IOException e) {
+ return stream.toByteStringAndReset();
+ } catch (IOException | RuntimeException e) {
+ stream.toByteStringAndReset();
throw new RuntimeException(e);
}
Review Comment:
This `catch` block wraps any caught exception in a new `RuntimeException`.
While this is necessary for a checked `IOException`, it unnecessarily wraps an
existing `RuntimeException` in another one, making stack traces harder to read.
To avoid this, you could split this into two separate `catch` blocks for
`IOException` and `RuntimeException`.
```suggestion
} catch (IOException e) {
stream.toByteStringAndReset();
throw new RuntimeException(e);
} catch (RuntimeException e) {
stream.toByteStringAndReset();
throw e;
}
```
--
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]
