Enable MapState and SetState for dataflow streaming engine pipelines with legacy runner by building on top of MultimapState. #31453

scwhittle · 2024-05-30T12:44:49Z

Tests were not enabled for MapState/SetState for appliance on legacy runner. Enabling tests shows that MapState wasn't conforming to the spec that a read iterator is unaffected by subsequent writes, so this was fixed. Additional tests for the new map state built on MultimapState, exposed that MultimapState had a bug with remove/put/remove/put sequence which was fixed.

Related to issue #18200

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.

github-actions · 2024-05-30T14:06:09Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

scwhittle · 2024-06-27T13:32:12Z

R: @acrites

github-actions · 2024-06-27T13:33:28Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

acrites

Just a couple small things I noticed.

acrites · 2024-06-28T16:29:19Z

...main/java/org/apache/beam/runners/dataflow/worker/windmill/state/WindmillMapViaMultimap.java

+  @Override
+  void initializeForWorkItem(
+      WindmillStateReader reader, Supplier<Closeable> scopedReadStateSupplier) {
+    super.cleanupAfterWorkItem();


Should this be initializeForWorkItem?

doh, fixed.

acrites · 2024-06-28T16:31:07Z

...main/java/org/apache/beam/runners/dataflow/worker/windmill/state/WindmillMapViaMultimap.java

+    @Override
+    public T read() {
+      Iterator<T> iterator = wrapped.read().iterator();
+      if (iterator.hasNext()) {


If `!hasNext()'?

Looks like the tests could use some improvement. I updated the validates runner test to cover a little more but I will also going to add unit tests to WindmillStateInternalsTest. I'll let you know when it's ready for another look.

Added some unit tests

acrites · 2024-07-01T20:34:27Z

.../java/org/apache/beam/runners/dataflow/worker/windmill/state/WindmillStateInternalsTest.java

@@ -763,6 +786,28 @@ public void testMultimapPutAndGet() {
    assertThat(result.read(), Matchers.containsInAnyOrder(1, 1, 2, 3));
  }

+  @Test
+  public void testMapViaMultimapPutAndGet() {


Some other edge cases that might be worth testing (unless I missed them somewhere):

Overwriting a key with a new value and then reading returns the new value.

Reading a key that hasn't been "put" returns correct value.

Put then Remove then Read returns empty.

added more tests and it exposed bug in multimapstate (improved multimap state test to expose it directly). During sequence of remove/put/remove/put multimapstate would remove the local knowledge of the remove.

acrites · 2024-07-01T20:36:30Z

.../java/org/apache/beam/runners/dataflow/worker/windmill/state/WindmillStateInternalsTest.java

+    waitAndSet(keysFuture, Arrays.asList(multimapEntry(key1), multimapEntry(key2)), 30);
+
+    mapState.put(key1, 7);
+    mapState.put(dup(key3), 8);


Just curious: why do we sometimes need to call this function?

it just copies the key to ensure that it is compared based upon value instead of shallow compararison

acrites · 2024-07-01T20:40:14Z

.../java/org/apache/beam/runners/dataflow/worker/windmill/state/WindmillStateInternalsTest.java

+    mapState.put(key1, 7);
+    mapState.put(dup(key3), 8);
+    mapState.put(key4, 1);
+    mapState.remove(key4);


I would also be interesting to test the case where key5 is returned by the backend read, but we've already called mapState.remove(key5) to make sure it's properly filtered out. I think this is basically the contents of testMultimapEntriesAndKeysMergeLocalRemove below, but not for MapViaMultimap.

acrites

Some more small comments about tests.

acrites

Nice!

CHANGES.md

with legacy runner by building on top of MultimapState.

scwhittle · 2024-07-04T19:16:33Z

Run Java PreCommit

…with legacy runner by building on top of MultimapState. (apache#31453)

github-actions bot added java runners dataflow core labels May 30, 2024

scwhittle force-pushed the layered_map_set_state branch 6 times, most recently from 91f0765 to 310042b Compare June 27, 2024 13:31

scwhittle force-pushed the layered_map_set_state branch from 310042b to 63627a6 Compare June 28, 2024 12:32

acrites suggested changes Jun 28, 2024

View reviewed changes

scwhittle force-pushed the layered_map_set_state branch 5 times, most recently from 24e1905 to b002791 Compare July 1, 2024 19:03

scwhittle requested a review from acrites July 1, 2024 19:04

acrites reviewed Jul 1, 2024

View reviewed changes

acrites suggested changes Jul 1, 2024

View reviewed changes

scwhittle force-pushed the layered_map_set_state branch from 0c743a7 to c586f41 Compare July 1, 2024 21:46

scwhittle requested a review from acrites July 1, 2024 21:47

acrites approved these changes Jul 1, 2024

View reviewed changes

svetakvsundhar assigned svetakvsundhar and unassigned svetakvsundhar Jul 3, 2024

svetakvsundhar self-requested a review July 3, 2024 14:49

svetakvsundhar reviewed Jul 3, 2024

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

scwhittle force-pushed the layered_map_set_state branch from 8e2ff30 to 3d4aab4 Compare July 4, 2024 17:41

scwhittle added 6 commits July 4, 2024 19:42

Enable MapState and SetState for dataflow streaming engine pipelines

4d90a21

with legacy runner by building on top of MultimapState.

improve tests

2d76025

fix vendoring

003ca36

add/fix tests, fix multimap add/remove/add/read exposed by test

51cf61a

CHANGES note, spotless

bce2ae0

fix tests

1c11d53

scwhittle force-pushed the layered_map_set_state branch from 3d4aab4 to 1c11d53 Compare July 4, 2024 17:46

scwhittle merged commit c08afea into apache:master Jul 4, 2024
33 checks passed

scwhittle deleted the layered_map_set_state branch July 4, 2024 20:22

acrites pushed a commit to acrites/beam that referenced this pull request Jul 17, 2024

Enable MapState and SetState for dataflow streaming engine pipelines …

73dc481

…with legacy runner by building on top of MultimapState. (apache#31453)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable MapState and SetState for dataflow streaming engine pipelines with legacy runner by building on top of MultimapState. #31453

Enable MapState and SetState for dataflow streaming engine pipelines with legacy runner by building on top of MultimapState. #31453

scwhittle commented May 30, 2024 •

edited

Loading

github-actions bot commented May 30, 2024

scwhittle commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

acrites left a comment

acrites Jun 28, 2024

scwhittle Jul 1, 2024

acrites Jun 28, 2024

scwhittle Jul 1, 2024

scwhittle Jul 1, 2024

acrites Jul 1, 2024

scwhittle Jul 1, 2024

acrites Jul 1, 2024

scwhittle Jul 1, 2024

acrites Jul 1, 2024

scwhittle Jul 1, 2024

acrites left a comment

acrites left a comment

scwhittle commented Jul 4, 2024

Enable MapState and SetState for dataflow streaming engine pipelines with legacy runner by building on top of MultimapState. #31453

Enable MapState and SetState for dataflow streaming engine pipelines with legacy runner by building on top of MultimapState. #31453

Conversation

scwhittle commented May 30, 2024 • edited Loading

GitHub Actions Tests Status (on master branch)

github-actions bot commented May 30, 2024

scwhittle commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

acrites left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrites left a comment

Choose a reason for hiding this comment

acrites left a comment

Choose a reason for hiding this comment

scwhittle commented Jul 4, 2024

scwhittle commented May 30, 2024 •

edited

Loading