increase vtctlclient backupShard command success rate

[jwangace]

Description

In the case of at least one tablet being healthy, this PR would increase the chance of successful backup, by only returning Primary + healthy tablets.

In the case of all Non-Primary tablets having errors, vtctlclient should fail in the same way.

Related Issue(s)

#14602

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[shlomi-noach]

LGTM. We can potentially rewrite the if/else/loop logic somewhat to unify the different scenarios, but it's also good as it is. Thank you!

[jwangace]

LGTM. We can potentially rewrite the if/else/loop logic somewhat to unify the different scenarios, but it's also good as it is. Thank you!

Hi @shlomi-noach Correct, it could be more precise in the future, what I proposed was the simplest format to be less buggy. With this change, our current way of backup method would be happy.

Thanks for the review and approval.

[ajm188]

there is a small chance that we are doing BackupShard --allow-primary and the shard has exactly one tablet (the primary), and this could incorrectly fail. i'd suggest: if len(tablets) < len(shardTablets) instead

[jwangace]

Hi @ajm188 do you mean that the shard is built only with 1 tablet, so it's the only 1 and primary?

if the code changed to:

if err != nil {
...
    if len(tablets) < len(shardTablets) {
        return err
    }
}

In normal cases (tablets >= 2), then it should equivelant to before and does not fix the bug, becase len(tablets) < len(shardTablets) should always true when err != nil

if err != nil {
    return err
}

Instead, can I suggest tablets should always include the PRIMARY and no matter it's stat nil or non-nil?

shardTablets, stats, err := reparentutil.ShardReplicationStatuses(ctx, s.ts, s.tmc, req.Keyspace, req.Shard)

var tablets []*topo.TabletInfo
// Instead of return on err directly, only return when no healthy tablets at all
if err != nil {
	for i, stat := range stats {
		// Always include TabletType_PRIMARY
		if shardTablets[i].Type == topodatapb.TabletType_PRIMARY {
			tablets = append(tablets, shardTablets[i])
			continue
		}
		// shardTablets[i] and stats[i] is 1:1 mapping
		// Healthy shardTablets[i] will be added to tablets
		if stat != nil {
			tablets = append(tablets, shardTablets[i])
		}
	}
	// Only return err when no usable tablet
	if len(tablets) == 0 {
		return err
	}
} else {
	tablets = shardTablets
}

[ajm188]

ah good point.

do you mean that the shard is built only with 1 tablet, so it's the only 1 and primary?

yes, that's what i mean.

i missed that the if len(tablets) == 0 happened inside the if err != nil check, not outside, so i think this is a moot point. sorry for the confusion!

[shlomi-noach]

OK, given the intricate logic here I'd like to suggest extracting the logic into a separate function and adding a unit test suite to validate expected outcome of different scenarios 🙏

[ajm188]

I agree with Shlomi here

[jwangace]

updated PR

[shlomi-noach]

Looks great, appreciate the unit tests!

[ajm188]

couple small things, but looking good!

[ajm188]

suggest condensing this into either a switch or if/else and avoiding the continue

[ajm188]

alternatively:

if tablets[i].Type == topodatapb.TabletType_PRIMARY || stats != nil {
    res = append(...)
}

[jwangace]

Good approach.