-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor(network): call app.Close()
on network cleanup
#18249
Conversation
WalkthroughThe changes primarily focus on enhancing the Changes
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- testutil/network/network.go (3 hunks)
- testutil/network/util.go (3 hunks)
Additional comments: 5
testutil/network/util.go (3)
11-11: The import
"cosmossdk.io/log"
has been added. Ensure that this package is being used in the code and that it is compatible with the existing logging system.47-47: The
app
object is now being assigned to theapp
field of theval
object. This change seems to be part of the effort to improve resource management by ensuring that the ABCI application associated with each validator is properly stopped during network cleanup. However, ensure that this does not introduce any unintended side effects, especially in the context of concurrent operations.114-114: The
logger.With
function calls in theservergrpc.StartGRPCServer
andapi.New
functions have been updated to use thelog.ModuleKey
constant. This change improves consistency in log messages. However, ensure that thelog.ModuleKey
constant is defined and correctly set to"module"
.testutil/network/network.go (2)
295-298: The addition of the
app
field to theValidator
struct is a good move. This allows for better resource management by ensuring that the ABCI application associated with each validator is properly stopped during network cleanup. However, please ensure that this new field is properly initialized and used throughout the codebase.640-643: The change from using a separate error declaration to an inline error check in the
startInProcess
function call is a good one. It makes the code more concise and readable.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- testutil/network/util.go (3 hunks)
Files skipped from review due to trivial changes (1)
- testutil/network/util.go
(cherry picked from commit 139a29e)
(cherry picked from commit 139a29e) # Conflicts: # testutil/network/network.go # testutil/network/util.go
…8249) (#18250) Co-authored-by: Julien Robert <[email protected]>
…8249) (#18251) Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #18151
app.Close()
on network cleanupAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit