[BEAM7473] [BEAM8478] Update SDF related APIs by boyuanzz · Pull Request #9794 · apache/beam

boyuanzz · 2019-10-14T23:28:15Z

API changes included:

Create WatermarkEstimator for tracking watermark.
Use try_split(0) as checkpoint and delete checkpoint API.
Create ThreadsafeRestrictionTracker which wraps a RestrictionTracker to provide synchronization.
Create RestrictionTrackerView to limit API expose to a DoFn.

OffsetRestrictionTracker changes:

Clean up lock logic
Clean up checkpoint()

Proto changes:

Add a new field into DelayedBundleApplication: google.protobuf.Duration requested_time_delay

R: @robertwb
cc: @lukecwik

boyuanzz · 2019-10-28T17:55:39Z

Run Python Precommit

asf-ci · 2019-10-28T19:38:06Z

SUCCESS

--none--

boyuanzz · 2019-10-28T20:30:46Z

Kindly pinged : )

robertwb

Thanks. I've left some comments. The least fleshed out seems WatermarkEstimator, but I don't think we have to resolve that before getting the other stuff in.

robertwb · 2019-10-29T19:21:58Z

While I think it makes sense to allow both in the public API, I generally try to avoid such redundancy in the internal implementation. However, it may be OK in this case given neither is clearly better. Any thoughts @lukecwik ?

I created a JIRA issue as follow up: BEAM-8536. Maybe it would be better to separate this PR from java changes?

robertwb · 2019-10-29T19:22:56Z

I wouldn't expose ThreadsafeRestrictionTracker and RestrictionTrackerView as public--users shouldn't be creating these. (One could argue perhaps the latter, for documentation purposes.)

robertwb · 2019-10-29T19:26:44Z

I would put this note down below "Keeps track of the restrictions claimed part for a Splittable DoFn." as it's not the most important thing about this class.

Also, to be completely clear, I might phrase it as

The restriction may be modified by different threads, however the system will ensure sufficient locking such that no methods on the restriction tracker will be called concurrently.

robertwb · 2019-10-29T19:28:02Z

The runner can...

robertwb · 2019-10-29T19:47:40Z

Hmm... Looking at this in context it seems out-of-place. I would remove this from RestrctionTracker and suggest the implementation(s) be placed in RestrictionProvider instead.

Removed default_size from RestrictionTracker in commit: 786fb3c. There is another API called restriction_size() in RestrictionProvider, which is duplicated with default_size.

robertwb · 2019-10-29T19:53:25Z

I wonder if we should require the user to implement this at all. Perhaps it should just be part of the RestrictionTrackerView, whose implementation is try_split(0), marking self as done, and populating that which needs to be returned for deferred_status.

robertwb · 2019-10-29T19:58:44Z

The fact that we have to do this seems like a flaw in our API... Can we not get this information out of current_restriction()?

Not resolved?

Ah sorry, I actually did the replacement in commit a1fc6fc but I forgot to delete original one. Deleted here: e89287c

robertwb · 2019-10-29T20:10:35Z

