Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40672 +/- ##
=============================================
- Coverage 56.16% 0 -56.17%
=============================================
Files 1124 0 -1124
Lines 178336 0 -178336
Branches 1489 0 -1489
=============================================
- Hits 100170 0 -100170
+ Misses 75645 0 -75645
+ Partials 2521 0 -2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR updates the Java Dataflow runner to defer Dataflow API root URL resolution to the Google API client library when neither --apiRootUrl nor an absolute --dataflowEndpoint is explicitly configured, enabling GOOGLE_CLOUD_UNIVERSE_DOMAIN support. The implementation, nullability annotations, documentation updates, and unit test coverage in DataflowTransportTest are clean and well-structured.
🔍 General Feedback
- Removing
@Default.String(Dataflow.DEFAULT_ROOT_URL)fromDataflowPipelineDebugOptions#getApiRootUrl()and conditionally callingbuilder.setRootUrl(...)inDataflowTransportaccurately avoids settingisUserConfiguredEndpoint = trueinAbstractGoogleClient.Builder, preserving universe domain resolution. - The subtle behavior change for callers inspecting
getApiRootUrl()(now returningnullwhen unset) is appropriately annotated with@Nullableand documented inCHANGES.md.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| @Nullable String getApiRootUrl(); | ||
|
|
||
| void setApiRootUrl(String value); | ||
| void setApiRootUrl(@Nullable String value); |
There was a problem hiding this comment.
minor: For optional pipeline option, we mark get method as nullable but usually not set method, e.g.
and set to empty string should work equally. There are violations though, but generally good to keep this pattern
apiRootUrl defaulted to https://dataflow.googleapis.com/, so DataflowTransport always called setRootUrl, which stopped google-api-client from honoring GOOGLE_CLOUD_UNIVERSE_DOMAIN. apiRootUrl now has no default, and the root URL and service path are set only when apiRootUrl or dataflowEndpoint is given.
Part of #40569, Builds upon #40670 (needs universe domain support from googleapis/google-api-java-client#2435)
Problem
The Java Dataflow runner always sends requests to
https://dataflow.googleapis.com/, even whenGOOGLE_CLOUD_UNIVERSE_DOMAINpoints to another universe.apiRootUrldefaults tohttps://dataflow.googleapis.com/, andDataflowTransportalways callsDataflow.Builder#setRootUrl. In google-api-client, callingsetRootUrl(even with the default URL) marks the endpoint as user-configured, so the library never builds thehttps://dataflow.<universe>/endpoint.Changes
apiRootUrlno longer has a default.DataflowTransportsets the root URL and service path only whenapiRootUrlordataflowEndpointis set. Otherwise the client library resolves the endpoint, honoringGOOGLE_CLOUD_UNIVERSE_DOMAINand falling back to the public URL.dataflowEndpointapiRootUrlapiRootUrl(+ path)dataflowEndpointBreaking change
DataflowPipelineDebugOptions.getApiRootUrl()now returnsnullby default instead ofhttps://dataflow.googleapis.com/.