-
Notifications
You must be signed in to change notification settings - Fork 1k
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
]: keyPrefix
in DataCache.Find
can be all 0xff.
#3681
Conversation
action.Should().Throw<ArgumentException>(); | ||
action = () => myDataCache.Find(new byte[] { }, SeekDirection.Backward); | ||
action.Should().Throw<ArgumentException>(); | ||
// Action action = () => myDataCache.Find(null, SeekDirection.Backward); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better adjust the logic instead of comment it
b8b9481
to
06cd75c
Compare
Fix
]: keyPrefix
in DataCache.Find
can be null, empty, or all 0xff.Fix
]: keyPrefix
in DataCache.Find
can be all 0xff.
{ | ||
throw new ArgumentException($"{nameof(key_prefix)} with all bytes being 0xff is not supported now"); | ||
// This case is rare | ||
seekPrefix = new byte[ApplicationEngine.MaxStorageKeySize + 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxStorageKeySize.
Maybe not long enough.
throw new ArgumentException($"{nameof(key_prefix)} with all bytes being 0xff is not supported now"); | ||
// This case is rare | ||
seekPrefix = new byte[ApplicationEngine.MaxStorageKeySize + 1]; | ||
Array.Fill(seekPrefix, (byte)0xff); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a logic change @shargon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it doesn't matter.
A better fix #3688 |
Description
DataCache.Find(keyPrefix, SeekDirection.Forward)
start from the first key if keyPrefix is null or empty.But
DataCache.Find(keyPrefix, SeekDirection.Backword)
throws an Exception.It's better to keep a similar logic with different
SeekDirection
.Allow all 0xff
keyPrefix
.This PR depends on #3680
Type of change
Checklist: