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

Fix restart update indexes #350

Closed
wants to merge 1 commit into from
Closed

Conversation

arj03
Copy link
Member

@arj03 arj03 commented Jun 8, 2022

While debugging #349 I noticed that restartUpdateIndexes was behaving a bit unexpectedly. Compaction added a call to restartUpdateIndexes on startup even if there was nothing to do. This means that if you have an empty database both the normal updateIndexes call, but also the updateIndexes call from restartUpdateIndexes would be queued up for when indexesStateLoaded is ready. This means that you will get 2 calls right after another one calling the other. What this change does it to only call updateIndexes if abortLogStreamForIndexes is set. Meaning that updateIndexes are already running and it will be stopped because we abort those log streams.

@staltz
Copy link
Member

staltz commented Jun 9, 2022

I see the problem! Thanks for spotting it.

I'm confused about this whole thing, though. I probably was lucid when I wrote that code, but it doesn't seem now. If I remove the whole restartUpdateIndexes(), all tests still pass (especially compaction.js and compaction-resume.js).

Let me take some time to revise the post-compaction logic.

@arj03
Copy link
Member Author

arj03 commented Jun 9, 2022

Yes, was not 100% sure this is the correct fix, but anyway all tests are still passing. And there is def. something there so take your take to go through the logic :)

@staltz
Copy link
Member

staltz commented Jun 9, 2022

@arj03 I made this commit on the formats-split branch cdb000d, I'm afraid of merge conflicts with master, so is it fine that we close #350 and use that commit instead?

@arj03
Copy link
Member Author

arj03 commented Jun 9, 2022

Sure

@staltz staltz closed this Jun 13, 2022
@staltz staltz deleted the fix-restart-update-indexes branch June 13, 2022 07:23
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