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

rbd: Add rbd_sparsify_with_progress rbd API #851

Merged
merged 1 commit into from
Apr 4, 2023
Merged

rbd: Add rbd_sparsify_with_progress rbd API #851

merged 1 commit into from
Apr 4, 2023

Conversation

Nikhil-Ladha
Copy link
Contributor

@Nikhil-Ladha Nikhil-Ladha commented Mar 27, 2023 •
edited
Loading

Description:
Added a new rbd API rbd_sparsify_with_progress, and supporting tests.

Fixes: #281

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Added a new file that begins with //go:build ceph_preview
  • Ran make api-update to record new APIs

@Nikhil-Ladha Nikhil-Ladha force-pushed the issue281/sparsify_with_progress_api branch from a13c776 to 1f96cc4 Compare March 27, 2023 11:50
anoopcs9
anoopcs9 previously requested changes Mar 27, 2023
View reviewed changes
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

Every test failure says that Image.SparsifyWithProgress is not tracked. Are you sure that you ran make api-update after new API(and tests) were added? Because api-status.json just says SparsifyWithProgress and not Image.SparsifyWithProgress.

rbd/rbd_sparsify_test.go Show resolved Hide resolved
@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Mar 27, 2023
phlogistonjohn
phlogistonjohn requested changes Mar 27, 2023
View reviewed changes
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

Nitpick: Remove the text **Description:** from the commit message. It might look nice on github when it renders the commit in the PR description but if you are like me and mainly use git on the CLI, it doesn't really add anything to the commit.

rbd/rbd_sparsify.go Show resolved Hide resolved
rbd/rbd_sparsify.go Outdated Show resolved Hide resolved
rbd/rbd_sparsify.go Outdated Show resolved Hide resolved
rbd/rbd_sparsify.go Outdated Show resolved Hide resolved
rbd/rbd_sparsify_test.go Show resolved Hide resolved
rbd/rbd_sparsify_test.go Show resolved Hide resolved
@Nikhil-Ladha Nikhil-Ladha force-pushed the issue281/sparsify_with_progress_api branch from 1f96cc4 to d9b9dfe Compare March 28, 2023 06:54
@Nikhil-Ladha Nikhil-Ladha requested review from anoopcs9 and phlogistonjohn and removed request for anoopcs9 March 28, 2023 06:59
@Nikhil-Ladha Nikhil-Ladha force-pushed the issue281/sparsify_with_progress_api branch from d9b9dfe to 5d9dff6 Compare March 28, 2023 07:02
@Nikhil-Ladha
Copy link
Contributor Author

The failed tests look flaky :/

phlogistonjohn
phlogistonjohn previously approved these changes Mar 28, 2023
View reviewed changes
Copy link
Collaborator

@phlogistonjohn phlogistonjohn 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 for another great contribution!

@phlogistonjohn phlogistonjohn requested a review from anoopcs9 March 28, 2023 14:38
@anoopcs9 anoopcs9 requested a review from ansiwen March 29, 2023 04:49
@mergify
Copy link

mergify bot commented Apr 4, 2023

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@phlogistonjohn phlogistonjohn force-pushed the issue281/sparsify_with_progress_api branch from 5d9dff6 to d810b7d Compare April 4, 2023 12:33
@mergify mergify bot dismissed phlogistonjohn’s stale review April 4, 2023 12:34

Pull request has been modified.

@anoopcs9
Copy link
Collaborator

anoopcs9 commented Apr 4, 2023

@Mergifyio rebase

@Nikhil-Ladha @anoopcs9
rbd: Add rbd_sparsify_with_progress rbd API
275b36f 
Added a new rbd API `rbd_sparsify_with_progress`, and supporting
tests.

Fixes: ceph#281

Signed-off-by: Nikhil-Ladha <[email protected]>
@mergify
Copy link

mergify bot commented Apr 4, 2023

rebase

✅ Branch has been successfully rebased

@anoopcs9 anoopcs9 force-pushed the issue281/sparsify_with_progress_api branch from d810b7d to 275b36f Compare April 4, 2023 13:24
ansiwen
ansiwen approved these changes Apr 4, 2023
View reviewed changes
Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

Docs changes will be handled in a follow-up PR.

@ansiwen ansiwen requested review from anoopcs9 and removed request for anoopcs9 April 4, 2023 15:06
@ansiwen ansiwen dismissed anoopcs9’s stale review April 4, 2023 15:13

It's blocking the merge and the issue apparently has been solved.

@mergify mergify bot merged commit 3d986f2 into ceph:master Apr 4, 2023
phlogistonjohn added a commit to phlogistonjohn/go-ceph that referenced this pull request Apr 4, 2023
@phlogistonjohn
rbd: document SparsifyWithProgress callback's arguments
39e6c65 
As discussed in ceph#851, we wanted to fully document the arguments the
callback function is expected to be called with.

Signed-off-by: John Mulligan <[email protected]>
@phlogistonjohn phlogistonjohn mentioned this pull request Apr 4, 2023
Merged
5 tasks
@phlogistonjohn
Copy link
Collaborator

Docs changes will be handled in a follow-up PR.

See: #861

mergify bot pushed a commit that referenced this pull request Apr 5, 2023
@phlogistonjohn @mergify
rbd: document SparsifyWithProgress callback's arguments
eaf74cb 
As discussed in #851, we wanted to fully document the arguments the
callback function is expected to be called with.

Signed-off-by: John Mulligan <[email protected]>
@phlogistonjohn phlogistonjohn mentioned this pull request Apr 6, 2023
Merged
@Nikhil-Ladha Nikhil-Ladha deleted the issue281/sparsify_with_progress_api branch April 10, 2023 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phlogistonjohn phlogistonjohn phlogistonjohn approved these changes

@ansiwen ansiwen ansiwen approved these changes

@anoopcs9 anoopcs9 Awaiting requested review from anoopcs9

Assignees
No one assigned
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing rbd API components: function rbd_sparsify_with_progress
4 participants
@Nikhil-Ladha @anoopcs9 @phlogistonjohn @ansiwen