Skip to content

feat(storage): reject fast open configs that are not a single read - #14833

Draft
Dhriti07 wants to merge 1 commit into
fast-open/02-hand-out-first-readfrom
fast-open/03-validate-fast-open-config
Draft

Dhriti07 wants to merge 1 commit into
fast-open/02-hand-out-first-readfrom
fast-open/03-validate-fast-open-config

Conversation

@Dhriti07

Copy link
Copy Markdown
Contributor

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.

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +229 to +233
assertThrows(
IllegalArgumentException.class,
() ->
dataClient.readSession(
OPEN, CTX, ImmutableList.of(ReadProjectionConfigs.asSeekableChannel())));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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()");

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant