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

scorch persister does not introduce new root when only internal values changed #763

Closed
mschoch opened this issue Feb 8, 2018 · 4 comments

Comments

@mschoch
Copy link
Member

mschoch commented Feb 8, 2018

Some previous conversation in #749

The persister will persist the root as it always does, but since no new files were persisted to disk (only changes in the root bolt), the if block check will fail and a new root is not introduced.

Upon further reflection this might be OK. The only reason for us to introduce a new root is to swap in the persisted zap segments, but that doesn't apply in this case. The new root with the new internal values was already made available to readers. So I'm starting to think this is OK.

@mschoch
Copy link
Member Author

mschoch commented Feb 8, 2018

The persister DID persist the snapshot containing the internal values. What it is NOT doing is introducing a new root. And the reason is that it does not have to. The only reason to introduce a new root is if we want to swap in some newly persisted zap segments, but we don't have any of those in this case. The new internal values are already available to readers through the work done by the introducer, and the persister did it's job to persist them in the root bolt with this snapshot. So, for the case of internal only changes, there is nothing more to be done.

@steveyen
Copy link
Contributor

steveyen commented Feb 8, 2018

Makes sense -- and I sense the same theory applies to batches that are doc deletions only.

@mschoch
Copy link
Member Author

mschoch commented Feb 8, 2018

Yes, same for deletions only. Can we improve the comment to not confuse ourselves later? Or should we just close?

@steveyen
Copy link
Contributor

steveyen commented Feb 8, 2018

Yep, good idea -- will put up a comment improvement

steveyen added a commit to steveyen/bleve that referenced this issue Feb 8, 2018
steveyen added a commit to steveyen/bleve that referenced this issue Feb 8, 2018
steveyen added a commit to steveyen/bleve that referenced this issue Feb 8, 2018
@steveyen steveyen closed this as completed Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants