-
Notifications
You must be signed in to change notification settings - Fork 16
chore: better resources closing for orchestrator and relayer #638
Conversation
WalkthroughThe changes involve reorganizing the order and handling of stop functions and error management in the orchestrator and relayer components. The primary goal is to ensure that resources are closed in the correct sequence to prevent errors, specifically ensuring that the DHT is closed before the data store. Changes
Assessment against linked issues
Poem
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: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- cmd/blobstream/orchestrator/cmd.go (2 hunks)
- cmd/blobstream/relayer/cmd.go (2 hunks)
- orchestrator/orchestrator.go (5 hunks)
Additional comments: 7
cmd/blobstream/relayer/cmd.go (2)
172-173: It appears that
storeStops
is appended tostopFuncs
twice, once after thecommon.OpenStore
call and again after the DHT is created. Please verify if this is intentional to avoid potential duplicate stop function calls, which could lead to unexpected behavior.180-180: The change from
ethclient.NewClient
toethclient.Dial
forethClient
assignment is consistent with the PR objective to improve resource management. Ensure that this change does not introduce any unintended side effects, especially sinceethclient.Dial
might behave differently in terms of connection handling compared toethclient.NewClient
.orchestrator/orchestrator.go (5)
3-8: The addition of the
goerrors
package import aligns with the PR's objective to enhance error handling.83-86: The change in log level from error to info for logging "stopping listening to new attestations" is consistent with the PR's objective.
95-98: The change in log level from error to info for logging "stopping processing attestations" is consistent with the PR's objective.
138-146: The modification in the
StartNewEventsListener
function to handle context cancellation by returningnil
instead of an error when receiving fromsignalChan
or when the context is canceled is consistent with the PR's objective.221-226: The loop in
EnqueueMissingEvents
has been modified to returnnil
instead of an error when receiving fromsignalChan
or when the context is canceled, which is consistent with the PR's objective.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #638 +/- ##
==========================================
- Coverage 25.91% 25.55% -0.37%
==========================================
Files 29 29
Lines 3044 3088 +44
==========================================
Hits 789 789
- Misses 2160 2204 +44
Partials 95 95 ☔ View full report in Codecov by Sentry. |
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)
- cmd/blobstream/orchestrator/cmd.go (2 hunks)
Additional comments: 2
cmd/blobstream/orchestrator/cmd.go (2)
100-101: The existing comment about appending
storeStops
tostopFuncs
after an error check is no longer valid as the code has been updated to appendstoreStops
before the error check.118-121: The changes in hunk 1 correctly append the
dht.Close()
andstoreStops
tostopFuncs
in the proper order, ensuring that resources are closed in the correct sequence as per the PR objective.
Overview
Closes #635
Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation
Style