Extra space after """

robertwb · 2019-10-29T20:10:47Z

Should this be set_watermark?

robertwb · 2019-10-29T20:13:51Z

I think we should flesh out what the interface is for a general WatermarkEstimator, vs. this particular ManualWatermarkEstimator with this particular implementation. This may inform whether we want a WatermarkEstimatorProvider or an actual WatermarkEstimator instance (which we repeatedly call reset() on.) A TODO would be fine.

Created BEAM-8537. I'll follow up with this one after merging this PR.

boyuanzz · 2019-11-01T18:05:09Z

All above comments have been addressed. I'll merge conflicts once changes look fine.
Please take another look @robertwb Thanks!

boyuanzz · 2019-11-06T18:25:34Z

Run Portable_Python PreCommit

robertwb

Just some more references to default_size to remove, then it looks good.

robertwb · 2019-11-08T01:08:11Z

Remove this test?

robertwb · 2019-11-08T01:11:43Z

We should remove its use as well.

robertwb · 2019-11-08T01:17:02Z

Move up to the restriction provider(s).

boyuanzz · 2019-11-08T18:04:40Z

Run Python PreCommit

boyuanzz · 2019-11-08T18:16:00Z

Thanks for your help, Robert! I have cleaned up all default_size in commit 712d95e. @robertwb

boyuanzz · 2019-11-11T20:56:22Z

Kindly pinged : D

robertwb

Looks like there's some merge conflicts, but other than that looks good to me. Thanks.

boyuanzz · 2019-11-12T23:00:17Z

Run Python PreCommit

boyuanzz · 2019-11-12T23:10:52Z

Run Java PreCommit

boyuanzz · 2019-11-13T01:06:18Z

Run Python PreCommit

boyuanzz · 2019-11-13T04:24:04Z

Run Python PreCommit

pabloem · 2019-11-13T17:23:46Z

fyi @rohdesamuel

boyuanzz changed the title ~~Watermark~~ [WIP] Update SDF watermark API with WatermarkEstimator & WatwrmarkEstimatorParam Oct 14, 2019

boyuanzz force-pushed the watermark branch 6 times, most recently from d856017 to a34b71a Compare October 24, 2019 22:41

boyuanzz changed the title ~~[WIP] Update SDF watermark API with WatermarkEstimator & WatwrmarkEstimatorParam~~ [7473] [8478] Update SDF related APIs Oct 24, 2019

boyuanzz force-pushed the watermark branch from a34b71a to bee88de Compare October 24, 2019 22:56

boyuanzz requested a review from robertwb October 24, 2019 22:56

boyuanzz changed the title ~~[7473] [8478] Update SDF related APIs~~ BEAM[7473] BEAM[8478] Update SDF related APIs Oct 24, 2019

boyuanzz force-pushed the watermark branch from bee88de to 930704d Compare October 24, 2019 23:16

boyuanzz changed the title ~~BEAM[7473] BEAM[8478] Update SDF related APIs~~ [BEAM7473] [BEAM8478] Update SDF related APIs Oct 25, 2019

robertwb reviewed Oct 29, 2019

View reviewed changes

robertwb reviewed Nov 8, 2019

View reviewed changes

robertwb approved these changes Nov 12, 2019

View reviewed changes

boyuanzz force-pushed the watermark branch from 004f945 to fb86a58 Compare November 12, 2019 19:53

Update SDF APIs

b94dca2

boyuanzz force-pushed the watermark branch from fb86a58 to b94dca2 Compare November 12, 2019 20:40

boyuanzz merged commit 352ecf3 into apache:master Nov 13, 2019

boyuanzz deleted the watermark branch November 13, 2019 18:15

lukecwik mentioned this pull request Feb 27, 2020

[BEAM-2939] Provide user facing API for reporting the watermark in SplittableDoFns. #10992

Merged

4 tasks

Conversation

boyuanzz commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boyuanzz commented Oct 28, 2019

Uh oh!

asf-ci commented Oct 28, 2019

Uh oh!

boyuanzz commented Oct 28, 2019

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boyuanzz commented Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boyuanzz commented Nov 6, 2019

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boyuanzz commented Nov 8, 2019

Uh oh!

boyuanzz commented Nov 8, 2019

Uh oh!

boyuanzz commented Nov 11, 2019

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

boyuanzz commented Nov 12, 2019

Uh oh!

boyuanzz commented Nov 12, 2019

Uh oh!

boyuanzz commented Nov 13, 2019

Uh oh!

boyuanzz commented Nov 13, 2019

Uh oh!

pabloem commented Nov 13, 2019

Uh oh!

Reviewers

Uh oh!

boyuanzz commented Oct 14, 2019 •

edited

Loading

boyuanzz commented Nov 1, 2019 •

edited

Loading