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

Initial version of incremental record commitment (take 2) #1201

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

nazar-pc
Copy link
Member

Similar, but not the same as #1161, please read #1161 for background.

In this iteration incremental commitment creation is simpler due to beginning of the segment never changing (see #1200). I also improved comments in the code a bit.

Resolves #1160

Code contributor checklist:

let bytes_before_next_record = ((self.processed_bytes / self.record_size) + 1)
* self.record_size
- self.processed_bytes;
let padding_bytes = bytes_before_next_record % self.record_size;
Copy link
Contributor

Choose a reason for hiding this comment

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

bytes_before_next_record should be < record_size, why is the % op needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

In your version yes, in the code that was there before no.

Copy link
Member Author

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Refactored in a different way, thanks for suggestions

let bytes_before_next_record = ((self.processed_bytes / self.record_size) + 1)
* self.record_size
- self.processed_bytes;
let padding_bytes = bytes_before_next_record % self.record_size;
Copy link
Member Author

Choose a reason for hiding this comment

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

In your version yes, in the code that was there before no.

@nazar-pc nazar-pc requested a review from rahulksnv March 1, 2023 19:39
Copy link
Contributor

@rahulksnv rahulksnv left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@nazar-pc nazar-pc merged commit acdd4f5 into main Mar 2, 2023
@nazar-pc nazar-pc deleted the incremental-archiving-commitment-take2 branch March 2, 2023 08:35
i1i1 added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Archiver API needs to become incremental
2 participants