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

Updated compact subcommend to throw warning instead of error if revisions are already compacted #355

Closed
wants to merge 1 commit into from

Conversation

aaronfern
Copy link
Contributor

What this PR does / why we need it:
Currently the compact subcommand raises a fatal warning if compaction stops for any reason. Updated this behaviour to throw a warning for a special case where compaction does not happen if revisions are already compacted.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Compaction job will now throw warning instead of error if revisions are already compacted

@aaronfern aaronfern requested a review from a team as a code owner July 5, 2021 15:26
@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Jul 5, 2021
@gardener-robot-ci-2
Copy link
Contributor

Thank you @aaronfern for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

Copy link
Collaborator

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @aaronfern! Are we sure if ErrCompacted is the only error that happens when there is nothing to compact?

How about the case where there are no delta snapshots at all?

@aaronfern
Copy link
Contributor Author

Are we sure if ErrCompacted is the only error that happens when there is nothing to compact?

Well, It's the only error I've encountered being thrown up when there is nothing to compact.
Looking at the defined errors, it seems likely to be the only one, but I can't say with certainty that it's the only one that happens.

How about the case where there are no delta snapshots at all?

I did try out this case and it does not throw up an error, so nothing to explicitly handle here

Copy link
Collaborator

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification @aaronfern! /lgtm

Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@aaronfern Nice catch with the error as well as the log message. LGTM

@shreyas-s-rao shreyas-s-rao added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 15, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 15, 2021
@shreyas-s-rao shreyas-s-rao added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 15, 2021
Copy link
Contributor

@abdasgupta abdasgupta left a comment

Choose a reason for hiding this comment

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

LGTM

@shreyas-s-rao shreyas-s-rao added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 15, 2021
@abdasgupta
Copy link
Contributor

Is it rebased and pushed again after merge of test cases fix?

@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 15, 2021
@shreyas-s-rao
Copy link
Collaborator

@abdasgupta Ah yes that's the issue. @aaronfern won't be able to rebase this for the next 10 days. I will raise another PR and cherry-pick his change in that.

@abdasgupta
Copy link
Contributor

@abdasgupta Ah yes that's the issue. @aaronfern won't be able to rebase this for the next 10 days. I will raise another PR and cherry-pick his change in that.

Or Maybe, we can ask him over the phone if he has access to a laptop with git installed.

@shreyas-s-rao
Copy link
Collaborator

Or Maybe, we can ask him over the phone if he has access to a laptop with git installed.

@abdasgupta I would suggest against doing that, since he's on vacation :)

@shreyas-s-rao
Copy link
Collaborator

Closing in favour of #358
/cc @aaronfern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants