Repository navigation
Conversation
Fourth step of fast open BiDi reads (go/fast-open-bidi-read, b/552263062). Pins how fast open behaves when things go wrong, using the existing read stream: - when the server fails the stream with OUT_OF_RANGE, the open fails with that error and is not retried; - when the server rejects only the fast open range (OUT_OF_RANGE naming its read id), the session re-opens without it and readAs fails with the error. This differs from the "fail open" choice in the design doc; it is kept as is until the error shape zonal buckets return is confirmed; - when the connection drops before any reply, the retried open sends the fast open range again; - when the connection drops part way through the fast open range, the re-open asks only for the remaining bytes and readAs returns the full range.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds several unit tests to ITFastOpenFakeTest.java to verify the behavior of ObjectReadSession under various server failure and connection drop scenarios. The feedback suggests improving the exception assertions in these tests by using instanceof with anyMatch instead of exact class equality checks with Throwable::getClass, which is more robust against subclasses and avoids stream collection overhead.
| assertThat( | ||
| Throwables.getCausalChain(e).stream() | ||
| .<Class<?>>map(Throwable::getClass) | ||
| .collect(toList())) | ||
| .contains(OutOfRangeException.class); |
There was a problem hiding this comment.
Using Throwable::getClass with .contains(OutOfRangeException.class) performs an exact class equality check. If a subclass of OutOfRangeException is thrown, this assertion will fail. Using instanceof (via anyMatch) is more robust as it correctly handles subclasses, is more idiomatic, and avoids the overhead of collecting the stream to a list. Additionally, this allows removing the unused import static java.util.stream.Collectors.toList;.
assertThat(Throwables.getCausalChain(e).stream().anyMatch(t -> t instanceof OutOfRangeException))
.isTrue();| assertThat( | ||
| Throwables.getCausalChain(e).stream() | ||
| .<Class<?>>map(Throwable::getClass) | ||
| .collect(toList())) | ||
| .contains(OutOfRangeException.class); |
There was a problem hiding this comment.
Using Throwable::getClass with .contains(OutOfRangeException.class) performs an exact class equality check. If a subclass of OutOfRangeException is thrown, this assertion will fail. Using instanceof (via anyMatch) is more robust as it correctly handles subclasses, is more idiomatic, and avoids the overhead of collecting the stream to a list. Additionally, this allows removing the unused import static java.util.stream.Collectors.toList;.
assertThat(Throwables.getCausalChain(e).stream().anyMatch(t -> t instanceof OutOfRangeException))
.isTrue();
Fourth step of fast open BiDi reads (go/fast-open-bidi-read, b/552263062).
Pins three things fast open gets from the existing read stream: