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

Check tablet alias before removing after error stream #7915

Merged
merged 2 commits into from
Apr 22, 2021

Conversation

5antelope
Copy link
Member

@5antelope 5antelope commented Apr 21, 2021

Signed-off-by: crowu y.wu4515@gmail.com

Description

When VTGate gets an error from the primary tablet connection, it should not just nuke out all the healthy tablets. Because an external reparent process might have already changed the primary tablet.

Instead we should only try to remove the tablet with stream error from the map

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

Signed-off-by: crowu <y.wu4515@gmail.com>
@5antelope 5antelope requested a review from deepthi as a code owner April 21, 2021 17:34
@@ -448,8 +453,7 @@ func (hc *HealthCheckImpl) updateHealth(th *TabletHealth, prevTarget *query.Targ
}
}
case isPrimary && !isPrimaryUp:
// No healthy master tablet
hc.healthy[targetKey] = []*TabletHealth{}
hc.deleteHealthDateLocked(targetKey, tabletAlias)
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this is correct. We want to delete it from healthy but not from healthData.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to keep it same as the original logic.

Signed-off-by: crowu <y.wu4515@gmail.com>
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

@deepthi
Copy link
Member

deepthi commented Apr 21, 2021

This should also go into 10.0 (release-10.0 branch)

// No healthy master tablet
hc.healthy[targetKey] = []*TabletHealth{}
if healthy, ok := hc.healthy[targetKey]; ok && len(healthy) > 0 {
var newHealthy []*TabletHealth
Copy link
Member Author

@5antelope 5antelope Apr 21, 2021

Choose a reason for hiding this comment

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

@deepthi i don't know if this is the proper way to do the deletion in this case, please let me know if you have better suggestions

Copy link
Member

Choose a reason for hiding this comment

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

This is fine too.

@deepthi deepthi merged commit b171b47 into vitessio:master Apr 22, 2021
@systay systay added Component: Cluster management Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Apr 28, 2021
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants