-
Notifications
You must be signed in to change notification settings - Fork 308
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
fix: ensure keep alive is completed in time #4349
Conversation
WalkthroughRecent updates introduced new imports and logic changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EtcdElection
participant EtcdCluster
participant EtcdLeader
Client->>EtcdElection: Request to keep alive
activate EtcdElection
EtcdElection->>EtcdCluster: Request lease keep alive
activate EtcdCluster
EtcdCluster-->>EtcdElection: Lease keep alive response
deactivate EtcdCluster
EtcdElection->>EtcdLeader: Check leader status
activate EtcdLeader
EtcdLeader-->>EtcdElection: Leader status response
deactivate EtcdLeader
EtcdElection-->>Client: Respond with keep alive status
deactivate EtcdElection
Poem
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 Configration 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
Outside diff range, codebase verification and nitpick comments (2)
src/meta-srv/src/election/etcd.rs (2)
240-264
: Ensure thecampaign
function is tested.No direct tests for the
campaign
function were found in the codebase. It is crucial to add tests to verify the functionality and error handling of this function.
- Add unit tests or integration tests covering the
campaign
function to ensure its reliability.Analysis chain
Keep-alive logic in
campaign
looks good.The modifications ensure the keep-alive operation is completed within the specified interval, and appropriate error handling is in place.
However, ensure that this function and its new logic are adequately tested.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `campaign` function is tested. # Test: Search for tests related to the `campaign` function. Expect: Results indicating tests. rg --type rust -A 5 $'campaign'Length of output: 2114
Script:
#!/bin/bash # Description: Verify if the `campaign` function is tested. # Test: Search for test functions related to the `campaign` function, particularly in test directories or files. rg --type rust -A 5 'campaign' --glob '*test*'Length of output: 46
Script:
#!/bin/bash # Description: Broaden the search to verify if the `campaign` function is tested. # Test: Search for all references to the `campaign` function in the entire codebase. rg --type rust -A 5 'campaign'Length of output: 2114
300-342
: No tests found for thekeep_alive
function.The search did not yield any results indicating that the
keep_alive
function is directly tested in the identified test files. It is recommended to add explicit tests to ensure the function's behavior is thoroughly validated.
- Review and add tests for the
keep_alive
function to ensure it is adequately covered.Analysis chain
The
keep_alive
function looks good.The function correctly handles the keep-alive logic and includes appropriate error handling and logging.
However, ensure that this function is adequately tested.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `keep_alive` function is tested. # Test: Search for tests related to the `keep_alive` function. Expect: Results indicating tests. rg --type rust -A 5 $'keep_alive'Length of output: 11998
Script:
#!/bin/bash # Description: Search for test files and functions related to the `keep_alive` function. # Find all Rust test files in the codebase fd --type f --extension rs --exec grep -l '#[cfg(test)]' # Search for test functions that call the `keep_alive` function rg --type rust -A 5 'fn test_.*keep_alive'Length of output: 1376
Script:
#!/bin/bash # Description: Search for the `keep_alive` function in the identified test files. test_files=( "./tests-integration/tests/sql.rs" "./tests-integration/tests/grpc.rs" "./tests-integration/src/prom_store.rs" "./tests-integration/src/tests/instance_test.rs" "./src/sql/src/statements/show.rs" "./src/sql/src/statements/set_variables.rs" "./src/sql/src/statements/drop.rs" "./src/sql/src/statements/create.rs" "./src/sql/src/statements/describe.rs" "./src/sql/src/statements/alter.rs" "./src/sql/src/statements/truncate.rs" "./src/servers/src/prom_store.rs" "./src/query/src/promql/planner.rs" "./src/query/src/optimizer/count_wildcard.rs" "./src/servers/tests/http/prom_store_test.rs" "./src/promql/src/functions/changes.rs" "./src/puffin/src/file_metadata.rs" "./src/script/src/python/ffi_types/utils.rs" "./src/promql/src/extension_plan/scalar_calculate.rs" "./src/promql/src/extension_plan/empty_metric.rs" "./src/log-store/src/kafka/log_store.rs" "./src/mito2/src/request.rs" "./src/flow/src/transform/expr.rs" "./src/flow/src/transform/aggr.rs" "./src/flow/src/transform/literal.rs" "./src/flow/src/expr/scalar.rs" "./src/datatypes/src/value.rs" "./src/common/time/src/range.rs" "./src/common/mem-prof/src/jemalloc.rs" "./src/common/macro/src/stack_trace_debug.rs" "./src/common/macro/src/range_fn.rs" "./src/common/macro/src/admin_fn.rs" "./src/common/query/src/lib.rs" "./src/common/base/src/secrets.rs" "./src/cmd/src/cli/export.rs" ) # Search for the `keep_alive` function in the identified test files for file in "${test_files[@]}"; do rg 'keep_alive' "$file" doneLength of output: 4345
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/meta-srv/src/election/etcd.rs (3 hunks)
Additional comments not posted (1)
src/meta-srv/src/election/etcd.rs (1)
21-21
: Imports look good.The new imports are necessary for the keep-alive functionality and are correctly added.
Also applies to: 26-26
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4349 +/- ##
==========================================
- Coverage 85.18% 84.89% -0.30%
==========================================
Files 1060 1061 +1
Lines 189069 189503 +434
==========================================
- Hits 161057 160870 -187
- Misses 28012 28633 +621 |
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 (1)
- src/meta-srv/src/election/etcd.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/meta-srv/src/election/etcd.rs
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 (1)
- .github/actions/setup-greptimedb-cluster/action.yml (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/actions/setup-greptimedb-cluster/action.yml
aafca73
to
7dda31c
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .github/actions/setup-etcd-cluster/action.yml (1 hunks)
- src/common/meta/src/distributed_time_constants.rs (1 hunks)
- src/meta-srv/src/election/etcd.rs (3 hunks)
- src/meta-srv/src/metasrv.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/actions/setup-etcd-cluster/action.yml
Additional comments not posted (4)
src/common/meta/src/distributed_time_constants.rs (1)
36-36
: Increase inMETA_LEASE_SECS
durationThe
META_LEASE_SECS
constant has been increased from 3 to 5 seconds. This change affects the lease duration for themetasrv
leader. Ensure that this adjustment aligns with the desired lease duration for the system.src/meta-srv/src/election/etcd.rs (2)
21-21
: Approved: New imports for lease management and timeout handlingThe new imports
LeaderKey
,LeaseKeepAliveStream
,LeaseKeeper
, andtimeout
are essential for the newkeep_alive
method and enhanced error handling in thecampaign
method.Also applies to: 26-26
238-253
: Approved: Enhanced error handling with timeout incampaign
methodThe usage of
timeout
for the keep-alive operation in thecampaign
method ensures that the operation completes within the specified interval, which is crucial for maintaining leadership. The error handling is also enhanced.Verify that the
META_KEEP_ALIVE_INTERVAL_SECS
value is appropriate for the desired keep-alive interval.src/meta-srv/src/metasrv.rs (1)
453-453
: Approved: Change tocommon_runtime::spawn_write
The change from
common_runtime::spawn_bg
tocommon_runtime::spawn_write
in thetry_start
method impacts asynchronous operation handling. This adjustment is likely aimed at ensuring that the write operations are handled appropriately in the background.Verify that this change aligns with the desired behavior for asynchronous write operations.
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 (1)
- src/meta-srv/src/election/etcd.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/meta-srv/src/election/etcd.rs
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.
LGTM
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 (1)
- src/meta-srv/src/election/etcd.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/meta-srv/src/election/etcd.rs
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/meta-srv/src/election/etcd.rs (3 hunks)
Additional comments not posted (2)
src/meta-srv/src/election/etcd.rs (2)
21-22
: Approved: New imports are necessary and used appropriately.The new imports (
LeaderKey
,LeaseKeepAliveStream
,LeaseKeeper
, andtimeout
) are essential for thekeep_alive
method.Also applies to: 26-26
235-271
: Ensure proper error handling for timeout.The timeout for the
keep_alive
operation ensures it completes withinMETA_KEEP_ALIVE_INTERVAL_SECS
. The error handling seems appropriate, but consider logging additional context for debugging purposes.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
ensure keep alive is completed in time
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
common_runtime::spawn_bg
tocommon_runtime::spawn_write
for better asynchronous operation handling.Performance Improvements