Skip to content

feat(Datastore): Swap the default transport from HttpJson to gRPC#12977

Open
lqiu96 wants to merge 4 commits intomainfrom
datastore-swap-default-transport
Open

feat(Datastore): Swap the default transport from HttpJson to gRPC#12977
lqiu96 wants to merge 4 commits intomainfrom
datastore-swap-default-transport

Conversation

@lqiu96
Copy link
Copy Markdown
Member

@lqiu96 lqiu96 commented May 1, 2026

No description provided.

Copy link
Copy Markdown
Contributor

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

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 transitions the default transport for Datastore from HTTP to gRPC, updating the DatastoreOptions builder and tests. Review feedback identifies several redundant assignments in the Builder class that should be removed to maintain idiomatic patterns and avoid logic errors, specifically in the constructors and the setTransportOptions method.

Comment on lines 143 to +145
this.channelProvider = validateChannelProvider(options.channelProvider);
this.host = options.getHost();
this.transportOptions = options.getTransportOptions();
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.

high

These assignments are redundant and potentially introduce a bug. The super(options) call already copies the host and transportOptions fields from the provided options object. By calling options.getHost() and options.getTransportOptions(), you are resolving the defaults and storing them as explicit values in the builder. This breaks the toBuilder() pattern where a user might want to change the transport and have the host automatically switch to its corresponding default. Per repository guidelines, reuse pre-configured objects from options classes directly instead of creating new ones and copying settings.

Suggested change
this.channelProvider = validateChannelProvider(options.channelProvider);
this.host = options.getHost();
this.transportOptions = options.getTransportOptions();
this.channelProvider = validateChannelProvider(options.channelProvider);
References
  1. Reuse pre-configured objects from options classes directly instead of creating new ones and copying settings.

Comment on lines 155 to 156
this.transportOptions = transportOptions;
return super.setTransportOptions(transportOptions);
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

The assignment this.transportOptions = transportOptions; is redundant because super.setTransportOptions(transportOptions) already performs this assignment in the base class.

Suggested change
this.transportOptions = transportOptions;
return super.setTransportOptions(transportOptions);
return super.setTransportOptions(transportOptions);

@lqiu96 lqiu96 requested a review from jinseopkim0 May 1, 2026 19:20
@lqiu96 lqiu96 marked this pull request as ready for review May 1, 2026 19:20
@lqiu96 lqiu96 requested a review from a team as a code owner May 1, 2026 19:20
@lqiu96 lqiu96 enabled auto-merge (squash) May 2, 2026 19:53
@lqiu96 lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 2, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 2, 2026
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.

4 participants