feat(datastore): add support for request tags by rahulmane-goog · Pull Request #13732 · googleapis/google-cloud-java

rahulmane-goog · 2026-07-13T13:26:55Z

Add RequestOptions support that enables ability of passing request tags to Datastore queries.
Introduced instance level request tags in the DatastoreOptions.
Introduced the logic of populating RequestOptions for all datastore queries. These are query level tags.
Introduced overloaded run(...) and runAggregation(...) client methods, that pass RequestOptions proto as the last parameter.
The instance and query level tags are combined using the new RequestOptionsHelper utility and sent inside the query requests.

gemini-code-assist

Code Review

This pull request introduces support for passing and merging RequestOptions (such as request tags) in Datastore queries and aggregation queries, allowing both instance-level and request-level tags to be propagated. The feedback highlights a critical issue where instance-level tags are ignored when query-level RequestOptions are not explicitly provided in QueryResultsImpl and AggregationQueryRequestProtoPreparer. Additionally, the reviewer suggests simplifying the varargs withRequestTags overload in Datastore by delegating to the List version, and adding a test case to verify that instance-level tags are correctly populated and sent with queries.

rahulmane-goog

Please review

jinseopkim0 · 2026-07-13T20:21:26Z

ci / lint is failing. Could you please take a look?

jinseopkim0 · 2026-07-13T20:03:02Z

      return this;
    }

+    public Builder setTags(ImmutableList<String> requestTags) {


Would it be better to use setReplaceTags?

I think setXx inherently means that. Setting the existing with a new value which is effectively a replace.

Sorry, I don't follow. Could you please explain a bit more?

I meant that because we are setting requestTags and since we have getRequestTags(), I'm thinking it'll be better to stay consistent. Having the builder methods match the property/getter name is standard convention in the SDK and helps users discover the setters via IDE auto-complete.

I think in your original comment "Would it be better to use setReplaceTags?" you intended to say setRequestTags instead of replace, and hence the confusion. I have renamed the method to setRequestTags()

jinseopkim0 · 2026-07-13T20:03:07Z

+      return this;
+    }
+
+    public Builder setTags(String... requestTags) {


Would it be better to use setReplaceTags?

I think setXx inherently means that. Setting the existing with a new value which is effectively a replace.

Sorry, I don't follow. Could you please explain a bit more?

I meant that because we are setting requestTags and since we have getRequestTags(), I'm thinking it'll be better to stay consistent. Having the builder methods match the property/getter name is standard convention in the SDK and helps users discover the setters via IDE auto-complete.

Replied in the previous comment.

lqiu96 · 2026-07-13T20:39:56Z

+  <T> QueryResults<T> run(Query<T> query, RequestOptions requestOptions);
+
+  /**
+   * Submits a {@link Query} with specified {@link com.google.datastore.v1.RequestOptions} and
+   * returns its result. {@link ReadOption}s can be specified if desired.
+   */
+  <T> QueryResults<T> run(Query<T> query, RequestOptions requestOptions, ReadOption... options);
+
+  /**
+   * Submits a {@link Query} with specified {@link com.google.cloud.datastore.models.ExplainOptions}
+   * and {@link com.google.datastore.v1.RequestOptions} and returns its result. {@link ReadOption}s
+   * can be specified if desired.
+   */
+  @BetaApi
+  <T> QueryResults<T> run(
+      Query<T> query,
+      ExplainOptions explainOptions,
+      RequestOptions requestOptions,
+      ReadOption... options);


This looks like we have to overload run and runAggregation every single time we add a variation of ...Option in here.

Would it be possible to just create a wrapping Option class (e.g. DatastoreExecutionOption or w/e name that wraps these three options)?

That way any new request option doesn't require an overload of new public methods for the Datastore interface

We actually dont foresee any additional options coming in near future. Shall we park this refactoring for later?

I know some of the changes above from older implementations aren't related to your feature. However, even if we don't ever add in any new option variant, this PR will still add in 3 new public methods that can't be removed without breaking changes.

If we are to add anything new to the public surface, I would suggest that go for the variant that only adds in one (unless there is a valid reason to not do this):

e.g. run(Query, DatastoreExecutionOption) that manages to wrap all current options.

I think we can just delegate all the existing variants to create this new root options wrapper

Same below with runAggregation

lqiu96 · 2026-07-13T20:41:14Z

+  default Datastore withRequestTags(String... requestTags) {
+    return withRequestTags(java.util.Arrays.asList(requestTags));
+  }


do we need a varargs variant? Why can't the list overload below work for all use cases?

Makes sense, removed it.

I think the method still exists upsteam. Might have forgot to push to remote

rahulmane-goog · 2026-07-14T06:26:15Z

ci / lint is failing. Could you please take a look?

Fixed

…uplication

jinseopkim0 · 2026-07-15T14:24:41Z

      return this;
    }

