xh123637 commented on PR #41021: URL: https://github.com/apache/beam/pull/41021#issuecomment-6757615411
Thanks for the review, @shunping — done in eac3a97, comments only, no logic changes: - `metrics.go`: the 11-line comment is now 4 lines — it just says Setup and bundle contexts for the same transform share one counterset instead of diverging into separate metric objects, with the `apache/beam#27038` reference kept. - `combine.go`: the "Without it..." sentence is deleted as suggested; the comment now only says the Setup context is tagged with the transform ID as ParDo does, with the issue reference kept. - `setup_metrics_test.go`: also trimmed the long file-header comment and the smaller helper comments the same way. Verified locally with Go 1.26.2: `gofmt` clean, `pkg/beam/core/metrics` passes, `pkg/beam/core/runtime/exec` passes in full including all four Setup-metrics tests (`TestSetupMetricsWithContext`, `TestSetupMetricsWithoutContext`, `TestSetupMetricsSharedWithBundle`, `TestCombineSetupMetricsAttribution`). `go vet` reports only the pre-existing `translate.go:95` finding, untouched by this change. On the Go tests run attempt 2 "Go Build" failure: I read that attempt's job log. The only failing package was `pkg/beam/io/textio` — `TestReadGzip` failed with "TextIO restriction lies outside the file being read. Restriction begins at 25 bytes, but file is only 9 bytes." That test is in SDF file-splitting code this PR does not touch (this PR only changes the metrics counterset sharing and `Combine.Up`'s Setup context), and both `pkg/beam/core/metrics` and `pkg/beam/core/runtime/exec` passed in that same attempt. The re-run (attempt 3) passed fully, including `textio`, with no code change — so it looks like a flaky failure unrelated to this PR, and nothing needed fixing here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
