feat: background worker routines to shutdown client for migration #2538
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant Supervisor
participant BackgroundThreads
participant Client
Main->>Supervisor: Start supervisor
Supervisor->>BackgroundThreads: Initiate TSS handling
BackgroundThreads->>Client: Monitor TSS updates
Client-->>BackgroundThreads: Notify TSS change
BackgroundThreads->>Supervisor: Log update and exit
Supervisor-->>Main: Notify completion
Assessment against linked issues
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 (
|
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
zetaclient/orchestrator/orchestrator.go (1)
Line range hint
65-71:
Ensure proper error handling forOnBeforeStop.The
OnBeforeStopmethod ofoc.zetacoreClientshould have proper error handling to ensure that any issues during the shutdown process are logged and managed appropriately.+ if err := oc.zetacoreClient.OnBeforeStop(shutdownOrchestrator); err != nil { + oc.logger.Std.Error().Err(err).Msg("error during OnBeforeStop") + }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- cmd/zetaclientd-supervisor/lib.go (5 hunks)
- cmd/zetaclientd-supervisor/main.go (1 hunks)
- cmd/zetaclientd/start.go (6 hunks)
- contrib/localnet/docker-compose.yml (2 hunks)
- pkg/bg/bg.go (4 hunks)
- pkg/retry/retry.go (1 hunks)
- zetaclient/context/app.go (1 hunks)
- zetaclient/orchestrator/orchestrator.go (1 hunks)
- zetaclient/zetacore/background_threads.go (1 hunks)
- zetaclient/zetacore/client_query_observer.go (1 hunks)
Additional context used
Path-based instructions (9)
pkg/bg/bg.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd-supervisor/main.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/retry/retry.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/zetacore/background_threads.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/zetacore/client_query_observer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd-supervisor/lib.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/context/app.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd/start.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/orchestrator/orchestrator.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (19)
pkg/bg/bg.go (3)
14-14: LGTM! The newcancelfield enhances flexibility.Adding
cancelof typecontext.CancelCauseFuncallows for more robust task management.
23-25: LGTM! The newWithCancelfunction adds flexibility.The
WithCancelfunction allows users to set thecancelfield, enhancing the configuration's flexibility.
Line range hint
36-60: LGTM! TheWorkfunction changes enhance functionality.The
Workfunction now uses thecancelfunction if provided, allowing the background task to signal a restart based on its outcome.cmd/zetaclientd-supervisor/main.go (1)
72-81: LGTM! Enhanced error handling and logging.The changes improve the visibility of the process's exit status and manage the lifecycle of goroutines more effectively.
pkg/retry/retry.go (1)
49-52: LGTM! The newDefaultConstantBackofffunction enhances retry capabilities.The
DefaultConstantBackofffunction provides a specific backoff configuration, enhancing the retry capabilities.zetaclient/zetacore/background_threads.go (3)
14-59: LGTM! Ensure the ticker interval is appropriate.The function is well-structured with proper error handling and logging. The use of retry logic for initial TSS retrieval is a good practice. Verify that the ticker interval of 5 seconds is appropriate for your use case.
61-110: LGTM! Ensure the ticker interval is appropriate.The function is well-structured with proper error handling and logging. The use of retry logic for initial TSS history retrieval is a good practice. Verify that the ticker interval of 5 seconds is appropriate for your use case.
112-161: LGTM! Ensure the ticker interval is appropriate.The function is well-structured with proper error handling and logging. The use of retry logic for initial keygen retrieval is a good practice. Verify that the ticker interval of 5 seconds is appropriate for your use case.
zetaclient/zetacore/client_query_observer.go (1)
170-176: LGTM! Ensure the retry logic is correctly implemented.The method is well-structured with proper error handling. The use of retry logic is a good practice. Verify that the retry logic is correctly implemented.
cmd/zetaclientd-supervisor/lib.go (2)
22-22: LGTM! Ensure the new method is correctly implemented.The change improves security by using
grpc.WithTransportCredentials(insecure.NewCredentials())instead of the deprecatedgrpc.WithInsecure(). Verify that the new method is correctly implemented.Also applies to: 81-81
225-226: LGTM! Ensure consistent use of the new variable name.The change improves code readability by changing the variable name
configtocfg. Verify that the new variable name is consistently used throughout the code.Also applies to: 234-234
contrib/localnet/docker-compose.yml (1)
127-127: Ensure service resilience withrestart: always.The addition of
restart: alwaysensures that the services automatically restart if they stop or crash, enhancing resilience and uptime.Also applies to: 144-144
zetaclient/context/app.go (1)
73-75: New methodLogger()added toAppContext.The new method provides direct access to the logger instance, which enhances the functionality by allowing logging capabilities through the context object.
cmd/zetaclientd/start.go (5)
216-218: Create a notification context for background threads.The
notifyCtxcontext is created usingcontext.WithCancelCause, allowing coordinated shutdown signals between the main thread and background processes.
220-220: Start background threads.The
startBackgroundThreadsfunction is deferred to ensure background threads are started and managed properly.
330-338: Clarify variable naming for orchestrator.The variable name change from
orchestratortocctxOrchestratorenhances clarity in the code.
362-370: Refine shutdown signal handling.The shutdown signal handling is refined using a
selectstatement to wait for either thenotifyCtxto be done or a system interrupt signal, improving robustness.
439-471: IntroducestartBackgroundThreadsfunction.The
startBackgroundThreadsfunction initiates multiple background tasks, each responsible for handling specific TSS-related operations, enhancing the maintainability and functionality of the code.zetaclient/orchestrator/orchestrator.go (1)
Line range hint
65-71:
Verify the correctness of the new shutdown process.The shutdown process now relies on closing the
oc.stopchannel and invoking theOnBeforeStopmethod ofoc.zetacoreClient. Ensure that this new approach correctly stops all observers and performs any necessary cleanup.Verification successful
The new shutdown process correctly stops all observers and performs necessary cleanup.
The
oc.stopchannel is closed to signal the shutdown, and theOnBeforeStopmethod is used to perform any necessary cleanup before stopping.
- Closing the
oc.stopchannel correctly stops the orchestrator.- The
OnBeforeStopmethod ensures necessary cleanup is performed before shutdown.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new shutdown process correctly stops all observers and performs necessary cleanup. # Test: Search for the `Stop` method in the codebase. Expect: Ensure that the `Stop` method is not required for proper shutdown. rg --type go -A 5 $'func Stop'Length of output: 31
Script:
#!/bin/bash # Description: Verify the usage of the `oc.stop` channel and `OnBeforeStop` method in the codebase. # Search for the usage of the `oc.stop` channel. rg --type go 'oc\.stop' # Search for the `OnBeforeStop` method in the codebase. rg --type go 'OnBeforeStop' # Verify if there are any other shutdown or cleanup mechanisms in place. rg --type go -A 5 'func Shutdown'Length of output: 770
Script:
#!/bin/bash # Description: Review the implementation of the `OnBeforeStop` method and the usage of the `oc.stop` channel. # Review the implementation of the `OnBeforeStop` method. rg --type go -A 10 'func (c *Client) OnBeforeStop' zetaclient/zetacore/client.go # Review the usage of the `oc.stop` channel in the orchestrator code. rg --type go -A 10 'oc\.stop' zetaclient/orchestrator/orchestrator.goLength of output: 699
There was a problem hiding this comment.
Would be good to have a review from @swift1337 for this PR. 🙏
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- cmd/zetaclientd-supervisor/main.go (1 hunks)
- cmd/zetaclientd/start.go (3 hunks)
- pkg/bg/bg.go (4 hunks)
- zetaclient/zetacore/client_tss_migration_listners.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- cmd/zetaclientd-supervisor/main.go
- cmd/zetaclientd/start.go
Additional context used
Path-based instructions (2)
pkg/bg/bg.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/zetacore/client_tss_migration_listners.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (8)
pkg/bg/bg.go (4)
14-14: LGTM!The addition of the
cancelfield enhances the flexibility of the configuration.
23-25: LGTM!The
WithCancelfunction allows users to set thecancelfield when configuring theconfigstruct, enhancing the flexibility of the configuration.
36-36: LGTM!The
Workfunction initializes thecancelfield tonil.
51-59: LGTM!The
Workfunction now includes logic to use thecancelfunction if it is provided, allowing the background task to signal a cancellation based on its outcome. This enhances error handling and control flow.zetaclient/zetacore/client_tss_migration_listners.go (4)
18-45: LGTM!The
StartTssMigrationRoutinesfunction starts background threads for handling TSS updates, new keygen, and new TSS key generation. The use of theWithCanceloption allows the main thread to be restarted based on the outcome of the background tasks.
48-92: LGTM!The
HandleTSSUpdatefunction listens for TSS updates and returns when the TSS address is updated. The function handles errors and logs warnings if it fails to retrieve the TSS.
94-142: LGTM!The
HandleNewTSSKeyGenerationfunction listens for new TSS key generation and returns when a new key is generated. The function handles errors and logs warnings if it fails to retrieve the TSS history.
144-193: LGTM!The
HandleNewKeygenfunction listens for new keygen and returns when a new keygen is set. The function handles errors and logs warnings if it fails to retrieve the keygen.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- cmd/zetaclientd-supervisor/main.go (2 hunks)
- pkg/bg/bg.go (3 hunks)
- zetaclient/zetacore/client_tss_migration_listners.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- cmd/zetaclientd-supervisor/main.go
- zetaclient/zetacore/client_tss_migration_listners.go
Additional context used
Path-based instructions (1)
pkg/bg/bg.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (3)
pkg/bg/bg.go (3)
12-14: LGTM!The addition of the
callbackfield in theconfigstruct enhances flexibility by allowing for a cancellation mechanism.
23-25: LGTM!The new function
WithCallbackprovides a useful way to set thecallbackfield in theconfigstruct.
34-36: Suggestion: Initializecallbackfield tonil.The initialization of the
callbackfield tonilis correct and ensures that the callback mechanism is optional.
|
Let's wait on #2560 before considering merging this one |
# Conflicts: # changelog.md # zetaclient/chains/interfaces/interfaces.go
Yes , the TSS migration test would use these routines |
…restart-thread-zetaclient # Conflicts: # zetaclient/maintenance/tss_listener.go
lumtis
changed the title
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
This pr adds three background threads which can trigger the zeta-client to shutdown to help the process of migration
Thread 1 : Check for keygen and restart if new keygen is set
Thread 2 : Check TSS list and restart if a new tss is added
Thread 3 : Check current TSS , and restart if it is updated
Closes #2455
#2492
How Has This Been Tested?
Summary by CodeRabbit
New Features
Logger()method to access logging capabilities within the application context.GetTSSto retrieve TSS objects directly from the observer client.Improvements
Bug Fixes