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

bugfix: get time string from current time instead of pod name #1229

Merged
merged 1 commit into from
Nov 26, 2019

Conversation

weekface
Copy link
Contributor

What problem does this PR solve?

fixes #1227

What is changed and how does it work?

Check List

Tests

Code changes

  • Has Helm charts change

Side effects

Related changes

  • Need to cherry-pick to the release branch

Does this PR introduce a user-facing change?:

get time string from current time instead of pod name

timestamp=$(echo ${POD_NAME}|awk -F- '{print $(NF-1)}')
## use UTC time zone to resolve timestamp, avoiding different parsing results due to different default time zones
backupName=scheduled-backup-`date -u -d @${timestamp} "+%Y%m%d-%H%M%S"`
backupName=scheduled-backup-`date "+%Y%m%d-%H%M%S"`
Copy link
Contributor Author

@weekface weekface Nov 25, 2019

Choose a reason for hiding this comment

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

Use TZ env timezone information of the pod instead of UTC. @onlymellb

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

timestamp=$(echo ${POD_NAME}|awk -F- '{print $(NF-1)}')
## use UTC time zone to resolve timestamp, avoiding different parsing results due to different default time zones
backupName=scheduled-backup-`date -u -d @${timestamp} "+%Y%m%d-%H%M%S"`
backupName=scheduled-backup-`date "+%Y%m%d-%H%M%S"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend continuing to use UTC timestamps date -u "+%Y%m%d-%H%M%S"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be strange if the user set TZ already.

@weekface
Copy link
Contributor Author

/run-e2e-tests

Copy link
Contributor

@onlymellb onlymellb left a comment

Choose a reason for hiding this comment

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

LGTM

@tennix
Copy link
Member

tennix commented Nov 25, 2019

Does this impact the restore from the scheduled backup?

@weekface
Copy link
Contributor Author

weekface commented Nov 26, 2019

The generated name is the same as the old way: "+%Y%m%d-%H%M%S", for example: scheduled-backup-20190822-041004. The user use scheduledBackupName to restore. So it should not impact restore behavior.

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@weekface weekface merged commit ae1264a into pingcap:master Nov 26, 2019
@weekface weekface deleted the fix-scheduled-backup branch November 26, 2019 06:24
weekface added a commit to weekface/tidb-operator that referenced this pull request Nov 26, 2019
yahonda pushed a commit that referenced this pull request Dec 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scheduled-backup failed to start when clusterName is too long
3 participants