-
Notifications
You must be signed in to change notification settings - Fork 115
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
skip liquidation task loop if last committed block height is the same #2124
Conversation
WalkthroughThe recent changes enhance error handling in tests and optimize the Changes
Poem
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 (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/daemons/liquidation/client/grpc_helper_test.go (1 hunks)
- protocol/daemons/liquidation/client/sub_task_runner.go (2 hunks)
Additional comments not posted (4)
protocol/daemons/liquidation/client/sub_task_runner.go (3)
29-31
: Addition oflastLoopBlockHeight
field is appropriate.The new field
lastLoopBlockHeight
is necessary for tracking the last processed block height, which supports the optimization logic.
55-61
: Conditional check for skipping the task loop is correctly implemented.The logic to skip the task loop when
lastCommittedBlockHeight
equalss.lastLoopBlockHeight
is correctly implemented, preventing unnecessary processing.
63-63
: UpdatinglastLoopBlockHeight
is correctly placed.The update of
s.lastLoopBlockHeight
tolastCommittedBlockHeight
ensures that the state is accurately maintained for the next iteration.protocol/daemons/liquidation/client/grpc_helper_test.go (1)
593-597
: Enhanced error handling logic is correctly implemented.The conditional structure accurately differentiates between expected and unexpected errors, improving the specificity of error assertions.
return nil | ||
} | ||
|
||
s.lastLoopBlockHeight = lastCommittedBlockHeight |
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.
Playing devil's advocate: are we sure this is thread-safe? Could two liquidation daemon loop overlap? Should we add a sync.Mutex
similar to LiquidationServer
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.
no daemon loops don't overlap since we are not spawning new go routines
for {
select {
case <-ticker.C:
if err := s.RunLiquidationDaemonTaskLoop(
ctx,
client,
flags.Liquidation,
); err != nil {
// TODO(DEC-947): Move daemon shutdown to application.
client.logger.Error("Liquidations daemon returned error", "error", err)
client.ReportFailure(err)
} else {
client.ReportSuccess()
}
case <-stop:
return
}
}
@@ -50,6 +52,16 @@ func (s *SubTaskRunnerImpl) RunLiquidationDaemonTaskLoop( | |||
return err | |||
} | |||
|
|||
if lastCommittedBlockHeight == s.lastLoopBlockHeight { |
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.
IIUC if application restarts, lastLoopBlockHeight
is by default 0
and we always continue below logic. If so could we document?
return nil | ||
} | ||
|
||
s.lastLoopBlockHeight = lastCommittedBlockHeight |
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.
Nit: I wonder if here's the best place to assign s.lastLoopBlockHeight = lastCommittedBlockHeight
since we haven't sent any current block info to application yet. Wdyt think about defer
ing the statement s.lastLoopBlockHeight = lastCommittedBlockHeight
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.
good question. thought about it a little bit more
putting it here strictly guarantees that loop doesn't run for the same block height, regardless of errors. If we defer the statement to the end (e.g. after SendLiquidatableSubaccountIds
), theoretically it's possible that SendLiquidatableSubaccountIds
only partially succeeds when first few requests goes through and later requests fail. In this case, we would be running into the same issue. I think might be slightly better here for correctness?
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.
defer
ing guarantees that the s.lastLoopBlockHeight = lastCommittedBlockHeight
is always run before return, even if SendLiquidatableSubaccountIds
fails right? Trying to understand when we'd send SendLiquidatableSubaccountIds
again for the same block.
No strong preference though, the current implementation also works. Up to you. Approved to unblock
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.
ohh you mean defer
and update block height regardless of error? I misunderstood
yeah I think it's basically the same - let's keep it as is to unstuck the testnet first
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/daemons/liquidation/client/sub_task_runner.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/daemons/liquidation/client/sub_task_runner.go
@Mergifyio backport release/protocol/v5.2.x |
✅ Backports have been created
|
… (backport #2124) (#2125) Co-authored-by: jayy04 <[email protected]>
@Mergifyio backport release/protocol/v6.x |
✅ Backports have been created
|
… (backport #2124) (#2129) Co-authored-by: jayy04 <[email protected]>
… (backport #2124) (#2125) Co-authored-by: jayy04 <[email protected]>
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.Summary by CodeRabbit
Bug Fixes
New Features