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 bug in MemorySnapshot #3133

Closed
wants to merge 5 commits into from
Closed

Fix bug in MemorySnapshot #3133

wants to merge 5 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Feb 10, 2024

Description

Snapshot data is not taken into account during memory store reading

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@shargon shargon added the bug Used to tag confirmed bugs label Feb 10, 2024
@shargon shargon self-assigned this Feb 10, 2024
@shargon
Copy link
Member Author

shargon commented Feb 10, 2024

@Jim8y Any idea about how to fix Seek without lack of performance?

@shargon shargon changed the title Found bug in MemoryStore Found bug in MemorySnapshot Feb 10, 2024
@shargon shargon changed the title Found bug in MemorySnapshot Fix bug in MemorySnapshot Feb 10, 2024
@shargon shargon added blocker Issues that are blocking other issues. Check issues details to see what it is blocking. critical Issues (bugs) that need to be fixed ASAP labels Feb 10, 2024
@shargon shargon marked this pull request as ready for review February 10, 2024 19:05
Comment on lines +68 to 74
if (writeBatch.TryGetValue(key, out var value))
{
return value;
}

immutableData.TryGetValue(key, out value);
return value?[..];
Copy link
Member

Choose a reason for hiding this comment

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

This should return null? If not then change writeBatch to do the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it returns null when it's not found

Copy link
Member

Choose a reason for hiding this comment

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

Do you see the difference? In the returns?

Copy link
Member

Choose a reason for hiding this comment

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

Why are we creating a new pointer?

@shargon
Copy link
Member Author

shargon commented Feb 11, 2024

It seems that LevelDb and RorcksRb snapshot works exactly like that :S, I understand snapshot as a different thing...

@shargon shargon marked this pull request as draft February 11, 2024 10:10
@shargon shargon closed this Feb 11, 2024
@cschuchardt88
Copy link
Member

It seems that LevelDb and RorcksRb snapshot works exactly like that :S, I understand snapshot as a different thing...

They use a bloom filter. Our memory store does not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Issues that are blocking other issues. Check issues details to see what it is blocking. bug Used to tag confirmed bugs critical Issues (bugs) that need to be fixed ASAP waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants