Converge divergent shard-id persisted in nodes.conf to master's shard…#3229
Conversation
roshkhatri
force-pushed
the
backport-divergent-shardid-7.2
branch
from
06c395c to
f0a5d97
Compare
… id (valkey-io#2174) Handle divergent shard-id across master and replica from nodes.conf and reconcile all the nodes in the shard to the master node's shard-id. Fixes valkey-io#2171 (cherry picked from commit 4a3e713) Signed-off-by: Harkrishn Patro <[email protected]> Signed-off-by: Roshan Khatri <[email protected]>
roshkhatri
force-pushed
the
backport-divergent-shardid-7.2
branch
from
f0a5d97 to
4f8df34
Compare
sarthakaggarwal97
left a comment
sarthakaggarwal97
left a comment
There was a problem hiding this comment.
@roshkhatri known 7.2 CI failures? also why not do it in #3139 itself?
| if (clusterGetMaxEpoch() > server.cluster->currentEpoch) { | ||
| server.cluster->currentEpoch = clusterGetMaxEpoch(); | ||
| } | ||
| if (dirty || shard_id_reconciled) clusterSaveConfigOrDie(1); |
There was a problem hiding this comment.
maybe I missed, why did we add this?
There was a problem hiding this comment.
@roshkhatri Do you want to answer this before we merge?
In unstable, I can't see that we save cluster config after loading it even if we have reconciled the shard ids.
There was a problem hiding this comment.
we needed those to get the same behavior, the configs did not save to the files after the shardid we reconciled in-memory
The tests were failing. I will look into unstable why we dont need these there
There was a problem hiding this comment.
So in clusterUpdateMyselfFlags in clusterInit,
on unstable:
(myself->flags != oldflags) is always true as we are adding new flags so the configs are saved automatically before sleep
on 7.2:
we dont add flags and (myself->flags != oldflags) is not true so we dont add clusterDoBeforeSleep(CLUSTER_TODO_SAVE_CONFIG | CLUSTER_TODO_UPDATE_STATE);
Maybe we can add a save config before sleep in clusterinit by default on 7.2 and we wont need this
There was a problem hiding this comment.
Thanks. That would affect other behavior too? Maybe best to do the minimum impact.
There was a problem hiding this comment.
+1 to minimum blast radius
There was a problem hiding this comment.
I'll merge this as-is then.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 7.2 #3229 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
4 participants
Backporting #2174 to 7.2 branch
Handle divergent shard-id across master and replica from nodes.conf and reconcile all the nodes in the shard to the master node's shard-id.
Fixes #2171
(cherry picked from commit 4a3e713)