Skip to content
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

[proxima-direct-core] #156 Added support for configurable watermark estimator and idle policy for commit log readers #366

Merged
merged 1 commit into from
Jun 5, 2020
Merged

[proxima-direct-core] #156 Added support for configurable watermark estimator and idle policy for commit log readers #366

merged 1 commit into from
Jun 5, 2020

Conversation

tisonet
Copy link

@tisonet tisonet commented May 25, 2020

No description provided.

@tisonet tisonet force-pushed the unify-watermark branch 5 times, most recently from 7202576 to dad628a Compare June 1, 2020 09:13
@dmvk dmvk mentioned this pull request Jun 1, 2020
Closed
@tisonet tisonet force-pushed the unify-watermark branch 2 times, most recently from 9e19949 to 27cf01a Compare June 3, 2020 06:04
@tisonet tisonet changed the title WIP: Watermark unified Added support for configurable watermark estimator and idle policy for commit log readers Jun 3, 2020
@tisonet tisonet force-pushed the unify-watermark branch 2 times, most recently from eb8930c to 207ffd4 Compare June 3, 2020 08:58
@je-ik
Copy link

je-ik commented Jun 3, 2020

@LesTR sonar is still not working for non datadriven forks. Did we do anything about it?
@tisonet I opened #373 so that we can let sonar verify the PR.

@je-ik
Copy link

je-ik commented Jun 3, 2020

Hm, the check started to work. I'll close the duplicate PR #373.

@je-ik je-ik mentioned this pull request Jun 3, 2020
Closed
je-ik
je-ik requested changes Jun 3, 2020
View reviewed changes
Copy link

@je-ik je-ik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very cool job 👍
I have a few comments, most important is to remove the PartitionedWatermarkEstimator.empty(). And there is probably possible some cleaning in KafkaLogReader - e.g. do we still need class VectorClock, or can it be completely removed?

core/src/main/java/cz/o2/proxima/time/AbstractWatermarkEstimator.java Outdated Show resolved Hide resolved
core/src/main/java/cz/o2/proxima/time/WatermarkEstimator.java Outdated Show resolved Hide resolved
core/src/main/java/cz/o2/proxima/time/WatermarkEstimator.java Outdated Show resolved Hide resolved
direct/core/src/main/java/cz/o2/proxima/direct/time/PartitionedWatermarkEstimator.java Outdated Show resolved Hide resolved
direct/core/src/main/java/cz/o2/proxima/direct/time/PartitionedWatermarkEstimator.java Outdated Show resolved Hide resolved
direct/io-kafka/src/main/java/cz/o2/proxima/direct/kafka/KafkaLogReader.java Outdated Show resolved Hide resolved
direct/io-kafka/src/main/java/cz/o2/proxima/direct/kafka/KafkaLogReader.java Show resolved Hide resolved
direct/io-kafka/src/test/java/cz/o2/proxima/direct/kafka/LocalKafkaCommitLogDescriptorTest.java Outdated Show resolved Hide resolved
direct/io-pubsub/src/main/java/cz/o2/proxima/direct/pubsub/PubSubWatermarkConfiguration.java Outdated Show resolved Hide resolved
direct/io-pubsub/src/main/java/cz/o2/proxima/direct/pubsub/PubSubReader.java Show resolved Hide resolved
@dmvk
Copy link

dmvk commented Jun 4, 2020

Good job 👍 I'll leave the review to @je-ik as he's more familiar with the internals. The overall code and approch looks good to me. Nice work 🎉

@tisonet
Copy link
Author

tisonet commented Jun 4, 2020

I wanted remove VectorClock class, but I am afraid that somebody can still use it. You are the boss so I will remove it at all.

@je-ik
Copy link

je-ik commented Jun 4, 2020

Generally, classes annotated with @Internal are not supposed to be used outside of proxima internal classes, so we can remove them at will.

@tisonet tisonet force-pushed the unify-watermark branch 7 times, most recently from 7be308c to 7a9158c Compare June 5, 2020 07:48
je-ik
je-ik requested changes Jun 5, 2020
View reviewed changes
Copy link

@je-ik je-ik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!
A few last things:
a) please fill issue to O2-Czech-Republic, as this is a larger change
b) please squash the commits and add issue ID to commit line (e.g. [proxima-direct-core] #156 ....)
c) will merge it as soon as all tests pass

@tisonet tisonet changed the title Added support for configurable watermark estimator and idle policy for commit log readers [proxima-direct-core] #156 Added support for configurable watermark estimator and idle policy for commit log readers Jun 5, 2020
@je-ik
Copy link

je-ik commented Jun 5, 2020

This closes O2-Czech-Republic#156

je-ik
je-ik approved these changes Jun 5, 2020
View reviewed changes
Copy link

@je-ik je-ik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks! 👍

@je-ik
Copy link

je-ik commented Jun 5, 2020

Uf, strange, the build fails on the same test, although other PRs seems not to have this issue. There should be no relation, though.

@je-ik je-ik closed this Jun 5, 2020
@je-ik je-ik reopened this Jun 5, 2020
@je-ik je-ik merged commit e14db8b into datadrivencz:master Jun 5, 2020
@je-ik je-ik mentioned this pull request Jun 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmvk dmvk dmvk left review comments

@je-ik je-ik je-ik approved these changes

Assignees
No one assigned
Labels
beam beam-core core direct direct-core direct-io
Projects
None yet
Milestone
No milestone
3 participants
@tisonet @je-ik @dmvk