[BEAM-7739] Implement ReadModifyWriteState in Python SDK #9067

rakeshcusat · 2019-07-14T07:15:25Z

ValueState is missing from python sdk though Java sdk has it. In order to have the feature parity we should have this implemented for Python sdk.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Gearpump	Samza
Go	---	---	---	---
Java
Python	---		---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

merging from upstream

robertwb · 2019-07-15T12:05:58Z

Thanks for looking at this. IIRC, the consensus last time this came up was to call it ReadModifyWriteState.

rakeshcusat · 2019-07-15T15:16:14Z

@robertwb Can you give me more context to that discussion or email chain that I can follow.
I was also talking to @pabloem offline and he mentioned that ValueState is controversial and java sdk doesn't allow it specifically when you are merging windows. I was just thinking of closing this PR.

If ReadModifyWriteState is not implemented yet then I will be more than happy to implement that state.

rakeshcusat · 2019-07-16T03:27:01Z

@robertwb never mind, I found the discussion here

robertwb · 2019-07-22T15:10:14Z

To follow up on this, it's not yet implemented but that would be a welcome contribution (likely just some renaming in this PR).

rakeshcusat · 2019-07-24T05:12:28Z

@robertwb can you confirm that we also need to introduce ReadModifyWriteState in the proto definition?

beam/model/pipeline/src/main/proto/beam_runner_api.proto

Lines 412 to 425 in d510c2b

    
           message StateSpec { 
        
             oneof spec { 
        
               ValueStateSpec value_spec = 1; 
        
               BagStateSpec bag_spec = 2; 
        
               CombiningStateSpec combining_spec = 3; 
        
               MapStateSpec map_spec = 4; 
        
               SetStateSpec set_spec = 5; 
        
             } 
        
           } 
        
           message ValueStateSpec { 
        
             string coder_id = 1; 
        
           }

robertwb · 2019-07-24T13:05:38Z

@robertwb can you confirm that we also need to introduce ReadModifyWriteState in the proto definition?

Or simply rename ValueStateSpec in the proto. (These protos are still in a state they can be modified.)

rakeshcusat · 2019-07-24T15:00:28Z

@robertwb can you confirm that we also need to introduce ReadModifyWriteState in the proto definition?

Or simply rename ValueStateSpec in the proto. (These protos are still in a state they can be modified.)

I just want to make sure I understand it correctly, In case I rename this, I also need to rename in Java SDK as well because it is used there as well, right?
I didn't see any reference in Go SDK other then the auto-generated code so I am assuming it is not yet implemented in Go SDK

I will rename the spec to be consistent and avoid any confusion in future.

Edit:
on second thought I am concerned about this line

beam/model/pipeline/src/main/proto/beam_runner_api.proto

Line 414 in 5796931

ValueStateSpec value_spec = 1;

if I would rename this proto. Wouldn't it cause proto to complain that we have changed the type and cause some incompatibility problem?

pabloem · 2019-07-24T21:12:06Z

@rakeshcusat I think the name for Java would still be the same (changing the name would be a breaking change).

As for the proto, I think you can change it, because it's not externally exposed to users of Beam - it's only meant to be used by Beam itself (in this repository).

robertwb · 2019-08-23T23:07:57Z

Ping?

rakeshcusat · 2019-08-24T19:10:49Z

Ping?

@robertwb I was busy but I will try to finish it this week.

rakeshcusat · 2019-08-26T05:57:30Z

@robertwb / @pabloem Here is the current status:

Updated proto beam_runner_api.proto
Python SDK side is ready.
Made most of the changes in Java SDK and I need feedback to not break the existing ValueState but deprecate it with minimal changes. I would really appreciate some pointers and ideas.
I have not looked into Go SDK and not sure whether it is implemented or not but would appreciate some pointers and ideas.

robertwb

Thanks. Just some minor thoughts. As for Go, the protos are still manually copied and generated there, so I don't think we have to change anything now.

robertwb · 2019-08-26T17:01:49Z

runners/core-java/src/main/java/org/apache/beam/runners/core/InMemoryStateInternals.java

  public static final class InMemoryValue<T>
