Improve replicated-loglet restatectl commands #2681
Merged
+652
−237
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
This was referenced Feb 9, 2025
AhmedSoliman
force-pushed
the
pr2681
branch
2 times, most recently
from
d774db8 to
22468ce
Compare
|
|
Note to reviewers. The code is far from being pretty, and I'd try and avoid tackling nits at the moment, so the main goal is to inform and spot if there is a clear bug in logic. |
This also adds a little bit more stress to the tests to help them fail more often if there is an issue ``` // intentionally empty ```
This fixes mishandling of deleted and unknown nodes in the config in f-majority checks. Integration tests were misconfigured where the nodeset was [N2..N4] where N4 didn't actually exist in config. In this case we should not accept f-majority seal if only one node is sealed (replication=2) Although this bug wouldn't impact us immediately, it's best to fix this condition and I took it as an opportunity to update the semantics of provisioning state to match the latest design direction. Documentation has also been updated to reflect the correct semantics. Summary: - Nodes observed in node-set but not in nodes-config is treated as "provisioning" rather than "disabled" - Nodes that are "deleted" in config (tombstone exists) are treated as "disabled" - Nodes in provisioning are fully authoritative, but are automatically excluded from new nodesets (already filtered by candidacy filter in nodeset selector) - If provisioning nodes were added to the nodeset, they are treated as fully authoritative and are required to participate in f-majority. ``` // intentionally empty ```
- Trim operation will wait for f-majority before reporting success to increase reliability of subsequent get_trim_point - Adds protection against a dangerous scenario if the loglet over-reported its trim point in a sealed loglet case. The loglet might have more records than the effective sealed tail, it should never report a trim point beyond that (if this happens, the system will believe that the subsequent segment is missing records) - Remove superfluous check. The trim task already checks that trim point is clamped to the known global tail ``` // intentionally empty ```
- `restatectl replicated-loglet info` now prints a table with info from every node in the nodeset - `restatectl replicated-loglet digest` doesn't require --from/--to to function, and fixes for overblown memory usage if the supplied range is unnecessarily large - For both commands, lots of UI improvements. Some screenshots will be attached in comments. ``` // intentionally empty ```
tillrohrmann
approved these changes
Feb 10, 2025
| .copied() | ||
| .filter_map(|node_id| { | ||
| let node = nodes_config.find_node_by_id(node_id).unwrap_or_else(|_| { | ||
| panic!("Node {node_id} doesn't seem to exist in nodes configuration"); |
There was a problem hiding this comment.
Could this happen if we are operating on a slightly outdated NodesConfiguration?
There was a problem hiding this comment.
Yes, in theory it can happen.
| records_table.add_row(std::iter::repeat("═════════").take(nodeset.len() + 1)); | ||
| // append the node-level info at the end | ||
| { | ||
| let mut row = Vec::with_capacity(nodeset.len() + 2); |
There was a problem hiding this comment.
Could be nodeset.len() + 1.
There was a problem hiding this comment.
We have one for offset, and one for "issues", and yeah it can be 1 since I don't use the issues column in this, but would it matter if I leave it to match the table? :)
Would you consider this a nitpick?
There was a problem hiding this comment.
Yes, feel free to ignore.
muhamadazmy
reviewed
Feb 10, 2025
| for (offset, responses) in digests.iter() { | ||
| checker.fill_with_default(); | ||
| if *offset >= digests.max_local_tail() { | ||
| break; | ||
| } | ||
| if *offset == known_global_tail.latest_offset() { | ||
| // divider to indicate that everything after global tail | ||
| records_table.add_row(std::iter::repeat("────").take(nodeset.len() + 2)); |
There was a problem hiding this comment.
When trying the command out I got this output
Which conflicts with the comment here, the line should be after the value of 75095 not before it
There was a problem hiding this comment.
The global tail is 75095, the output is correct.
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.
restatectl replicated-loglet infonow prints a table with info from every node in the nodesetrestatectl replicated-loglet digestdoesn't require --from/--to to function, and fixes for overblown memory usage if the supplied range is unnecessarily largeStack created with Sapling. Best reviewed with ReviewStack.