-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[DRAFT] statement-store: allow controlling of eviction policy #10413
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
base: master
Are you sure you want to change the base?
Conversation
cfef5b0 to
3bdd4d1
Compare
Signed-off-by: Alexandru Gheorghe <[email protected]>
3bdd4d1 to
0890270
Compare
gui1117
left a comment
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 should work good, I wonder if 256 slots is enough. probably it is.
| /// Mask indicating which storage slots in the statement store are preferred for replacement. | ||
| /// When statements with a lower priority are considered for replacement, we prefer to replace | ||
| /// statements whose storage slot is found in this mask. | ||
| pub storage_slots_mask: BitVec<u8, bitvec::order::Lsb0>, |
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.
it will be limited to a maximum capacity of 256 (the number of slots), it sounds reasonable.
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.
Yeah, at least the usecases I saw 256, should be enough.
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'm trying to be frugal with the space, do you think we need more than that ?
| /// The first byte is the storage slot, the remaining 31 bytes are the channel id. | ||
| /// | ||
| /// The storage slot allows categorizing channels into 256 different slots, which can be used | ||
| /// for replacement preference when evicting lower priority statements. |
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.
256 slots seems enough, at least for now. Or should we expect more than 256 statements for a single account?
| /// | ||
| /// It is used to chose which lower priority statements to evict when the account is over | ||
| /// quota. | ||
| replacement_preference_mask: Option<ReplaceSlotPreferenceMask>, |
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 guess we can have this as the field number 9.
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.
Yeah, it shouldn't be a problem.
Description
When a new statement is submitted the
priorityandchannelis used to determine which statements get evicted when the used storage is above the account quota. The two are used like that:Problem
The limitation of this approach is that it does not allow applications for a way to decide which statements they still want to keep in the store. For example if an app submits A, B, C, D, E, F with increasing priorities and they know B and C are not needed anymore, they have a few options:
Gjust one of them by using the same channel, but they will be limited to how bigBandCare. There is a trick around that by submitting an empty statement on B channel(EmptyB) with higher priority, and then submitting G on C's channel. This is really problematic because statement-store protocol does not have ordering guarantees, so on some nodes G could still arrive before EmptyB and trigger eviction of other statements.Credit to @gui1117 for these ideas.
Proposal
Change the API to explicitly allow the App to state their replacement preference, the proposed way to achieve that is like this:
Statementcalledreplacement_preference_mask, which allows App to identify storage slots that can be evicted.The eviction policy becomes like this:
replacement_preference_mask.