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.

Sign up for GitHub

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

Jump to bottom

Introduce a BoundedTrie metric which is used to efficiently store and… #33385

Open

rohitsinha54 wants to merge 18 commits into apache:master from rohitsinha54:user-bounded-trie

Contributor

rohitsinha54 commented Dec 16, 2024 •

edited

Loading

… aggregate a collection of string sequences (FQNs) with a limited size.

It is recommended to review this PR by commits.

BoundedTrie is a space-saving way to store many string sequences (like FQN/file paths). It acts like a tree with branches, holding sequences within a size limit. It can efficiently add, combine, and search and perform trimming of children when the size increases beyond defined max.

Let's say we want to store these sequences, with a size limit of 3:

"folder1/file1.txt"
"folder1/file2.txt"
"folder2/file3.txt"
Here's how the BoundedTrie might look:

root
  - folder1
     - file1.txt
     - file2.txt
  - folder2
     - file3.txt

If we try to add "folder1/file4.txt", the trie might trim to "folder1", dropping all children to stay within the size limit.

This will be used to replace the StringSet metric for lineage tracking for very large lineage graphs to overcome the size limits.

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.

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 or the workflows README to see a list of phrases to trigger workflows.


          Introduce a BoundedTrie metric which is used to efficiently store and…

38ae8e9

… aggregate a collection of string sequences (FQNs) with a limited size.

github-actions bot added java runners core labels

Contributor Author

rohitsinha54 commented Dec 16, 2024

Contributor

github-actions bot commented Dec 16, 2024

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

rohitsinha54 commented

View reviewed changes

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

robertwb reviewed

View reviewed changes

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieCell.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated

+                  } else if (other.root().isPresent() && other.singleton().isPresent()) {
+                    return this;
+                  } else {
+                    BoundedTrieNode combined = new BoundedTrieNode(asTrie());

Contributor

robertwb Dec 16, 2024

Same as above.

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/BoundedTrieResult.java Outdated Show resolved Hide resolved

github-actions bot added the dataflow label

rohitsinha54 force-pushed the user-bounded-trie branch from c9f106b to 01ffe0f Compare

December 19, 2024 21:22

github-actions bot added the jet label

rohitsinha54 force-pushed the user-bounded-trie branch 3 times, most recently from 5a5ddae to ebbe4f9 Compare

December 19, 2024 21:49

github-actions bot removed dataflow jet labels


          Address comments, perform deep copies and support synchronization for…

35e0a8c

… mutable BoundedTrieData

rohitsinha54 force-pushed the user-bounded-trie branch from ebbe4f9 to 35e0a8c Compare

December 19, 2024 23:52

Contributor Author

rohitsinha54 commented Dec 19, 2024

@robertwb : Please have another look. Thank you very much.

rohitsinha54 added 3 commits

December 19, 2024 23:25


          Add more unit tests for trienode and fix merge on empty node

d76412a


          Add more unit test for BoundedTrieData which are not already covered …

10f68d0

…in fuzzy run tests


          Add unit tests for Cell

ba96765

rohitsinha54 commented

View reviewed changes

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/BoundedTrieResult.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java

+                }
+                /** Returns a new {@link BoundedTrieData} instance that is a deep copy of this instance. */
+                public synchronized BoundedTrieData getCumulative() {

Contributor Author

rohitsinha54 Dec 20, 2024

This returns a BoundedTrieData over a Set to be consistent with other MetricData in java SDK

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Outdated

+                 *
+                 * @param other The other {@link BoundedTrieData} to combine with.
+                 */
+                public synchronized void combine(@Nonnull BoundedTrieData other) {

Contributor Author

rohitsinha54 Dec 20, 2024

This return a void rather than a reference to updated object because the object is mutable and returning a reference from here will then require unnecessary overhead of using AtomicReference in BoundedTrieCell to safely be able to handle combine call from mutli-threads.

rohitsinha54 added 4 commits

December 21, 2024 13:53


          Plumb BoundedTrie to MetricsContainerImpl

bfe3de0


          Plumb BoundedTrie to StreamingStepMetricsContainer

4b0b070


          Plumb BoundedTrie to StreamingStepMetricsContainer

3c5d613


          Plumb BoundedTrie to MetricQueryResults

724d310

github-actions bot added dataflow jet labels


          Plumb BoundedTrie to MetricsContainerStepMap and make combine return …

c0bb3f1

…reference

rohitsinha54 added 4 commits

December 23, 2024 12:52


          Add test in MetricsContainerStepMapTest

f9faf23


          Plumb BoundedTrie to DirectMetrics

0c8aa32


          Add BoundedTrieResult class

aaaff29


          Plumb BoundedTrieResult and BoundedTrie in DataflowMetrics

386c362

github-actions bot added the direct label

rohitsinha54 added 2 commits

December 25, 2024 09:36


          Plumb BoundedTrie JetMetric and PortableMetric

7a2509c


          Add test for BoundedTrieResult and some more plumbing

83b3d0d

github-actions bot added extensions portability labels


          Fixes for github checks

269c5d5

Contributor Author

rohitsinha54 commented Dec 25, 2024

The 2 failing checks
:runners:google-cloud-dataflow-java:worker:test and :sdks:java:io:mqtt:test both passed locally so they seem to be transient failure in the workflow.


          Multi-threaded test in MetricsContainerImplTest and some clean up

034c4e6

rohitsinha54 mentioned this pull request

Btrie disable flag #33457

Open

3 tasks

robertwb reviewed

View reviewed changes

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java Show resolved Hide resolved

...ogle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowMetrics.java

+                    if (metricUpdate.get(BOUNDED_TRIE) == null) {
+                      return BoundedTrieResult.empty();
+                    }
+                    BoundedTrie bTrie = (BoundedTrie) metricUpdate.get(BOUNDED_TRIE);

Contributor

robertwb Dec 27, 2024

How has this code been tested?

...ogle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowMetrics.java

		private static final Logger LOG = LoggerFactory.getLogger(DataflowMetrics.class);
		// TODO (rosinha): Remove this once bounded_trie is available in metrics proto Dataflow

Contributor

robertwb Dec 27, 2024

I would be more comfortable excluding these changes from this CL until the dataflow client protos are updated. We can do this in a follow-up PR (this one is already huge enough) rather than have a dummy implementation here (especially given the issues we had last time with class cast exceptions that suddenly manifest when the service is updated). The most important is that such metrics are reported and the round trip works on direct runners.

...r/src/main/java/org/apache/beam/runners/dataflow/worker/MetricsToCounterUpdateConverter.java

+                  return new CounterUpdate()
+                      .setStructuredNameAndMetadata(name)
+                      .setCumulative(false)
+                      .set("bounded_trie", boundedTrieData.toProto());

Contributor

robertwb Dec 27, 2024

Will the dataflow worker even accept this yet? Again, let's pull the legacy worker changes out into their own PR.

runners/portability/java/src/main/java/org/apache/beam/runners/portability/PortableMetrics.java Show resolved Hide resolved

sdks/java/core/src/test/java/org/apache/beam/sdk/metrics/MetricsTest.java

                     UsesCounterMetrics.class,
                     UsesDistributionMetrics.class,
                     UsesGaugeMetrics.class,
-                    UsesStringSetMetrics.class
+                    UsesStringSetMetrics.class,
+                    UsesBoundedTrieMetrics.class

Contributor

robertwb Dec 27, 2024

Does this mean we lose (ordinary) counter coverage for runners not yet supporting bounded trie metrics?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core dataflow direct extensions java jet portability runners