-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature: add absolute timestamp RaftMetrics::last_quorum_acked
#94
Conversation
`RaftMetrics::last_quorum_acked` is the absolute timestamp of the most recent time point that is accepted by a quorum via `AppendEntries` RPC. This field is a wrapped `Instant` type: `SerdeInstant` which support serde for `Instant`. This field is added as a replacement of `millis_since_quorum_ack`, which is a relative time. `SerdeInstant` serialize `Instant` into a string formatted as "%Y-%m-%dT%H:%M:%S%.9f%z", e.g., "2024-07-24T04:07:32.567025000+0000". Note: Serialization and deserialization are not perfectly accurate and can be indeterministic, resulting in minor variations each time. These deviations(could be smaller or greater) are typically less than a microsecond (10^-6 seconds).
WalkthroughThe recent changes enhance the Raft protocol's metrics handling and error management, introducing better serialization for time-related metrics and refining error documentation. Key updates include the implementation of Changes
Sequence Diagram(s)sequenceDiagram
participant Leader
participant Quorum
participant Metrics
Leader->>Quorum: Sends heartbeat
Quorum-->>Leader: Acknowledgment received
Leader->>Metrics: Update last_quorum_acked timestamp
Metrics-->>Leader: Store updated timestamp
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- openraft/src/core/raft_core.rs (3 hunks)
- openraft/src/error.rs (1 hunks)
- openraft/src/metrics/mod.rs (2 hunks)
- openraft/src/metrics/raft_metrics.rs (7 hunks)
- openraft/src/metrics/serde_instant.rs (1 hunks)
- openraft/src/metrics/wait_test.rs (2 hunks)
- scripts/check.kdl (1 hunks)
- tests/tests/fixtures/mod.rs (2 hunks)
- tests/tests/metrics/t10_leader_last_ack.rs (4 hunks)
Files skipped from review due to trivial changes (2)
- openraft/src/error.rs
- scripts/check.kdl
Additional context used
GitHub Check: lint
tests/tests/fixtures/mod.rs
[failure] 157-157:
single-character string constant used as pattern
[failure] 164-164:
single-character string constant used as pattern
[failure] 157-157:
single-character string constant used as pattern
[failure] 164-164:
single-character string constant used as pattern
[failure] 157-157:
single-character string constant used as pattern
[failure] 164-164:
single-character string constant used as pattern
Additional comments not posted (28)
openraft/src/metrics/mod.rs (2)
35-35
: Addition ofserde_instant
module looks good.The new module
serde_instant
is correctly added to the list of modules.
46-46
: Export ofSerdeInstant
looks good.The
SerdeInstant
type is correctly exported for use in other modules.openraft/src/metrics/serde_instant.rs (8)
1-7
: Imports look good.The necessary modules and traits are correctly imported.
8-23
:SerdeInstant
struct definition looks good.The struct is correctly defined with necessary traits.
25-33
:Deref
implementation looks good.The
Deref
trait is correctly implemented forSerdeInstant
.
35-41
:From
implementation looks good.The
From
trait is correctly implemented forSerdeInstant
.
43-49
:fmt::Display
implementation looks good.The
fmt::Display
trait is correctly implemented forSerdeInstant
.
51-61
:SerdeInstant
methods look good.The methods for creating a new instance and extracting the inner type are correctly implemented.
63-201
:serde_impl
module looks good.The module correctly implements serialization and deserialization for
SerdeInstant
.
162-201
: Tests forSerdeInstant
look good.The tests are comprehensive and cover edge cases.
tests/tests/metrics/t10_leader_last_ack.rs (4)
Line range hint
17-104
:
Updates toleader_last_ack_3_nodes
look good.The function correctly includes assertions for
millis_since_quorum_ack
.
119-210
: New functionleader_last_ack_3_nodes_abs_time
looks good.The function correctly tests the
last_quorum_acked
timestamp in a three-node configuration.
Line range hint
216-254
:
Updates toleader_last_ack_1_node
look good.The function correctly includes assertions for
last_quorum_acked
.
256-258
:timeout
function looks good.The function correctly provides a timeout duration for the tests.
openraft/src/metrics/wait_test.rs (3)
252-252
: LGTM! Suppressing deprecated warnings is acceptable here.The
#[allow(deprecated)]
attribute is necessary to maintain functionality without addressing deprecation immediately.
265-265
: LGTM! The addition of thelast_quorum_acked
field enhances metrics tracking.The
last_quorum_acked
field provides more precise information about the leader's synchronization status with the cluster.
Line range hint
252-265
: LGTM! Initialization oflast_quorum_acked
toNone
maintains consistency.The
init_wait_test
function correctly initializes thelast_quorum_acked
field toNone
, ensuring consistency in the structure's state.openraft/src/metrics/raft_metrics.rs (6)
10-11
: LGTM! ImportingSerdeInstant
is necessary for serialization and deserialization.The
SerdeInstant
type is required for handling the newlast_quorum_acked
field.
78-78
: LGTM! Deprecatingmillis_since_quorum_ack
is appropriate.The deprecation of
millis_since_quorum_ack
in favor oflast_quorum_acked
provides a more precise representation of acknowledgment timing.
93-93
: LGTM! Addinglast_quorum_acked
enhances metrics tracking.The
last_quorum_acked
field provides more precise information about the leader's synchronization status with the cluster.
132-141
: LGTM! Updatingfmt::Display
implementation to includelast_quorum_acked
is correct.The
fmt::Display
implementation is correctly updated to format and display the newlast_quorum_acked
field.
208-208
: LGTM! Deprecatingmillis_since_quorum_ack
is appropriate.The deprecation of
millis_since_quorum_ack
in favor oflast_quorum_acked
provides a more precise representation of acknowledgment timing.
223-223
: LGTM! Addinglast_quorum_acked
enhances metrics tracking.The
last_quorum_acked
field provides more precise information about the leader's synchronization status with the cluster.tests/tests/fixtures/mod.rs (2)
135-138
: LGTM! Enhancingset_panic_hook
to store and call the previous hook is correct.Storing the previous panic hook and calling it after logging ensures that the original behavior is preserved while adding logging functionality.
157-164
: LGTM! Adjustinglog_panic
to improve log readability and use structured logging is correct.Replacing newline characters with spaces in the panic message improves readability. Using
tracing::error!
for structured logging enhances error reporting.Tools
GitHub Check: lint
[failure] 157-157:
single-character string constant used as pattern
[failure] 164-164:
single-character string constant used as pattern
[failure] 157-157:
single-character string constant used as pattern
[failure] 164-164:
single-character string constant used as pattern
[failure] 157-157:
single-character string constant used as pattern
[failure] 164-164:
single-character string constant used as patternopenraft/src/core/raft_core.rs (3)
63-63
: Import ofSerdeInstant
is correct.The import statement is necessary for the changes made to the
RaftMetrics
andRaftDataMetrics
structures.
574-574
: Change tolast_quorum_acked
field inRaftMetrics
is correct.The change to use
SerdeInstant
for thelast_quorum_acked
field enhances timestamp serialization capabilities.However, ensure that all references to
last_quorum_acked
in the codebase are updated to handleSerdeInstant
.Verification successful
Change to
last_quorum_acked
field inRaftMetrics
is correct.The change to use
SerdeInstant
for thelast_quorum_acked
field enhances timestamp serialization capabilities. The references tolast_quorum_acked
in the core implementation files (raft_core.rs
,leader.rs
,raft_metrics.rs
) have been updated to handle the newSerdeInstant
type correctly.
openraft/src/core/raft_core.rs
: References tolast_quorum_acked
have been updated to useSerdeInstant
.openraft/src/proposer/leader.rs
: Functions related tolast_quorum_acked_time
handleSerdeInstant
.openraft/src/metrics/raft_metrics.rs
: Thelast_quorum_acked
field is correctly defined asSerdeInstant
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `last_quorum_acked` in the codebase are updated to handle `SerdeInstant`. # Test: Search for the `last_quorum_acked` references. Expect: Only occurances of the new type. rg --type rust -A 5 $'last_quorum_acked'Length of output: 14969
589-589
: Change tolast_quorum_acked
field inRaftDataMetrics
is correct.The change to use
SerdeInstant
for thelast_quorum_acked
field enhances timestamp serialization capabilities.However, ensure that all references to
last_quorum_acked
in the codebase are updated to handleSerdeInstant
.Verification successful
All references to
last_quorum_acked
in the codebase are correctly updated to handleSerdeInstant
.
- The references in the following files are correctly updated:
tests/tests/metrics/t10_leader_last_ack.rs
openraft/src/core/raft_core.rs
openraft/src/proposer/leader.rs
openraft/src/metrics/wait_test.rs
openraft/src/metrics/raft_metrics.rs
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `last_quorum_acked` in the codebase are updated to handle `SerdeInstant`. # Test: Search for the `last_quorum_acked` references. Expect: Only occurances of the new type. rg --type rust -A 5 $'last_quorum_acked'Length of output: 14969
Script:
#!/bin/bash # Description: Verify that all references to `last_quorum_acked` in the codebase are updated to handle `SerdeInstant`. # Check if `SerdeInstant` is used correctly in the context of `last_quorum_acked` references. rg --type rust -A 5 'last_quorum_acked' | rg 'SerdeInstant'Length of output: 465
Script:
#!/bin/bash # Description: Verify that the references to `last_quorum_acked` in `tests/tests/metrics/t10_leader_last_ack.rs` are updated to handle `SerdeInstant`. # Check the usages of `last_quorum_acked` in the test file. rg --type rust 'last_quorum_acked' tests/tests/metrics/t10_leader_last_ack.rsLength of output: 1160
Script:
#!/bin/bash # Description: Verify that the references to `last_quorum_acked` in `tests/tests/metrics/t10_leader_last_ack.rs` are updated to handle `SerdeInstant`. # Display the context around each reference to `last_quorum_acked` in the test file. rg --type rust -A 5 -B 5 'last_quorum_acked' tests/tests/metrics/t10_leader_last_ack.rsLength of output: 2998
RaftMetrics::last_quorum_acked
is the absolute timestamp of the most recent time point that is accepted by a quorum viaAppendEntries
RPC. This field is a wrappedInstant
type:SerdeInstant
which support serde forInstant
. This field is added as a replacement ofmillis_since_quorum_ack
, which is a relative time.SerdeInstant
serializeInstant
into a string formatted as "%Y-%m-%dT%H:%M:%S%.9f%z", e.g., "2024-07-24T04:07:32.567025000+0000".Note: Serialization and deserialization are not perfectly accurate and can be indeterministic, resulting in minor variations each time. These deviations(could be smaller or greater) are typically less than a microsecond (10^-6 seconds).
Checklist
Summary by CodeRabbit
New Features
last_quorum_acked
, to track leader acknowledgment timing inRaftMetrics
andRaftDataMetrics
.Bug Fixes
Tests
Chores