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

fixes #1920 #2049

Open
wants to merge 23 commits into
base: integration
Choose a base branch
from
Open

fixes #1920 #2049

wants to merge 23 commits into from

Conversation

ivakegg
Copy link
Collaborator

@ivakegg ivakegg commented Jul 28, 2023

  • Added comment explaining why we do not persist the ivarator set in some situations.

ivakegg added 2 commits July 28, 2023 17:28
* Added comment explaining why we do not persist the ivarator set in some situations.
Comment on lines +817 to +819
// Note that we will not persist the data if we are not over the scan threshold and the data fits into
// one buffer (no persisted data). That means that if we need to rebuild the sorted set that we can do
// it relatively quickly so no need for a persisted set with a complete marker.
Copy link
Collaborator

@jwomeara jwomeara Aug 10, 2023

Choose a reason for hiding this comment

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

I'm not sure what this comment has to do with the original issue I reported.

In the scenario I described, one of two things happen.

Either,

  1. We have entries buffered in memory, and that triggers the ivarator to persist the entries to disk and create a complete file.

OR

  1. We have no entries buffered in memory (because everything was ALREADY persisted to disk), and that does NOT trigger the creation of a complete file.

I don't think this has anything to do with the scan threshold. If it did, then in scenario #1 the code wouldn't persist the buffer to disk, (but it absolutely does!!!). Scenario #1 is not the problem.

Scenario #2 where data has already been written to disk, but there are no results in the in-memory buffer is the problem. We already have data written to disk, but we aren't creating the complete file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am pretty sure that "this.set.hasPersistedData()" is the part of the check that takes care of your scenario 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jwomeara are you saying that the code in scenario 2 is not making an accurate determination about the sate of persistence? That it's not enough that the buffer is null to call it "persisted". Should the check be buffer == null && isPersisted instead of an || ? Or am I misunderstanding altogether?

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.

4 participants