You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/11/05 20:42:00 UTC
[GitHub] [beam] riteshghorse opened a new pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
riteshghorse opened a new pull request #15911:
URL: https://github.com/apache/beam/pull/15911
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on pull request #15911:
URL: https://github.com/apache/beam/pull/15911#issuecomment-966654496
Okay, so each of LiftedCombine, MergeAccumulators, and ExtractOutput have to track the ProcessElement's phase in their respective methods. StartBundle and FinishBundle in LiftedCombine however seems to be calling the Combine's method. So for these two, we have to track their phase in Combine itself. Am I understanding it correct?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on pull request #15911:
URL: https://github.com/apache/beam/pull/15911#issuecomment-966674271
Run Go PostCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on pull request #15911:
URL: https://github.com/apache/beam/pull/15911#issuecomment-966670802
Got it.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #15911:
URL: https://github.com/apache/beam/pull/15911#issuecomment-966668074
That's right.
We should however, still add one to the `(*LiftedCombine).FinishBundle()` call at the top of the function (rather than in the loop.). The Lifted Combine isn't calling Combine's FinishBundle until the end.
In practice, a LiftedCombine is always immeadiately followed by a DataSink, whose process element is what gets called. I don't know if we need to be separating the DataSource and DataSink into it's their PID metrics just yet, vs folding them into the their adjacent transforms. That's a later change we can make once we determine it's worthwhile. At that point, it's likely moot whether we're attributing anything to the LiftedCombine's FinishBundle since it's doing so little work anyway.
Adding it to the top of the FinishBundle will at least attribute all that work to the LiftedCombine properly instead of attributing them to the previous DoFn's FinishBundle call, which is what it's currently doing, which is certainly incorrect.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on pull request #15911:
URL: https://github.com/apache/beam/pull/15911#issuecomment-962174007
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck merged pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
Posted by GitBox <gi...@apache.org>.
lostluck merged pull request #15911:
URL: https://github.com/apache/beam/pull/15911
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on pull request #15911:
URL: https://github.com/apache/beam/pull/15911#issuecomment-964536333
R: @lostluck
Completely forgot to request your review on this PR while waiting for the test to finish.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse edited a comment on pull request #15911: [BEAM-13001] collect DoFn metrics for Combine
Posted by GitBox <gi...@apache.org>.
riteshghorse edited a comment on pull request #15911:
URL: https://github.com/apache/beam/pull/15911#issuecomment-966654496
Okay, so each of LiftedCombine, MergeAccumulators, and ExtractOutput have to track the ProcessElement's phase in their respective methods. StartBundle and FinishBundle for LiftedCombine however seems to be calling the Combine's method. So for them we have to track their phase in Combine itself. Am I understanding it correct?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org