-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
orc: vitess mode #6613
orc: vitess mode #6613
Conversation
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
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.
Please see comments/questions inline.
Overall everything here makes sense and seems like a pretty smooth integration; that is, it feels like the code changes belong here and play well with existing logic. Good work!
@@ -116,12 +125,16 @@ const ( | |||
type ReplicationAnalysis struct { | |||
AnalyzedInstanceKey InstanceKey | |||
AnalyzedInstanceMasterKey InstanceKey | |||
TabletType topodatapb.TabletType | |||
MasterTimeStamp time.Time |
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.
What does this timestamp stand for?
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.
This was our first step towards a consensus protocol. Every newly elected master saves its timestamp. If the previous master did not get properly demoted (maybe it wasn't reachable), then we use the timestamp to resolve who the real master is.
I'm not happy that we currently rely on a timestamp because rogue clocks can mess things up. We should eventually move towards a system that doesn't depend on the clock.
if !inst.IsBannedFromBeingCandidateReplica(replica) { | ||
candidate = replica | ||
} | ||
} |
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.
a bit too much if-else-if-else. Can you please explain the logic in words?
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.
I've refactored this. LMK if the new code reads better.
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
} | ||
refreshTabletsUsing(func(instanceKey *inst.InstanceKey) { | ||
_ = inst.InjectSeed(instanceKey) | ||
}) |
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.
This should work well upon startup
} | ||
refreshTabletsUsing(func(instanceKey *inst.InstanceKey) { | ||
_ = inst.InjectSeed(instanceKey) | ||
}) | ||
// TODO(sougou): parameterize poll interval. | ||
return time.Tick(15 * time.Second) //nolint SA1015: using time.Tick leaks the underlying ticker | ||
} | ||
|
||
// RefreshTablets reloads the tablets from topo. | ||
func RefreshTablets() { |
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.
What's the purpose/usage of this function?
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.
This is called from the main discovery loop. It polls for new/deleted tablets, and updates Orc accordingly.
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.
got it. Makes sense; one thing to note is that this causes a synchronous topology read on all tablets. Possibly this isn't what you wanted? Consider calling DiscoverInstance()
instead, which will queue the instance for discovery, and avoid double-probing it within a pre configured timeslice.
Signed-off-by: Sugu Sougoumarane <[email protected]>
@@ -59,7 +59,7 @@ func OpenTabletDiscovery() <-chan time.Time { | |||
// RefreshTablets reloads the tablets from topo. | |||
func RefreshTablets() { | |||
refreshTabletsUsing(func(instanceKey *inst.InstanceKey) { | |||
_, _ = inst.ReadTopologyInstance(instanceKey) | |||
DiscoverInstance(*instanceKey) |
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.
🤞 let's see how this goes!
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.
Code walkthrough and getting all the TODOs makes code read better.
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.
Also LGTM
This is a POC/MVP of some sort where Orchestrator runs as a vitess component. Features of this PR are described in #6612.
This is not production ready yet, but people can start playing with it. The code mostly works, but there are lots of TODOs, and tests have to be written.
This version does not handle semi-sync settings correctly.
High level
vitess_tablet
table, and uses them as the authoritative source of mysql instances. For example, if a tablet record is deleted, the mysql instance is forgotten.vitess_tablet
table and left joins on database_instance.Details
cluster_alias_override
is not used because vitess is the authority here.SuggestedClusterAlias
as the authoritative source. We'll need to eventually change this toClusterName
. But there are places where the cluster name is inferred from the instance key.ChangeMasterTo
always sets the credentials, with deprecatesChangeMasterCredentials
.inst.BeginDowntime
of the dead master. In the case of vitess, we should wire it back to the cluster when it comes back up. But this downtime was preventing it.LockShard
is sprayed in a few places, but it's not watertight yet.ChangeType(MASTER)
on the tablet. If anything fails after this, this master status is used to authoritatively finish anything that was incomplete.MasterHasMaster
has been created, which is evaluated based on whether the tablet record is the master.electNewMaster
function has been added to elect a brand new master if no previous master existed. This replaces the vitessInitShardMaster
.