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.

Sign up for GitHub

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

Jump to bottom

Restore: rework progress aggregation #4225

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Restore: rework progress aggregation #4225

wants to merge 7 commits into from

Conversation

Michal-Leszczynski
Copy link
Collaborator

This PR does the following:

Note, the RestoredBytes and RestoreDuration need to be added to the sctool progress display as a part of a separate PR.

@Michal-Leszczynski
refactor(schema): remove unused restore_run_progress skipped column
bea4928 
This column might be interesting for backup which performs
deduplication, but it does not make any sense for restore.
It wasn't even a part of restore progress API, so it's safe
to remove it.
@Michal-Leszczynski
feat(schema): add restore_run_progress restored column
925b932 
This column wasn't needed with the sync load&stream Scylla API
(restored is equal to either 0 or downloaded), but it is cleaner
to have it and it can be useful for native restore Scylla API.
@Michal-Leszczynski
feat(restore): adjust Rclone and l&s restore progress update
04e1237 
Followup to the e112747.
In the past we treated RestoredStartedAt/RestoreCompletedAt
as the time frame for l&s, but we should look at it more
inclusively, so as the time frame of restoring bytes,
which for Rclone API + l&s approach starts with the download.
@Michal-Leszczynski
refactor(restore): separate progress aggregation from DB query
fe7604e 
Previously, the code aggregating restore progress and the code
querying the run progress for aggregation were tightly tangled.
This resulted in difficulties in writing unit tests and a more
confusing code overall.
This commit changes progress aggregation to use iterator over
run progresses, which can be easily mocked in unit tests.
@Michal-Leszczynski
feat(restore): fill HostProgress RestoredBytes/Duration
284fb80 
Followup to the e112747.
@Michal-Leszczynski
feat(restore): allow for specifying now in progress aggregation
5d19e3f 
It is important to specify now when testing.
@Michal-Leszczynski
feat(restore): add unit tests for progress aggregation
b4cb53d 
Thanks to the previous commit, it is now possible
to add unit tests for progress aggregation!
@Michal-Leszczynski Michal-Leszczynski marked this pull request as ready for review January 24, 2025 09:59
VAveryanov8
VAveryanov8 approved these changes Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@VAveryanov8 VAveryanov8 left a comment

Choose a reason for hiding this comment

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

Nice usage of iterators and tests!

@Michal-Leszczynski
Copy link
Collaborator Author

Thanks @VAveryanov8!
I'm just wondering, which approach to iterator that can encounter an error do you prefer?

  1. The one from this PR - iterator stores the error internally and it needs to be checked manually after iteration (similar to gocql iterator). It allows to hide the error handling from other parts of the code (might be good or bad).
  2. Instead of iter.Seq[K], use iter.Seq2[K, error] which allows for handling errors by the underlying code.

I think that the second is probably better in general, but the first approach seamed more tailored to this PR use case.

@VAveryanov8
Copy link
Collaborator

The one from this PR - iterator stores the error internally and it needs to be checked manually after iteration (similar to gocql iterator). It allows to hide the error handling from other parts of the code (might be good or bad).
Instead of iter.Seq[K], use iter.Seq2[K, error] which allows for handling errors by the underlying code.
I think that the second is probably better in general, but the first approach seamed more tailored to this PR use case.

Good question! I don't have a strong opinion on this - iterators are quite new to the language and there are not that many projects that already using them.
In general second approach is more flexible (err can be skipped and etc), but in this particular case I think what you did looks better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VAveryanov8 VAveryanov8 VAveryanov8 approved these changes

@karol-kokoszka karol-kokoszka Awaiting requested review from karol-kokoszka karol-kokoszka is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Michal-Leszczynski @VAveryanov8