-      implements ValueState<T>, InMemoryState<InMemoryValue<T>> {
+          implements ValueState<T>, InMemoryState<InMemoryValue<T>> {


Seems like this could be mostly merged with InMemoryReadModifyWrite (possibly requiring a common baseclass).

Actually, I wonder if InMemoryReadModifyWrite was a subclass of InMemoryValue one could get rid of some of the parallel code here (e.g. bindValue vs. bindReadModifyWrite). Haven't pursued this to its conclusion though.

This is a good idea, I need to look more closely and consolidate duplicate code.

robertwb · 2019-08-26T17:06:50Z

...he/beam/runners/flink/translation/wrappers/streaming/state/FlinkBroadcastStateInternals.java

@@ -273,14 +280,73 @@ void clearInternal() {
  }

  private class FlinkBroadcastValueState<T> extends AbstractBroadcastState<T>
-      implements ValueState<T> {
+          implements ValueState<T> {


Similarly, perhaps ReadModifyWriteState could extend ValueState?

rakeshcusat · 2019-09-09T21:53:36Z

R: @kennknowles

rakeshcusat · 2019-09-10T18:22:14Z

@kennknowles and I discuss it offline.
This change is getting bigger and it is also not moving fast. The bigger changes are also hard to review and debug. I am going to break this down into multiple PRs which will be easier to review and we can move faster.

Add ReadModifyWrite state in Python Sdk without proto changes but will use valueState from proto.
Rename the proto and update the Python and Java sdk accordingly.
Add ReadModifyWrite State in Java sdk.

Let me know if you have any concerns with this approach.
@robertwb

robertwb · 2019-09-25T00:08:54Z

Sound good to me. It'd be less work to swap steps (1) and (2).

rakeshcusat · 2019-09-30T03:02:36Z

Sound good to me. It'd be less work to swap steps (1) and (2).

Sounds good. I will do as the suggested way.

robertwb · 2019-11-15T23:41:27Z

Any update on this? I see #9687, were there other PRs?

stale · 2020-01-14T23:55:57Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

stale · 2020-01-22T01:27:23Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

rakeshcusat and others added 3 commits April 9, 2019 15:55

Merge pull request #1 from apache/master

665570d

merging from upstream

Merge remote-tracking branch 'upstream/master'

f70a239

Merge remote-tracking branch 'upstream/master'

6bba28a

robertwb self-requested a review July 15, 2019 12:06

Merge remote-tracking branch 'upstream/master'

a93ed31

rakeshcusat changed the title ~~[BEAM-7739] Implement ValueState in Python SDK~~ [BEAM-7739] Implement ReadModifyWriteState in Python SDK Jul 24, 2019

rakeshcusat added 4 commits July 31, 2019 22:47

Merge remote-tracking branch 'upstream/master'

7773faf

Merge remote-tracking branch 'upstream/master'

2bdf953

[BEAM-7739] Implement ReadModifyWriteState Py sdk

21c9e61

updated_proto

fdfd029

rakeshcusat force-pushed the BEAM-7739-implement-valuestate branch from aaadc02 to fdfd029 Compare August 12, 2019 22:23

update java sdk as well

d74ff56

robertwb reviewed Aug 26, 2019

View reviewed changes

some more changes, need to refactor code

9b726cd

kennknowles requested review from kennknowles and robertwb September 24, 2019 22:03

stale bot added the stale label Jan 14, 2020

stale bot closed this Jan 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-7739] Implement ReadModifyWriteState in Python SDK #9067

[BEAM-7739] Implement ReadModifyWriteState in Python SDK #9067

rakeshcusat commented Jul 14, 2019

robertwb commented Jul 15, 2019

rakeshcusat commented Jul 15, 2019 •

edited

rakeshcusat commented Jul 16, 2019

robertwb commented Jul 22, 2019

rakeshcusat commented Jul 24, 2019 •

edited

robertwb commented Jul 24, 2019

rakeshcusat commented Jul 24, 2019 •

edited

pabloem commented Jul 24, 2019

robertwb commented Aug 23, 2019

rakeshcusat commented Aug 24, 2019

rakeshcusat commented Aug 26, 2019

robertwb left a comment

robertwb Aug 26, 2019

robertwb Aug 26, 2019

rakeshcusat Sep 3, 2019

robertwb Aug 26, 2019

rakeshcusat commented Sep 9, 2019

rakeshcusat commented Sep 10, 2019

robertwb commented Sep 25, 2019

rakeshcusat commented Sep 30, 2019

robertwb commented Nov 15, 2019

stale bot commented Jan 14, 2020

stale bot commented Jan 22, 2020

[BEAM-7739] Implement ReadModifyWriteState in Python SDK #9067

[BEAM-7739] Implement ReadModifyWriteState in Python SDK #9067

Conversation

rakeshcusat commented Jul 14, 2019

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

robertwb commented Jul 15, 2019

rakeshcusat commented Jul 15, 2019 • edited

rakeshcusat commented Jul 16, 2019

robertwb commented Jul 22, 2019

rakeshcusat commented Jul 24, 2019 • edited

robertwb commented Jul 24, 2019

rakeshcusat commented Jul 24, 2019 • edited

pabloem commented Jul 24, 2019

robertwb commented Aug 23, 2019

rakeshcusat commented Aug 24, 2019

rakeshcusat commented Aug 26, 2019

robertwb left a comment

Choose a reason for hiding this comment

robertwb Aug 26, 2019

Choose a reason for hiding this comment

robertwb Aug 26, 2019

Choose a reason for hiding this comment

rakeshcusat Sep 3, 2019

Choose a reason for hiding this comment

robertwb Aug 26, 2019

Choose a reason for hiding this comment

rakeshcusat commented Sep 9, 2019

rakeshcusat commented Sep 10, 2019

robertwb commented Sep 25, 2019

rakeshcusat commented Sep 30, 2019

robertwb commented Nov 15, 2019

stale bot commented Jan 14, 2020

stale bot commented Jan 22, 2020

rakeshcusat commented Jul 15, 2019 •

edited

rakeshcusat commented Jul 24, 2019 •

edited

rakeshcusat commented Jul 24, 2019 •

edited