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

Only sync translog when global checkpoint increased #27973

Closed
wants to merge 1 commit into from

Conversation

@dnhatn
Copy link
Contributor

commented Dec 23, 2017

Today we did not set the global checkpoint when opening an engine from an existing store. If we are forced to close an engine before advancing the global checkpoint, we will also close translog which in turn sync a new checkpoint with an unassigned global checkpoint. This is not caught until the global checkpoint assertion was introduced in PR #27837.

This commit tightens the syncNeeded conditions.

Relates #27970

Today we did not set the global checkpoint when opening an engine from
an existing store. If we are forced to close an engine before advancing
the global checkpoint, we also have to close translog which in turn sync
a new checkpoint with an unassigned global checkpoint. This is not
caught until the global checkpoint assertion was introduced in PR
#27837.

This commit tightens the syncNeeded conditions.

Relates #27970
@bleskes

This comment has been minimized.

Copy link
Member

commented Dec 23, 2017

I don't think we should do this. Currently the translog is "just" a dump storage system for the checkpoint and I think we should keep it this way. The assertion that this tries to bypass is flag that something else is wrong, which is what #27972 tries to fix. Having the logic around global checkpoint advancement/ persistence in multiple places will make our life complicated.

@dnhatn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2017

@bleskes, I agree, that's why I labelled this 6.0.3 and 6.1.2 which don't have the right patch. What do you think about these versions?

@dnhatn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

Discussed with @ywelsch, this is not really an issue in previous versions.

@dnhatn dnhatn closed this Dec 28, 2017
@dnhatn dnhatn deleted the dnhatn:only_sync_gcp_increased branch Dec 28, 2017
@dnhatn dnhatn removed v6.0.3 v6.1.2 labels Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.