Handle retry for syncLoop and watchLoop #863
Conversation
WalkthroughThe recent updates introduce an enumeration 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 Configration File (
|
hackerwins
force-pushed
the
retrable-connect-error
branch
from
872fb03 to
a35e640
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #863 +/- ##
==========================================
+ Coverage 80.89% 80.93% +0.04%
==========================================
Files 60 60
Lines 4600 4615 +15
Branches 936 937 +1
==========================================
+ Hits 3721 3735 +14
- Misses 610 612 +2
+ Partials 269 268 -1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/client/client.ts (8 hunks)
- src/yorkie.ts (2 hunks)
- test/integration/client_test.ts (3 hunks)
Additional comments not posted (11)
src/yorkie.ts (2)
28-28: Export Addition Approved.The addition of
ClientConditionto the exports is correct.
116-116: Export Modification Approved.The change to export
LogLevelas a regular export is correct.src/client/client.ts (7)
92-105: Enum Addition Approved.The
ClientConditionenum is correctly defined withSyncLoopandWatchLoopkeys.
176-176: Property Addition Approved.The
conditionsproperty is correctly added to hold boolean values forClientCondition.
198-201: Property Initialization Approved.The
conditionsproperty is correctly initialized in the constructor.
558-563: Method Addition Approved.The
getConditionmethod is correctly defined to return the condition of the client.
Line range hint
566-599: Method Addition Approved.The
runSyncLoopmethod is correctly defined to run the sync loop and handle retryable errors.
Line range hint
603-678: Method Addition Approved.The
runWatchLoopmethod is correctly defined to run the watch loop for the given document and handle retryable errors.
787-808: Method Addition Approved.The
isRetryableConnectErrormethod is correctly defined to identify retryable errors.test/integration/client_test.ts (2)
6-6: Import Addition Approved.The import of
ClientConditionfrom@yorkie-js-sdk/src/yorkieis correct.
768-802: Test Case Addition Approved.The test case for network failure retry logic is correctly defined.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- public/index.html (1 hunks)
- src/client/client.ts (8 hunks)
- src/yorkie.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/client/client.ts
- src/yorkie.ts
Additional comments not posted (1)
public/index.html (1)
319-319: Ensure the log level setting is appropriate for the production environment.Setting the log level to
Debugis useful for development and debugging but may expose sensitive information or generate excessive logs in production. Ensure this setting is configured appropriately based on the environment.
| @@ -763,4 +765,39 @@ describe.sequential('Client', function () { | |||
| await cli.deactivate(); | |||
| } | |||
| }); | |||
|
|
|||
| it('Should retry on network failure and eventually succeed', async ({ | |||
There was a problem hiding this comment.
@hackerwins
Would you please explain why this tc doesn't assert the condition of WatchLoop?
| err instanceof ConnectError && | ||
| err.code != ConnectErrorCode.Canceled | ||
| ) { | ||
| if (this.isRetryableConnectError(err)) { |
There was a problem hiding this comment.
@hackerwins
If the watchLoop stops due to this condition, when should it restart?
What this PR does / why we need it?
Handle retry for syncLoop and watchLoop based on ConnectError
This commit addresses the need to retry the
syncLoopandwatchLoopfunctions based on different error codes fromConnectError.The
Connectguide indicates that for error codes likeResourceExhaustedandUnavailable, retries should be attempted following their guidelines: Error Codes.Additionally, the
UnknownandCanceledare added separately as it typically occurs when the server is stopped unexpectedly.To enhance visibility into the status of
syncLoopandwatchLoop,Conditions has been added.Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit
New Features
ClientConditionfor better client state management.Improvements
Debugby default for better troubleshooting.