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

[vtctldserver] Add additional backup info fields #8321

Merged
merged 5 commits into from
Jun 14, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Jun 12, 2021

Description

This PR adds the following additional fields to the BackupInfo proto message:

  • Keyspace => set by VtctldServer
  • Shard => set by VtctldServer
  • Time => the time the backup started, parsed out of the backuphandle name, set by BackupHandleToProto
  • TabletAlias => the alias of the tablet that took the backup, parsed out of the backuphandle name, set by BackupHandleToProto
  • Engine => the name of the backupstorage engine that produced the backup, currently unset, to be added in a future PR
  • Status => the status of the backup, to be set by a combination of the backupengine implementation and the backupstorage.BackupHandle implementation; currently unset, to be added in a future PR.

We also add limiting fields to the GetBackupsRequest, to reduce the number of backup infos that have to be marshaled and sent over network; this is especially useful in the case where the caller is interested only in the most recent backup. In a future change we can revisit adding time-based filtering (i.e. "give me all backups before $x and after $y"), but I didn't want to tackle that now.

Finally, I updated the CLI to expose the limit field, and also to change the posargs from $keyspace $shard as two arguments to the more standard ${keyspace}/${shard} single argument.

Examples:
❯ vtctldclient --server ":15999" GetBackups commerce/0

❯ vtctldclient --server ":15999" GetBackups commerce/0 -j
{
  "backups": []
}
❯ vtctlclient -server ":15999" Backup zone1-101
❯ vtctldclient --server ":15999" GetBackups commerce/0
2021-06-12.185714.zone1-0000000101
❯ vtctlclient -server ":15999" Backup zone1-102
❯ vtctldclient --server ":15999" GetBackups commerce/0
2021-06-12.185714.zone1-0000000101
2021-06-12.185730.zone1-0000000102
❯ vtctldclient --server ":15999" GetBackups commerce/0 -l 1
2021-06-12.185730.zone1-0000000102
❯ vtctldclient --server ":15999" GetBackups commerce/0 -l 1 -j
{
  "backups": [
    {
      "name": "2021-06-12.185730.zone1-0000000102",
      "directory": "commerce/0",
      "keyspace": "commerce",
      "shard": "0",
      "tablet_alias": {
        "cell": "zone1",
        "uid": 102
      },
      "time": {
        "seconds": "1623524250",
        "nanoseconds": 0
      },
      "engine": "",
      "status": 0
    }
  ]
}

Related Issue(s)

#7352

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@ajm188 ajm188 added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Cluster management labels Jun 12, 2021
@ajm188 ajm188 requested a review from doeg June 12, 2021 19:07
@ajm188 ajm188 requested a review from deepthi as a code owner June 12, 2021 19:07
@ajm188 ajm188 added this to In progress in Vtctld Service via automation Jun 12, 2021
Copy link
Contributor

@doeg doeg 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. Neither comment is blocking.

Name: bh.Name(),
Directory: bh.Directory(),
}

if parts := strings.Split(bi.Name, "."); len(parts) == 3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking (scope-creepy) suggestion: colocating this backup name parse logic with the format call that happens in mysqlctl/backup.go could be useful for searchability (and any future refactors).

topodata.TabletAlias tablet_alias = 5;
vttime.Time time = 6;

// The following fields are may or may not be currently set. Work is inflight
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful to create + link an issue for this. Up to you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

…tBackupsRequest

Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…, add limiting

Signed-off-by: Andrew Mason <amason@slack-corp.com>
We're not including the Detailed fields here yet, because they are
unused in the server implementation (for now).

Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188 ajm188 force-pushed the am_additional_backup_info branch from b280a4d to 411950f Compare June 14, 2021 21:02
@ajm188 ajm188 merged commit 83dfb9a into vitessio:main Jun 14, 2021
Vtctld Service automation moved this from In progress to Done Jun 14, 2021
@ajm188 ajm188 deleted the am_additional_backup_info branch June 14, 2021 22:09
ajm188 added a commit to tinyspeck/vitess that referenced this pull request Jun 15, 2021
…info

[vtctldserver] Add additional backup info fields

Signed-off-by: Andrew Mason <amason@slack-corp.com>
ajm188 added a commit to tinyspeck/vitess that referenced this pull request Jul 23, 2021
…info

[vtctldserver] Add additional backup info fields

Signed-off-by: Andrew Mason <amason@slack-corp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Cluster management Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants