Skip to content
This repository has been archived by the owner on Apr 15, 2024. It is now read-only.

chore: better resources closing for orchestrator and relayer #638

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Dec 2, 2023

Overview

Closes #635

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

Summary by CodeRabbit

  • Refactor

    • Improved error handling and resource cleanup processes in the system's start-up sequence.
  • Bug Fixes

    • Adjusted error flow to ensure more robust operations and prevent potential issues during start-up and shutdown.
  • Documentation

    • Updated function documentation to reflect changes in error handling and resource management.
  • Style

    • Enhanced logging for better clarity on system operations during start-up and shutdown phases.

@rach-id rach-id added enhancement New feature or request orchestrator orchestrator related relayer relayer related labels Dec 2, 2023
@rach-id rach-id self-assigned this Dec 2, 2023
@rach-id rach-id requested a review from evan-forbes as a code owner December 2, 2023 13:34
Copy link
Contributor

coderabbitai bot commented Dec 2, 2023

Walkthrough

The 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

File Path Change Summary
cmd/blobstream/orchestrator/cmd.go Added error handling and reordering of stopFuncs
cmd/blobstream/relayer/cmd.go Moved defer statement, introduced storeStops, and changed ethClient initialization
orchestrator/orchestrator.go Imported goerrors, changed log levels, and modified event listener error handling

Assessment against linked issues

Objective Addressed Explanation
Fix the order of the stop funcs for orchestrator and relayer (#635) The changes in the cmd.go files for both orchestrator and relayer indicate a reordering of stop functions, which aligns with the objective to correct the sequence of resource shutdown.

Poem

In the code where resources flow,
A rabbit hopped to make it so.
Stop funcs aligned, errors at bay,
A seamless close, hip hip hooray! 🎉🐰


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between 95e2ae1 and 9410b67.
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 to stopFuncs twice, once after the common.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 to ethclient.Dial for ethClient assignment is consistent with the PR objective to improve resource management. Ensure that this change does not introduce any unintended side effects, especially since ethclient.Dial might behave differently in terms of connection handling compared to ethclient.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 returning nil instead of an error when receiving from signalChan or when the context is canceled is consistent with the PR's objective.

  • 221-226: The loop in EnqueueMissingEvents has been modified to return nil instead of an error when receiving from signalChan or when the context is canceled, which is consistent with the PR's objective.

cmd/blobstream/orchestrator/cmd.go Show resolved Hide resolved
cmd/blobstream/orchestrator/cmd.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2023

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (2496504) 25.91% compared to head (cc1ec8c) 25.55%.
Report is 2 commits behind head on main.

Files Patch % Lines
relayer/relayer.go 0.00% 44 Missing ⚠️
orchestrator/orchestrator.go 0.00% 7 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between 9410b67 and 1dbf2a3.
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 to stopFuncs after an error check is no longer valid as the code has been updated to append storeStops before the error check.

  • 118-121: The changes in hunk 1 correctly append the dht.Close() and storeStops to stopFuncs in the proper order, ensuring that resources are closed in the correct sequence as per the PR objective.

@rach-id rach-id linked an issue Dec 2, 2023 that may be closed by this pull request
@rach-id rach-id enabled auto-merge (squash) December 4, 2023 13:09
@rach-id rach-id merged commit 7a9c3d7 into celestiaorg:main Dec 4, 2023
17 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request orchestrator orchestrator related relayer relayer related
Projects
None yet
3 participants