Repository navigation
Conversation
Third step of fast open BiDi reads (go/fast-open-bidi-read, b/552263062). A fast open read is sent as one range in the open request, so only configs that make a single read on the stream (asFutureBytes, asFutureByteString, asChannel) can be one. asSeekableChannel issues reads as the caller seeks, so it cannot. readSession now rejects it with an IllegalArgumentException before anything is sent, instead of failing with an UnsupportedOperationException while creating the read.
There was a problem hiding this comment.
Code Review
This pull request introduces a validation check in StorageDataClient to ensure that all fast open read configurations are of type ProjectionType.STREAM_READ, throwing an IllegalArgumentException if they are not. A corresponding integration test has been added to verify this behavior. The reviewer suggests enhancing the test by asserting the specific exception message to ensure the validation fails for the expected reason.
| assertThrows( | ||
| IllegalArgumentException.class, | ||
| () -> | ||
| dataClient.readSession( | ||
| OPEN, CTX, ImmutableList.of(ReadProjectionConfigs.asSeekableChannel()))); |
There was a problem hiding this comment.
When asserting that an exception is thrown, it is best practice to also verify the exception message. Since readSession has multiple checkArgument validations that throw IllegalArgumentException, asserting on the message ensures that the test fails for the expected reason rather than a different validation failure.
IllegalArgumentException exception = assertThrows(
IllegalArgumentException.class,
() ->
dataClient.readSession(
OPEN, CTX, ImmutableList.of(ReadProjectionConfigs.asSeekableChannel())));
assertThat(exception)
.hasMessageThat()
.contains("a fast open read must be asFutureBytes(), asFutureByteString() or asChannel()");
3/n for fast open BiDi reads
A fast open read is sent as one range in the open request, so only configs that make a single read on the stream (asFutureBytes, asFutureByteString, asChannel) can be one. asSeekableChannel issues reads as the caller seeks, so it cannot. readSession now rejects it with an IllegalArgumentException before anything is sent, instead of failing with an UnsupportedOperationException while creating the read.