You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "rohitsinha54 (via GitHub)" <gi...@apache.org> on 2025/01/02 00:46:30 UTC
[PR] Btrie legacy worker [beam]
rohitsinha54 opened a new pull request, #33474:
URL: https://github.com/apache/beam/pull/33474
This PR is based on top of https://github.com/apache/beam/pull/33457
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `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, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on code in PR #33474:
URL: https://github.com/apache/beam/pull/33474#discussion_r1943183171
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/MetricsToCounterUpdateConverter.java:
##########
@@ -111,6 +118,46 @@ public static CounterUpdate fromStringSet(
.setStringList(stringList);
}
+ public static CounterUpdate fromBoundedTrie(
+ MetricKey key, boolean isCumulative, BoundedTrieData boundedTrieData) {
+ // BoundedTrie uses SET kind metric aggregation which tracks unique strings as a trie.
+ CounterStructuredNameAndMetadata name = structuredNameAndMetadata(key, Kind.SET);
+ BoundedTrie counterUpdateTrie = getBoundedTrie(boundedTrieData.toProto());
Review Comment:
https://yaqs.corp.google.com/eng/q/775343714070429696
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb merged PR #33474:
URL: https://github.com/apache/beam/pull/33474
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #33474:
URL: https://github.com/apache/beam/pull/33474#discussion_r1943352418
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/MetricsToCounterUpdateConverter.java:
##########
@@ -111,6 +118,46 @@ public static CounterUpdate fromStringSet(
.setStringList(stringList);
}
+ public static CounterUpdate fromBoundedTrie(
+ MetricKey key, boolean isCumulative, BoundedTrieData boundedTrieData) {
+ // BoundedTrie uses SET kind metric aggregation which tracks unique strings as a trie.
Review Comment:
This doesn't seem 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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on code in PR #33474:
URL: https://github.com/apache/beam/pull/33474#discussion_r1943398739
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/MetricsToCounterUpdateConverter.java:
##########
@@ -111,6 +118,46 @@ public static CounterUpdate fromStringSet(
.setStringList(stringList);
}
+ public static CounterUpdate fromBoundedTrie(
+ MetricKey key, boolean isCumulative, BoundedTrieData boundedTrieData) {
+ // BoundedTrie uses SET kind metric aggregation which tracks unique strings as a trie.
Review Comment:
I was just looking in that if we can omit it and still cook up something internally to work.
This is only used on router translation logic when we translate from CounterUpdate to Aggregation to determine combiner type:
1. https://source.corp.google.com/piper///depot/google3/cloud/dataflow/router/translation/v1beta3.cc;l=1384
2. https://source.corp.google.com/piper///depot/google3/cloud/dataflow/router/translation/v1beta3.cc;l=1083
For BoundedTrie at least for now we don't need kind switch as we have only one kind.
I do not have a strict preference either ways on this one. Let me know if you prefer one way over the other.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2629054305
R: @robertwb
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #33474:
URL: https://github.com/apache/beam/pull/33474#discussion_r1943387050
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/MetricsToCounterUpdateConverter.java:
##########
@@ -111,6 +118,46 @@ public static CounterUpdate fromStringSet(
.setStringList(stringList);
}
+ public static CounterUpdate fromBoundedTrie(
+ MetricKey key, boolean isCumulative, BoundedTrieData boundedTrieData) {
+ // BoundedTrie uses SET kind metric aggregation which tracks unique strings as a trie.
Review Comment:
OK. I suppose this value isn't actually used anywhere, and is reasonable enough as informative metadata.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2638052621
I'm still seeing
```
2025-02-05T18:44:03.8370351Z > Task :runners:google-cloud-dataflow-java:worker:test
2025-02-05T18:44:03.8371552Z
2025-02-05T18:44:03.8373642Z org.apache.beam.runners.dataflow.worker.BatchModeExecutionContextTest > extractMetricUpdatesBoundedTrie FAILED
```
in the failed test run.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2638413308
Yes. I was trying to get the tests to pass, but this does look unrelated (and possibly flaky). I'll go ahead and merge.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2637472422
The two failure seem unrelated to this change.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #33474:
URL: https://github.com/apache/beam/pull/33474#discussion_r1943404219
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/MetricsToCounterUpdateConverter.java:
##########
@@ -111,6 +118,46 @@ public static CounterUpdate fromStringSet(
.setStringList(stringList);
}
+ public static CounterUpdate fromBoundedTrie(
+ MetricKey key, boolean isCumulative, BoundedTrieData boundedTrieData) {
+ // BoundedTrie uses SET kind metric aggregation which tracks unique strings as a trie.
Review Comment:
Let's leave it as is for now.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2635765397
@robertwb Please take a look. Thanks.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2638088028
Sorry. Fixed.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on code in PR #33474:
URL: https://github.com/apache/beam/pull/33474#discussion_r1943376738
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/MetricsToCounterUpdateConverter.java:
##########
@@ -111,6 +118,46 @@ public static CounterUpdate fromStringSet(
.setStringList(stringList);
}
+ public static CounterUpdate fromBoundedTrie(
+ MetricKey key, boolean isCumulative, BoundedTrieData boundedTrieData) {
+ // BoundedTrie uses SET kind metric aggregation which tracks unique strings as a trie.
Review Comment:
When we create the CounterStructuredName for CounterUpdate we need to send an aggregation kind it is defined here:
https://source.corp.google.com/piper///depot/google3/google/dataflow/service/v1b3/work_items.proto;l=151;bpv=0;bpt=1
My thinking on this is: SET aggregation kind represent collection on of unique elements.
Which hold true here as our BoundedTrie implementation is nested hashmap i.e. it has unique elements and when n BoundedTrie from from n CounterUpdate is 'aggregrated' the result is one, containing unique elements from all of them.
We can use something else like a TRIE itself but unfortunately this definition needs to go in CounterUpdate proto and then have a inception push for our apis to be released followed up by google-client api release so we would not be able to make the definition on time.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2629055117
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment `assign set of reviewers`
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2567214838
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2638253640
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/33474?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 59.09%. Comparing base [(`fc43c12`)](https://app.codecov.io/gh/apache/beam/commit/fc43c12b6fc6a5fec8a34484794ac2389e496bed?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e6c55be`)](https://app.codecov.io/gh/apache/beam/commit/e6c55be0ce7b12871f35c26646860b75c44b8541?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 15 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #33474 +/- ##
=========================================
Coverage 59.08% 59.09%
- Complexity 3238 3240 +2
=========================================
Files 1156 1156
Lines 176924 176924
Branches 3391 3391
=========================================
+ Hits 104543 104552 +9
+ Misses 69015 69008 -7
+ Partials 3366 3364 -2
```
| [Flag](https://app.codecov.io/gh/apache/beam/pull/33474/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [java](https://app.codecov.io/gh/apache/beam/pull/33474/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.36% <ø> (+0.03%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/beam/pull/33474?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2638390013
Failing test `testOnNewWorkerMetadata_redistributesBudget`
is not related to change. I also ran locally and it passed.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2638391694
@robertwb can you please help merging this? Thanks.
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "rohitsinha54 (via GitHub)" <gi...@apache.org>.
rohitsinha54 commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2638450217
Thank you.
Created a CP to 2.63: https://github.com/apache/beam/pull/33890
--
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
Re: [PR] Add support for bounded trie metric in legacy worker [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33474:
URL: https://github.com/apache/beam/pull/33474#issuecomment-2629051713
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @johnjcasey added as fallback since no labels match configuration
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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