-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic Opeartor Metric Support For Data Shuffle (GBK and Combine Per Key) Operators for Samza Runner #26649
Conversation
…ey) Operators for Samza Runner
R: @xinyuiscool please take a look |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
runners/samza/src/main/java/org/apache/beam/runners/samza/metrics/SamzaInputGBKMetricOp.java
Outdated
Show resolved
Hide resolved
runners/samza/src/main/java/org/apache/beam/runners/samza/metrics/SamzaOutputGBKMetricOp.java
Outdated
Show resolved
Hide resolved
.../samza/src/main/java/org/apache/beam/runners/samza/metrics/SamzaTransformMetricRegistry.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clean up. One more refactoring comment for the previous patch. Please take a look.
if (isDataShuffleTransform(urn)) { | ||
return new SamzaGBKMetricOp<>( | ||
pValue, transformName, opType, samzaTransformMetricRegistry); | ||
} | ||
return new SamzaInputMetricOp(pValue, transformName, samzaTransformMetricRegistry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to GBKMetricOp, can we also merge SamzaInputMetricOp and SamzaOutputMetricOp into one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup it did make sense to simplify this, done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Run Java PreCommit |
…ey) Operators for Samza Runner (apache#26649)
…ey) Operators for Samza Runner (apache#26649)
…ey) Operators for Samza Runner (apache#26649)
…ey) Operators for Samza Runner (apache#26649)
…ey) Operators for Samza Runner (apache#26649)
Issue
#26456
Summary
Tests
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.