Improve node and services closing #1554
Merged
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
WalkthroughThe recent changes focus on improving the clean shutdown process of the full node and its associated services by introducing a Changes
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 Configration File (
|
tzdybal
added
T:bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- block/block_sync.go (1 hunks)
- block/header_sync.go (1 hunks)
- node/full.go (1 hunks)
- node/full_node_integration_test.go (1 hunks)
- store/store.go (1 hunks)
- store/store_test.go (1 hunks)
- store/types.go (1 hunks)
Additional comments: 8
store/types.go (1)
- 44-45: The addition of the
Closemethod to theStoreinterface is a necessary and well-implemented change to support clean shutdown processes. This aligns with the objectives of the PR and follows Go's idiomatic practices for resource management.store/store_test.go (1)
- 132-165: The modifications to the
TestRestarttest case are well-designed to test the newClosemethod's functionality in the store. Creating a temporary directory for the KV store, properly closing and reopening the store, and asserting the expected height after reopening are all crucial steps that have been correctly implemented. The use ofrequire.NoErrorfor error handling is appropriate for ensuring the test stops immediately if an error occurs.store/store.go (1)
- 42-45: The implementation of the
Closemethod in theDefaultStorestruct is correctly done, effectively delegating the close operation to the underlying datastore'sClosemethod. This ensures that the datastore is properly closed, aligning with the objectives of enabling a clean shutdown process for the store.block/block_sync.go (1)
- 224-228: The modifications to the
Stopmethod in theBlockSyncService, including adjusting the order of stopping services and placing theblockStore.Stopcall at the end, are well-thought-out changes that contribute to a cleaner and more orderly shutdown process. The use oferrors.Joinfor error aggregation during the shutdown is a good practice that enhances error handling.block/header_sync.go (1)
- 221-225: The modifications to the
Stopmethod, specifically the use oferrors.Joinfor error aggregation, are well-implemented. This approach enhances the readability and maintainability of the error handling logic during the shutdown sequence of theHeaderSyncService. Good job on ensuring a clean and orderly shutdown process.node/full.go (1)
- 414-425: The adjustments made to the
OnStopmethod, including the incorporation ofp2pClient.Close()andStore.Close()into the error aggregation process, are commendable. These changes ensure a more thorough and clean shutdown process for theFullNode, capturing any errors encountered during resource release. Well done on enhancing the reliability of the shutdown sequence.node/full_node_integration_test.go (2)
- 97-98: The simplification of context handling by using a single background context (
context.Background()) for both the aggregator and client nodes is a positive change for readability and maintainability. However, it's important to ensure that this change aligns with the intended test behavior, especially regarding asynchronous operations that might require cancellation or timeouts.- 101-105: Adding deferred node stops (
defer func() { for _, n := range nodes { assert.NoError(n.Stop()) } }()) is a good practice to ensure proper cleanup after test execution. This change helps prevent potential resource leaks and ensures that each node is stopped regardless of how the test exits (e.g., due to an assertion failure or panic). It's a significant improvement in test reliability and resource management.
tzdybal
removed
the
T:code-hygiene
Manav-Aggarwal
previously approved these changes
Feb 23, 2024
gupadhyaya
previously approved these changes
Feb 23, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
MSevey
requested changes
Feb 23, 2024
tzdybal
dismissed stale reviews from gupadhyaya and Manav-Aggarwal
via
f82160b
tzdybal
force-pushed
the
tzdybal/closing
branch
from
bc987c0
to
f82160b
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (8)
- block/block_sync.go (1 hunks)
- block/header_sync.go (1 hunks)
- block/manager.go (2 hunks)
- node/full.go (1 hunks)
- node/full_node_integration_test.go (1 hunks)
- store/store.go (1 hunks)
- store/store_test.go (1 hunks)
- store/types.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- block/block_sync.go
- block/header_sync.go
- node/full.go
- node/full_node_integration_test.go
- store/store.go
- store/store_test.go
- store/types.go
Manav-Aggarwal
previously approved these changes
Feb 24, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- block/manager.go (2 hunks)
- node/full_node_integration_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- block/manager.go
- node/full_node_integration_test.go
tzdybal
force-pushed
the
tzdybal/closing
branch
from
bf29762
to
cb0602f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- block/block_sync.go (1 hunks)
- block/header_sync.go (1 hunks)
- block/manager.go (2 hunks)
- node/full.go (1 hunks)
- node/full_node_integration_test.go (1 hunks)
- store/store.go (1 hunks)
- store/store_test.go (1 hunks)
- store/types.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- block/block_sync.go
- block/header_sync.go
- block/manager.go
- node/full.go
- node/full_node_integration_test.go
- store/store.go
- store/store_test.go
- store/types.go
gupadhyaya
reviewed
Feb 26, 2024
gupadhyaya
previously approved these changes
Feb 26, 2024
tzdybal
dismissed stale reviews from gupadhyaya and Manav-Aggarwal
via
c0b3caa
tzdybal
force-pushed
the
tzdybal/closing
branch
from
cb0602f
to
c0b3caa
Compare
`Close` method was introduced in `Store` interface and implemented in `DefaultSore`. `TestRestart` was updated to use `Close`. It needs to use on-disk KV because it's not possible close and re-open in-memory KV.
Additionally, this commit ensures that Store is closed by Full Node. The error handling in the stop methods of the Full Node, BlockSyncService, and HeaderSyncService has been refactored. The methods now join and handle the potential errors from closing their services in a more streamlined way. This change simplifies the error handling model and enhances code readability.
The shutdown sequence in the full node was rearranged to ensure proper closure of all services. Instead of immediately cancelling all node tasks and waiting for their termination, the update takes care of shutting down all sub services first before invoking cancellation and thread management wait. Furthermore, the clean-up process in the full node integration test was adjusted to delay node stoppage until after the state testing is done. This is to ensure that the store remains accessible for testing purposes.
The block manager error message is modified to reflect the failed submission attempt more accurately. Rather than stating the total number of blocks and the submitted amount, the error message now includes the number of remaining blocks after the submission attempt. This gives a clearer understanding of the submission state at the point of failure.
tzdybal
force-pushed
the
tzdybal/closing
branch
from
c0b3caa
to
8ced59d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- block/block_sync.go (1 hunks)
- block/header_sync.go (1 hunks)
- block/manager.go (2 hunks)
- node/full.go (1 hunks)
- node/full_node_integration_test.go (1 hunks)
- store/store.go (1 hunks)
- store/store_test.go (1 hunks)
- store/types.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- block/block_sync.go
- block/header_sync.go
- block/manager.go
- node/full.go
- node/full_node_integration_test.go
- store/store.go
- store/store_test.go
- store/types.go
Manav-Aggarwal
approved these changes
Feb 27, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- block/block_sync.go (1 hunks)
- block/header_sync.go (1 hunks)
- block/manager.go (2 hunks)
- node/full.go (1 hunks)
- node/full_node_integration_test.go (1 hunks)
- store/store.go (1 hunks)
- store/store_test.go (1 hunks)
- store/types.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- block/block_sync.go
- block/header_sync.go
- block/manager.go
- node/full.go
- node/full_node_integration_test.go
- store/store.go
- store/store_test.go
- store/types.go
gupadhyaya
approved these changes
Feb 27, 2024
MSevey
approved these changes
Feb 27, 2024
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.
Overview
Resolves #1552
Resolves #1553
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor