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

Event carries binlog filename and offset. #217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelschiff
Copy link

@michaelschiff michaelschiff commented May 2, 2018

Simplifies logic for EventListeners that care about checkpointing progress

@michaelschiff michaelschiff force-pushed the feature/offset-on-event branch from ec07b37 to 7f2de13 Compare May 2, 2018 20:11
EventListeners that care about checkpointing progress.
@michaelschiff michaelschiff force-pushed the feature/offset-on-event branch from 7f2de13 to 7b49acb Compare May 2, 2018 20:16
@shyiko
Copy link
Owner

shyiko commented May 20, 2018

I'm sorry, but signatures of public constructors/methods are not allowed to change / offset++ is incorrect.

@michaelschiff
Copy link
Author

Understood. Do you have any thoughts on the intent of the change (exposing the binlog filename and offset on the event itself)? I modified the constructor to keep the new fields effectively final, but I can expose this information without changing the existing public API.

@shyiko
Copy link
Owner

shyiko commented May 21, 2018

To be honest, I'm not sure. So far, I've tried to keep Event|s close to their actual representaion in the binlog (one of the reasons why Event::getHeader() content is not part of the Event itself). I understand why it would be helpful to have both binlogFilename and the already present postion in the Event itself but considering that this piece of information is already available to the user either directly or inderectly I'm more inclined to reevaluate the addition in 1.0.0.

@michaelschiff
Copy link
Author

Fair enough, I'm happy to revisit it then. Accessing this information indirectly through the client certainly works the way things are currently implemented. The only thing that would force this change from nice-to-have to correctness requirement would be if listener notification was made asynchronous. Barring that maybe the more appropriate change would be an extra line in the documentation/README describing how listeners can track position.

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