+    public Builder setTags(ImmutableList<String> requestTags) {


Sorry, I don't follow. Could you please explain a bit more?

I meant that because we are setting requestTags and since we have getRequestTags(), I'm thinking it'll be better to stay consistent. Having the builder methods match the property/getter name is standard convention in the SDK and helps users discover the setters via IDE auto-complete.

jinseopkim0 · 2026-07-15T14:25:04Z

+      return this;
+    }
+
+    public Builder setTags(String... requestTags) {


Sorry, I don't follow. Could you please explain a bit more?

I meant that because we are setting requestTags and since we have getRequestTags(), I'm thinking it'll be better to stay consistent. Having the builder methods match the property/getter name is standard convention in the SDK and helps users discover the setters via IDE auto-complete.

jinseopkim0 · 2026-07-15T14:31:33Z

+   * @param requestTags the request tags to append to existing ones
+   */
+  default Datastore withRequestTags(List<String> requestTags) {
+    ImmutableList.Builder<String> builder = ImmutableList.builder();


Can the requestTags be null here? And if so, would it make sense to just return the current client rather than build a new one?

e.g.

if (requestTags == null || requestTags.isEmpty()) { return this; }

jinseopkim0 · 2026-07-15T14:40:07Z

+    CommitRequest.Builder requestPb =
+        CommitRequest.newBuilder()
+            .setMode(CommitRequest.Mode.NON_TRANSACTIONAL)
+            .setProjectId(getOptions().getProjectId())
+            .setDatabaseId(getOptions().getDatabaseId())
+            .addAllMutations(mutationsPb);


Do we need to support instance-level tags here? e.g.

if (!getOptions().getRequestTags().isEmpty()) { requestPb.setRequestOptions(RequestOptionsHelper.createRequestOptions(getOptions(), null)); }

jinseopkim0 · 2026-07-15T14:40:48Z

Do we need to support instance-level tags here? e.g.

if (!getOptions().getRequestTags().isEmpty()) { requestPb.setRequestOptions(RequestOptionsHelper.createRequestOptions(getOptions(), null)); }

jinseopkim0 · 2026-07-15T14:41:34Z

@@ -425,23 +508,21 @@ public List<Key> allocateId(IncompleteKey... keys) {
    if (keys.length == 0) {
      return Collections.emptyList();
    }
-    com.google.datastore.v1.AllocateIdsRequest.Builder requestPb =
-        com.google.datastore.v1.AllocateIdsRequest.newBuilder();
+    AllocateIdsRequest.Builder requestPb = AllocateIdsRequest.newBuilder();
    for (IncompleteKey key : keys) {
      requestPb.addKeys(trimNameOrId(key).toPb());
    }
    requestPb.setProjectId(getOptions().getProjectId());
    requestPb.setDatabaseId(getOptions().getDatabaseId());


Do we need to support instance-level tags here? e.g.

if (!getOptions().getRequestTags().isEmpty()) { requestPb.setRequestOptions(RequestOptionsHelper.createRequestOptions(getOptions(), null)); }

lqiu96 · 2026-07-15T16:54:34Z

+  /**
+   * Returns a new Datastore client with the specified request tags added.
+   *
+   * @param requestTags the request tags to append to existing ones
+   */
+  default Datastore withRequestTags(List<String> requestTags) {
+    ImmutableList.Builder<String> builder = ImmutableList.builder();
+    builder.addAll(getOptions().getRequestTags());
+    builder.addAll(requestTags);
+    return getOptions().toBuilder().setTags(builder.build()).build().getService();
+  }


qq, what is the user need to re-build a Datastore client with new request tags via withRequestTags(...)? I'm not entirely sure I follow why we need a new default method on the Datastore interface.

Why can't the user just set the tag in the DatastoreOption builder and then manually create the client again

e.g. something like
DatastoreOptions.toBuilder().setTags(...).build.getService()

rahulmane-goog requested review from a team as code owners July 13, 2026 13:26

gemini-code-assist Bot reviewed Jul 13, 2026

View reviewed changes

rahulmane-goog force-pushed the feat-datastore-request-tags branch from f124116 to aab1fd3 Compare July 13, 2026 13:43

rahulmane-goog commented Jul 13, 2026

View reviewed changes

lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 13, 2026

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 13, 2026

jinseopkim0 reviewed Jul 13, 2026

View reviewed changes

lqiu96 reviewed Jul 13, 2026

View reviewed changes

Comment thread ...astore/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java Outdated

lqiu96 reviewed Jul 13, 2026

View reviewed changes

Comment thread ...astore/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java

lqiu96 reviewed Jul 13, 2026

View reviewed changes

Comment thread ...astore/google-cloud-datastore/src/main/java/com/google/cloud/datastore/QueryResultsImpl.java Outdated

lqiu96 reviewed Jul 13, 2026

View reviewed changes

Comment thread java-datastore/google-cloud-datastore/src/main/java/com/google/cloud/datastore/ReadOption.java

rahulmane-goog closed this Jul 14, 2026

rahulmane-goog reopened this Jul 14, 2026

rahulmane-goog added 2 commits July 15, 2026 06:47

feat(datastore): add support for request tags

f1311d9

fix: address code review comments regarding instance-level tags and d…

5f377f8

…uplication

jinseopkim0 reviewed Jul 15, 2026

View reviewed changes

lqiu96 reviewed Jul 15, 2026

View reviewed changes

Fixed review comments

e5396c0

rahulmane-goog force-pushed the feat-datastore-request-tags branch from 6c23afc to e5396c0 Compare July 15, 2026 18:04

Uh oh!

Conversation

rahulmane-goog commented Jul 13, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulmane-goog left a comment

Choose a reason for hiding this comment

Uh oh!

jinseopkim0 commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulmane-goog commented Jul 14, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lqiu96 Jul 15, 2026 •

edited

Loading