-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
bigtable-backup: Tool for creating and managing periodic tables stored in Bigtable #729
bigtable-backup: Tool for creating and managing periodic tables stored in Bigtable #729
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to me but I don't have enough knowledge to approve. I think @gouthamve should have a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I think this is super brittle. We need some checks and monitoring here. When will this exit unsuccessfully?
|
||
while True: | ||
line = popen.stdout.readline() | ||
if i == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be a bool. But this seems hacky, can we add a json output to the tool and we parse json?
bigtable-backup .... -ojson
? And we parse the json here.
|
||
backup_active_periodic_table_parser = subparser.add_parser('ensure-backups', | ||
help="Ensure backups of right tables exist") | ||
backup_active_periodic_table_parser.add_argument('--period-from', required=True, type=valid_date) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have a time-range, like 90 days? --period-for
? And make sure only one is set.
if last_timestamp_from_table_number > timestamps[-1]: | ||
# Checking whether backup is created after last timestamp of tables period. | ||
create_backup(table_id, args) | ||
elif len(timestamps) > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need of an elif
here. We need both checks always.
@gouthamve Thanks for the review! All the requested changes are done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits, other than that good to go!
registry = CollectorRegistry() | ||
bigtable_backup_job_last_run_unixtime = Gauge('bigtable_backup_job_last_run_unixtime', 'Last time a bigtable backup job ran at', registry=registry) | ||
bigtable_backup_job_last_success_unixtime = Gauge('bigtable_backup_job_last_success_unixtime', 'Last time a bigtable backup job successfully finished', registry=registry) | ||
bigtable_backup_job_runtime_seconds = Gauge('bigtable_backup_job_runtime_seconds', 'Runtime of last successfully finished bigtable backup job', registry=registry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add 2 more metrics: backups_created
and backups_existing
?
from prometheus_client import CollectorRegistry, Gauge, push_to_gateway | ||
|
||
registry = CollectorRegistry() | ||
bigtable_backup_job_last_run_unixtime = Gauge('bigtable_backup_job_last_run_unixtime', 'Last time a bigtable backup job ran at', registry=registry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be _seconds
and not _unixtime
.
It has 2 commands: 1. backup-active-periodic-table: For backing up active periodic table 2. ensure-backups: It helps ensure right tables are backed up and retain only single backup for non active periodic tables
d1b5d2c
to
8e8893a
Compare
Relies on this tool: https://github.com/grafana/bigtable-backup
It has 2 commands:
Usage: