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

Add snapshot time #10854

Merged
merged 16 commits into from
Jan 13, 2022
Merged

Add snapshot time #10854

merged 16 commits into from
Jan 13, 2022

Conversation

viko600
Copy link
Contributor

@viko600 viko600 commented Dec 11, 2021

Added snapshot creation time when the yb-admin list snapshot command is issuedalaigned

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2021

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Dec 11, 2021

✔️ Deploy Preview for infallible-bardeen-164bc9 ready!

🔨 Explore the source changes: 58cafd4

🔍 Inspect the deploy log: https://app.netlify.com/sites/infallible-bardeen-164bc9/deploys/61d0c585cf4f1200079b7063

😎 Browse the preview: https://deploy-preview-10854--infallible-bardeen-164bc9.netlify.app

@sanketkedia sanketkedia self-requested a review December 13, 2021 10:49
Copy link
Contributor

@sanketkedia sanketkedia left a comment

Choose a reason for hiding this comment

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

Hey Viktor, Great work on these changes. They look good. Left one minor comment. Also, it'd be great if you can attach a screenshot of how the list_snapshots command looks like both without and with your change since this is user-facing. Also, if this is the first time you are contributing then it will be great to add your name to contributors file

Thank you!

ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
@viko600
Copy link
Contributor Author

viko600 commented Dec 17, 2021

Hi @sanketkedia and @OlegLoginov I have fixes the suggested errors and have made a screenshot with the 2 outputs of the list_snapshots command. For the first output I used my branch and for the second, I used the master branch.

changes vs current

Copy link
Contributor

@sanketkedia sanketkedia left a comment

Choose a reason for hiding this comment

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

Hey @viko600, Some comments on code style, and then we are good to go.

ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
ent/src/yb/tools/yb-admin_client_ent.cc Outdated Show resolved Hide resolved
@viko600 viko600 requested a review from sanketkedia December 21, 2021 09:05
@viko600
Copy link
Contributor Author

viko600 commented Dec 21, 2021

I want to take another issue after this one is resolved. Should I only look for issues with the help wanted label, or an I free to chose from all the issues?

Copy link
Contributor

@OlegLoginov OlegLoginov left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@sanketkedia
Copy link
Contributor

sanketkedia commented Jan 4, 2022

@viko600 Backup tests are failing due to this change because they expect a strict format. I'll commit the change to fix them, run tests and then we'll probably be good to go.

@sanketkedia
Copy link
Contributor

Screen Shot 2022-01-13 at 3 55 23 AM

Adding screenshot of the behavior when the snapshot is in "CREATING" or "DELETING" state.

Copy link
Contributor

@sanketkedia sanketkedia left a comment

Choose a reason for hiding this comment

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

@viko600 Tests look good, so accepting this PR. Also, cases like the behavior when the snapshot is in "CREATING" or "DELETING" state also work. I will merge it.

@sanketkedia sanketkedia merged commit 73e2c66 into yugabyte:master Jan 13, 2022
@sanketkedia
Copy link
Contributor

I want to take another issue after this one is resolved. Should I only look for issues with the help wanted label, or an I free to chose from all the issues?

@viko600 We don't necessarily restrict from our end but the help wanted label is guidance on issues with less complicated fixes that are great for the OSS community. Also, do check with the issue creator/other people in the thread to see if somebody is already working on it. It could be that internally someone has already started working on it.

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.

4 participants