-
Notifications
You must be signed in to change notification settings - Fork 242
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
Generate object mappings incrementally #3100
Conversation
66dda15
to
545ce28
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
545ce28
to
307d0a4
Compare
307d0a4
to
b9d86b5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
b9d86b5
to
a12e79c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
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.
This is exactly the approach I was imagining, thanks!
Mostly have nits about internal APIs and suggested improvements to comments. The only thing I don't understand is the change of with_initial_state()
API.
Maybe we could set a 40 minute timeout for the test, instead of the default 6 hours?
I’ve only ever seen them take about 25 minutes.
We use self-hosted runners that are much more powerful than the basic official runners, but our CI is configured to not depend on our runners in any way and to remain fully functional in forks (so contributors can run all the tests, produce custom builds, etc.). Latest run in my fork took ~1h25m, so I think decreasing time limit for tests and clippy to 2h is probably fine, but I wouldn't go as low as 40 minutes.
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’ll give these changes a shot now.
06343da
to
3b7df06
Compare
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 this might be what you want?
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 produce_object_mappings
should process all segments like it did before. This simplifies assumptions removing the need to write comments about it, this also allows us to not return ArchiveBlockOutcome
from constructor, maintaining previous API.
3b7df06
to
355e990
Compare
Ok, this should do it the way you want now. |
355e990
to
9956a53
Compare
9956a53
to
38d9547
Compare
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, thank you!
This PR generates object mapping when each block is added to the archiver, rather than only at the end of a full segment. When a part of a block added in
Archiver::with_initial_state()
, those mappings are generated when the next block is added.This reduces load bursts on the mapping indexer, and reduces object submission to mapping latency.
Piece generation still requires a full segment.
Cleanups
Code contributor checklist: