kvserver/loqrecovery: persist new replica ID in RaftReplicaID#80470
kvserver/loqrecovery: persist new replica ID in RaftReplicaID#80470craig[bot] merged 2 commits intocockroachdb:masterfrom
RaftReplicaID#80470Conversation
The recently introduced local `RaftReplicaIDKey` was not updated when `unsafe-remove-dead-replicas` changed the replica's ID. This could lead to assertion failures. Release note: None
|
This change is |
aliher1911
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @sumeerbhola, and @tbg)
pkg/kv/kvserver/loqrecovery/recovery_env_test.go line 219 at r2 (raw file):
if err := sl.SetHardState(ctx, eng, hardState); err != nil { t.Fatalf("failed to save raft hard state: %v", err) }
We need to add:
if err := sl.SetRaftReplicaID(ctx, eng, localReplicaID); err != nil {
t.Fatalf("failed to set raft replica ID: %v", err)
}
so that we could have initial store in line with expectations and not have replicaID = 0 for unchanged replicas.
We can return that value from buildReplicaDescriptorFromTestData where all data is prepared based on test structs.
The recently introduced local `RaftReplicaIDKey` was not updated when loss of quorum recovery changed the replica's ID. This could lead to assertion failures. Release note: None
0c98606 to
00e7fdf
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/loqrecovery/recovery_env_test.go line 219 at r2 (raw file):
Previously, aliher1911 (Oleg) wrote…
We need to add:
if err := sl.SetRaftReplicaID(ctx, eng, localReplicaID); err != nil { t.Fatalf("failed to set raft replica ID: %v", err) }so that we could have initial store in line with expectations and not have replicaID = 0 for unchanged replicas.
We can return that value from buildReplicaDescriptorFromTestData where all data is prepared based on test structs.
Good call, done.
|
bors r=aliher1911 |
|
Build succeeded: |
I'm not certain that this is the cause of #75133 (wasn't able to reproduce), but it seems plausible. I think it's something that we'd need to fix anyway, but I'm not familiar with all of the nuance here, so I'd appreciate careful reviews.
For reference, this was introduced in #75761.
cli: persist new replica ID in
unsafe-remove-dead-replicasThe recently introduced local
RaftReplicaIDKeywas not updated whenunsafe-remove-dead-replicaschanged the replica's ID. This could leadto assertion failures.
Touches #75133.
Touches #79074.
Release note: None
kvserver/loqrecovery: persist new replica ID in
RaftReplicaIDThe recently introduced local
RaftReplicaIDKeywas not updated whenloss of quorum recovery changed the replica's ID. This could lead to
assertion failures.
Release note: None