-
Notifications
You must be signed in to change notification settings - Fork 413
feat: Persist RequestList state #1274
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
Conversation
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.
Looks good
next_index: Annotated[int, Field(alias='nextIndex')] = 0 | ||
next_unique_key: Annotated[str | None, Field(alias='nextUniqueKey')] = None | ||
in_progress: Annotated[set[str], Field(alias='inProgress')] = set() |
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.
Are the camelCase
aliases necessary? AFAIK I also did not use them in FS storage clients.
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.
Probably not. Sessions and Statistics (other instances of recoverable state) use them too. I have no strong opinion here, if you do, say the word and I'll remove them.
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.
Well, so currently somewhere we use them, and somewhere we don't - up to you then.
The request data snapshotting is pretty inefficient - it loads the whole thing into memory (same as the JS version) and stores it uncompressed in the key-value store (JS version uses gzip). After quite some trial and error, I believe we can use Ostrich algorithm now and optimize if it proves necessary. |
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.
Some small comments and one more serious about the test.
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.
LGTM
RequestList
#99