-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
kvserver/loqrecovery: persist new replica ID in RaftReplicaID
#80470
kvserver/loqrecovery: persist new replica ID in RaftReplicaID
#80470
Conversation
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
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.
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
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @sumeerbhola)
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.
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.
🕺 🎉
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-replicas
The recently introduced local
RaftReplicaIDKey
was not updated whenunsafe-remove-dead-replicas
changed the replica's ID. This could leadto assertion failures.
Touches #75133.
Touches #79074.
Release note: None
kvserver/loqrecovery: persist new replica ID in
RaftReplicaID
The recently introduced local
RaftReplicaIDKey
was not updated whenloss of quorum recovery changed the replica's ID. This could lead to
assertion failures.
Release note: None