Attach version and member information to Raft configuration changes #2546
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The failing |
muhamadazmy
approved these changes
Jan 27, 2025
There was a problem hiding this comment.
Thank you @tillrohrmann for creating this PR. The changes looks good to me. I think you will probably need to rebase with main after I merge my changes. I have small changes in the protobuf file.
| MetadataStoreConfiguration configuration = 1; | ||
| // unfortunately map keys only support strings or integer types | ||
| // but not bytes. | ||
| map<string, VersionedValue> kv_entries = 2; |
There was a problem hiding this comment.
I changed this to entries you will probably need to rebase after I merge
crates/metadata-store/src/lib.rs
Outdated
| fn decode_from_bytes(mut bytes: Bytes) -> Result<Self, StorageDecodeError> { | ||
| StorageCodec::decode::<WriteRequest, _>(&mut bytes) | ||
| fn decode_from_bytes(bytes: Bytes) -> Result<Self, StorageDecodeError> { | ||
| let result = grpc::WriteRequest::decode(bytes).unwrap(); |
There was a problem hiding this comment.
I totally overlooked the unwrap() call. i will create a follow up PR to fix it.
tillrohrmann
force-pushed
the
configuration-version
branch
from
5ea4d47 to
65efad1
Compare
|
Thanks for the review @muhamadazmy. I've rebased onto the latest master. Merging once GHA gives green light. |
This commit adds version and member information to Raft configuration changes so that we can check for members that have lost their disks and improve observability.
Setting nodes that are not part of the configuration to Standby is problematic because it might set some nodes that are trying to join the cluster to Standby. If this happens, then the nodes will stop trying to join the cluster. Hence, we are only setting those nodes to Member which are part of the current configuration. Note, currently this does not change the NodesConfiguration since we use the Member state to tell nodes to join a cluster in the first place.
tillrohrmann
force-pushed
the
configuration-version
branch
from
65efad1 to
ace0c11
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds version and member information to Raft configuration changes
so that we can check for members that have lost their disks and improve
observability.
This PR is based on #2544.