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

Revisit sync commits #59

Merged
merged 4 commits into from
Jan 10, 2018

Conversation

achille-roussel
Copy link
Contributor

@savaki I modified the CommitMessages implementation in the case where the reader is configured to use synchronous commits, in which case the method now actually waits for the commit to complete and any error is propagated back to the program.

reader.go Outdated
}
}
}

func (r *Reader) commitOffsetStash(conn offsetCommitter) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

given commitOffsetStash is only called from one place and it only performs a single task and is already wrapped in the commit func, is there a reason not to inline it above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I can change that 👍

@savaki
Copy link
Contributor

savaki commented Jan 10, 2018

Looks good to me.

@achille-roussel
Copy link
Contributor Author

Thanks for the review 👍

@achille-roussel achille-roussel merged commit e179f91 into commit-messages-documentation Jan 10, 2018
achille-roussel added a commit that referenced this pull request Jan 10, 2018
* add CommitMessages documentation

* Revisit sync commits (#59)

* revisit sync commits

* drop commit request chan capacity

* remove error prone check

* shorter code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants