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.

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

Committer sink #622

Merged
merged 4 commits into from Nov 1, 2018

Conversation

Projects
None yet
3 participants
@rtimush
Copy link
Contributor

commented Oct 30, 2018

Design considerations:

  • committer is a Sink as in my experience it is usually the last step of the flow
  • committer settings max-batch and max-interval define the amount of work we can afford to lose, in terms of messages or time

Implements #619

@ennru
Copy link
Member

left a comment

Looks good, just a few suggestions.
I think you could add a second sink to commit one-by-one. Committer.oneByOne or something alike.

@rtimush rtimush force-pushed the rtimush:committer-flow branch from f046dc2 to 27e9219 Oct 31, 2018

@rtimush

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

Thanks for the review! I'm not sure about Committer.oneByOne — while technically it's trivial, it's also a subcase of the generic sink with max-batch = 1. If we add oneByOne it might make an impression that this case is somehow special, or that it gives some additional guarantees. Also, in the real environment committing one by one will very likely become a bottleneck, so one will have to switch to the batching alternative anyway, but it will be a code change rather than a configuration change.

@rtimush rtimush force-pushed the rtimush:committer-flow branch from 27e9219 to 480c595 Oct 31, 2018

@ennru
Copy link
Member

left a comment

I guess you are right. I'd like this to be used for all committing going forward.

rtimush added some commits Oct 29, 2018

@rtimush rtimush force-pushed the rtimush:committer-flow branch from 480c595 to 8f75d84 Oct 31, 2018

@rtimush rtimush changed the title WIP - Committer sink Committer sink Oct 31, 2018

@ennru

ennru approved these changes Nov 1, 2018

Copy link
Member

left a comment

LGTM.

@ennru ennru merged commit f87f19d into akka:master Nov 1, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@ennru ennru added this to the 1.0-M1 milestone Nov 1, 2018

@ennru

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Thank you for the super fast turn-around.

max-batch = 1000

# Maximum interval between commits
max-interval = 1m

This comment has been minimized.

Copy link
@patriknw

patriknw Nov 5, 2018

Member

That's a rather long duration as default, which increases the risk of that a crash happens with uncommitted messages - resulting in duplicates at next startup. 10s should introduce too much overhead.

This comment has been minimized.

Copy link
@rtimush

rtimush Nov 5, 2018

Author Contributor

I really had a hard time defining the defaults here. For kafka-streams the default commit frequency is every 30s. In our production setup with alpakka-kafka we also couldn't get much below that number.

This comment has been minimized.

Copy link
@ennru

ennru Nov 6, 2018

Member

I changed it to 1000/10 seconds as the default. I think of the defaults to be most important for people starting to use the library -- when they master it, they will understand their needs better and change to suitable values.

@rtimush rtimush deleted the rtimush:committer-flow branch Nov 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.