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

refactor(cli): remove unpaid/paid distinction from chunk manager #1022

Merged

Conversation

RolandSherwin
Copy link
Member

@RolandSherwin RolandSherwin commented Nov 29, 2023

Closes #997

Description

Summary generated by Reviewpad on 29 Nov 23 18:13 UTC

This pull request includes changes related to the upload of file chunks.

  • The condition in line 166 has been changed to reflect the use of a new method that checks if there are any chunks to upload.
  • The variables unpaid_chunks_to_upload and unpaid_chunks_to_upload_len have been renamed to chunks_to_upload and chunks_to_upload_len, respectively.
  • The progress bar now shows the number of chunks_to_upload_len instead of unpaid_chunks_to_upload_len.
  • The methods mark_paid and mark_verified have been replaced with mark_completed.
  • The variables failed_payments, failed_uploads, failed_payments_len, and total_failures have been removed.
  • The error message in line 265 has changed to state the number of failed uploads.
  • The log messages in lines 272, 276, 287, 291, 300, and 302 have been updated to use {chunks_to_upload_len}.

These changes improve the upload process and provide better logging information.

@reviewpad reviewpad bot requested a review from jacderida November 29, 2023 18:13
@reviewpad reviewpad bot added Large Large sized PR waiting-for-review labels Nov 29, 2023
}
}
}
progress_bar.finish_and_clear();

// report errors
let failed_payments = chunk_manager.get_unpaid_chunks();
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this would not have worked prior to the changes too as we call mark_paid and mark_verfied right after each other.
I don't think we have any way to just know if the payment alone failed?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed! Although we might want to keep printing out pay errors.

Copy link
Member

Choose a reason for hiding this comment

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

By checking recorded_pay_errors.len() instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see! Updated the PR.

@RolandSherwin RolandSherwin force-pushed the chunk_manager_remove_verification branch from 675dd86 to a9fac3d Compare November 30, 2023 07:55
Copy link
Member

@grumbach grumbach left a comment

Choose a reason for hiding this comment

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

LGTM

@RolandSherwin RolandSherwin added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@RolandSherwin RolandSherwin added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@RolandSherwin RolandSherwin added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@grumbach grumbach added this pull request to the merge queue Dec 1, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 1, 2023
@RolandSherwin RolandSherwin added this pull request to the merge queue Dec 5, 2023
@RolandSherwin RolandSherwin force-pushed the chunk_manager_remove_verification branch from a9fac3d to cd7c535 Compare December 5, 2023 10:23
Merged via the queue into maidsafe:main with commit 26ac756 Dec 5, 2023
28 checks passed
@RolandSherwin RolandSherwin deleted the chunk_manager_remove_verification branch December 5, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Large Large sized PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Since we verify during upload, ChunkManager mark_paid and mark_verified could be merged into one
2 participants