fix(readability): adds some notes for some problems we experienced #35
Conversation
WalkthroughThe recent changes to the Changes
Poem
Warning Review ran into problemsProblems (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- relayer/processor/path_processor.go (1 hunks)
- relayer/processor/path_processor_internal.go (2 hunks)
Additional comments not posted (1)
relayer/processor/path_processor_internal.go (1)
1350-1356: Clarify the handling of delayed acknowledgments.#!/bin/bash # Description: Verify if there are any additional mechanisms or logs that handle delayed acknowledgments. # Test: Search for logs or error handling related to delayed acknowledgments. Expect: Relevant code handling or logs. rg --type go 'delayedack' 'relayer/processor/'
| /* | ||
| NOTE: it is possible that there are still outstanding broadcasts | ||
| This cancel will cancel them | ||
| In the future, we may want to wait for them to finish (<-pp.pathEnd1.finishedProcessing etc) | ||
| */ |
There was a problem hiding this comment.
The addition of the conditional block to handle broadcast termination is a good enhancement for managing resources and ensuring clean termination. However, the comment about future enhancements could be formalized into a TODO to ensure it is tracked properly.
+ // TODO: In the future, consider waiting for broadcasts to finish before cancellation.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /* | |
| NOTE: it is possible that there are still outstanding broadcasts | |
| This cancel will cancel them | |
| In the future, we may want to wait for them to finish (<-pp.pathEnd1.finishedProcessing etc) | |
| */ | |
| /* | |
| NOTE: it is possible that there are still outstanding broadcasts | |
| This cancel will cancel them | |
| In the future, we may want to wait for them to finish (<-pp.pathEnd1.finishedProcessing etc) | |
| */ | |
| // TODO: In the future, consider waiting for broadcasts to finish before cancellation. |
| /* | ||
| It's possible that an acknowledgement event was not yet published on the dst chain | ||
| */ |
There was a problem hiding this comment.
Enhance monitoring for skipped packets during flush operations.
+ pp.log.Debug("Skipped packets during flush", zap.Any("skipped_packets", skipped))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /* | |
| It's possible that an acknowledgement event was not yet published on the dst chain | |
| */ | |
| /* | |
| It's possible that an acknowledgement event was not yet published on the dst chain | |
| */ | |
| + pp.log.Debug("Skipped packets during flush", zap.Any("skipped_packets", skipped)) |
Consider improving the error handling for unacknowledged packets.
- return err
+ if errors.Is(err, provider.ErrAckNotPublished) {
+ dst.log.Warn("Acknowledgement not yet published", zap.Uint64("sequence", seq))
+ return nil // Consider retrying later or other recovery mechanisms
+ } else {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /* | |
| It's possible that an acknowledgement event was not yet published on the dst chain | |
| */ | |
| /* | |
| It's possible that an acknowledgement event was not yet published on the dst chain | |
| */ | |
| if errors.Is(err, provider.ErrAckNotPublished) { | |
| dst.log.Warn("Acknowledgement not yet published", zap.Uint64("sequence", seq)) | |
| return nil // Consider retrying later or other recovery mechanisms | |
| } else { | |
| return err | |
| } |
Summary by CodeRabbit