Repository navigation
Conversation
xlangx imported jobopts to read the --resource_hints flag. The root beam package imports xlangx, so every binary importing beam registered the jobopts flags (--endpoint, --job_name, --async, --experiments and more) on flag.CommandLine, and binaries defining flags with any of those names panicked at startup with "flag redefined". Look the flag up by name instead, and move the hint parsing into the resource package, keeping jobopts.GetPipelineResourceHints as a wrapper.
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly resolves flag redefinition panics (#40730) caused by xlangx importing jobopts (which is transitively imported by the root beam package). Moving resource hint parsing to resource.ParseHints, updating jobopts.stringSlice.Get() to return []string, and looking up --resource_hints via flag.Lookup in xlangx effectively decouples the packages while preserving cross-language resource hint forwarding.
🔍 General Feedback
- The architectural separation between
options/resource,options/jobopts, andcore/runtime/xlangxis clean and aligns well with the approach used in #40729. - Unit tests thoroughly cover the new flag lookup behavior,
flag.Gettercontract onstringSlice, hint parsing, and regression prevention against job submission flag registration inxlangx.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40944 +/- ##
===========================================
- Coverage 79.83% 56.43% -23.41%
===========================================
Files 494 876 +382
Lines 83575 154761 +71186
===========================================
+ Hits 66722 87339 +20617
- Misses 16853 65566 +48713
- Partials 0 1856 +1856
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:
|
|
Assigning reviewers: R: @jrmccluskey for label go. This pull request likely touches a core component ("core" label). Please review with scrutiny. 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). |
|
Bringing in @derrickaw since he worked on the first PR addressing this issue |
@shunping might be better with the option changes since he has been doing a lot of design work with this recently. Thanks |
|
Ack! I will take a look this week. |
Fixes #40730.
#40729 removed the
gcpoptsimport fromgcsx. This change handles the other regression listed in the issue. #40379 madecore/runtime/xlangximportoptions/joboptsto read--resource_hints. The rootbeampackage importsxlangx, so any binary that importsbeamnow registers all of jobopts' flags onflag.CommandLine:--endpoint,--job_name,--async,--experiments,--parallelismand the rest. A binary or test that defines a flag with one of those names panics at startup.The repro from the issue, run unchanged as a test in a separate module against each Beam version:
Both #40379 and #40729 are in
sdks/v2.78.0-RC1. 2.78.0 therefore fixes the--projectcollision but still has this one.Change
xlangxnow looks up--resource_hintsby name withflag.Lookup, asgcsx.GetGcsEndpointdoes since [Go] Avoid importing gcpopts in gcsx and dataflowlib to prevent flag collisions #40729. If the flag isn't registered, nothing could have set it, and no hints are forwarded.jobopts.GetPipelineResourceHintsinto a newresource.ParseHints, which both packages can use without registering flags.jobopts.GetPipelineResourceHintsstays as a wrapper, so its API is unchanged.jobopts'stringSlice.Get()returns a[]stringrather than the unexportedstringSlicetype, so the value can be read throughflag.Getterfrom outside the package.After this,
go list -deps ./go/pkg/beamno longer listsoptions/jobopts, as in v2.77.0.The issue's long-term point, that runners register flags in
init(), is out of scope here.Testing
New tests:
TestNoJobSubmissionFlags(xlangx): asserts the xlangx test binary has no--endpoint,--job_nameor--resource_hintsflag. If thejoboptsimport is put back inexpand.go, it fails withflag --endpoint is registered (usage "Job service endpoint (required)."), plus the same message for the other two flags.TestPipelineResourceHints(xlangx): covers the lookup with the flag unregistered, and with two--resource_hintsvalues.TestResourceHintsFlagGetter(jobopts): pins the[]stringresult ofGet()that xlangx relies on. IfGet()returnssagain, it fails.TestParseHints(resource): the previous jobopts parsing test, moved with the same inputs and assertions.I also checked that the #40379 behaviour still works end to end, through exported API only. A test module imports
jobopts(as a runner does), sets--resource_hints=min_ram=2GB, and callsbeam.TryCrossLanguageagainst a handler registered withxlangx.RegisterHandler. The capturedExpansionRequesthasbeam:option:resource_hints:v1=["min_ram=2000000000B"], both on v2.78.0-RC1 and with this change.Run locally:
go testforoptions/...,core/runtime/xlangx/...,runners/universal/...,runners/dataflowand the rootbeampackage: all pass.gofmt -l,go vet --copylocks=false --unsafeptr=falseandstaticcheck2026.2 on the touched packages: all clean.The full
./go/pkg/...run and the cross-language integration tests are left to CI.