feat(datastore): add support for request tags - #13732
Conversation
There was a problem hiding this comment.
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.
f124116 to
aab1fd3
Compare
|
ci / lint is failing. Could you please take a look? |
Fixed |
d61b477 to
75e872b
Compare
|
Thanks! I think the changes are looking good. I've added another round of comments and smaller changes for readability. One question that probably depends on plans regarding stability of request tags: When do we feel comfortable marking it from @BetaApi to GA? I'm thinking that the methods in the Datastore interface that do not have DatastoreExecutionOptions probably should be marked with Perhaps it may be better to mark the requestTag getters/ settings with @BetaApi instead? Since 2/3 of DatastoreExecutionOptions (Explain and Read Options are stable, just the request tags is new) |
ea088e1 to
c2d9004
Compare
We are planning to do a public preview of supporting request tags , the backend changes are already enabled. Should I create a separate bug for doing what you suggested? |
Sure, I think that also works. We don't need to encourage people to use the new variants until they stabilize. In that case, I think it may also be worthwhile to add |
|
Changes generally LGTM, added some small comments. Please also let @jinseopkim0 another chance to review and I'll be fine to approve |
d3a5393 to
04a178a
Compare
9efbab9 to
c0b0c8d
Compare
411128b to
efde3da
Compare
… in DatastoreImpl
acd9d7e to
57f258d
Compare
| toReadOptionsPb(executionOptions.getReadOptions().toArray(new ReadOption[0])), | ||
| query, | ||
| explainOptions != null ? explainOptions.toPb() : null, | ||
| executionOptions.getRequestOptions() != null |
There was a problem hiding this comment.
IIUC, we should have enforced that requestOptions cannot be null. There is a default value which should have a default instance.
| ? executionOptions.getExplainOptions().toPb() | ||
| : null; | ||
| RequestOptions requestOptions = | ||
| executionOptions.getRequestOptions() != null |
lqiu96
left a comment
There was a problem hiding this comment.
LGTM, thanks for the changes and working with us on this!
This PR introduced sending request tags for different datastore requests. The tags can be passed using 2 mechanisms: