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

CommittingSpec: add logging #1007

Merged
merged 2 commits into from Dec 16, 2019
Merged

CommittingSpec: add logging #1007

merged 2 commits into from Dec 16, 2019

Conversation

ennru
Copy link
Member

@ennru ennru commented Dec 16, 2019

  • Log the ask timeout's message when failing a commit
  • Introduce timings in CommittingSpec to keep it from failing

Intends to solve #957

@ennru ennru added this to the 2.0.0-RC1 milestone Dec 16, 2019
@ennru ennru added this to Incoming Issues and PRs in Akka streaming via automation Dec 16, 2019
@ennru ennru moved this from Incoming Issues and PRs to Ready for review in Akka streaming Dec 16, 2019
Copy link
Member

@seglo seglo left a comment

Choose a reason for hiding this comment

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

LGTM. Will wait for tests before a merge.

Comment on lines +446 to +453
.committableSource(
consumerSettings
.withStopTimeout(100.millis), // this consumer actor needs to stay around to receive the commit
Subscriptions.assignment(new TopicPartition(topic, partition0))
)
.take(10)
// triggers commit timeout as the actor is terminated
.delay(50.millis)
Copy link
Member

@seglo seglo Dec 16, 2019

Choose a reason for hiding this comment

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

It's good that this resolves the issue, but it looks like it could still arise on systems with slow clocks? We should try repeating this test (i.e. sbt "testOnly thistest -- -DtimesToRepeat=100") to see if it fails occasionally.

More long term we should find a way around the stop timeout for happy-path shutdown scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR fixes the test, not the issue (and added more information to the commit timeout error).
I'm not sure there is a way to fix this, for how long after emitting data should a source live to receive commits? Should the actor stay around, but the stage complete?

Akka streaming automation moved this from Ready for review to Reviewer approved Dec 16, 2019
@seglo
Copy link
Member

seglo commented Dec 16, 2019

Unrelated build error

#1009

@seglo seglo merged commit 1bded73 into akka:master Dec 16, 2019
Akka streaming automation moved this from Reviewer approved to Done Dec 16, 2019
@ennru ennru deleted the log-for-957 branch December 17, 2019 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants