feat: implement network error handling and recovery for stmt2 operations#112
feat: implement network error handling and recovery for stmt2 operations#112qevolg wants to merge 1 commit intofeat/main/failover4.0from
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the resilience of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces network error handling and recovery for stmt2 operations, a valuable addition for client resilience. The implementation correctly saves statement context and replays operations upon network failure, centered around a rebuildContext method. The inclusion of a comprehensive test suite using a proxy to simulate failures is commendable. I have a couple of suggestions in wsStmt2.ts to enhance code clarity and robustness.
| await this.doPrepare(this._savedSql!); | ||
| if (failedStep === StmtStep.PREPARE) return; | ||
|
|
||
| await this.doSendBindBytes(this._savedBindBytes!); |
There was a problem hiding this comment.
Using non-null assertion operators (!) on _savedSql and _savedBindBytes can be brittle. While the current logic ensures these are non-null, future changes might break this assumption and lead to runtime errors. For improved robustness and clearer error handling, it would be better to add explicit checks for undefined and throw a descriptive error if the properties are not set when expected.
| await this.doPrepare(this._savedSql!); | |
| if (failedStep === StmtStep.PREPARE) return; | |
| await this.doSendBindBytes(this._savedBindBytes!); | |
| if (this._savedSql === undefined) { | |
| throw new TaosResultError(ErrorCode.ERR_INVALID_PARAMS, "Internal error: _savedSql is undefined during rebuild."); | |
| } | |
| await this.doPrepare(this._savedSql); | |
| if (failedStep === StmtStep.PREPARE) return; | |
| if (this._savedBindBytes === undefined) { | |
| throw new TaosResultError(ErrorCode.ERR_INVALID_PARAMS, "Internal error: _savedBindBytes is undefined during rebuild."); | |
| } | |
| await this.doSendBindBytes(this._savedBindBytes); |
| if (failedStep === StmtStep.BIND) return; | ||
|
|
There was a problem hiding this comment.
The StmtStep.BIND enum member and this check appear to be unused. The rebuildContext method is never called with StmtStep.BIND. In exec(), failures in both doSendBindBytes and doExec trigger a rebuild with StmtStep.EXEC. To improve clarity and remove dead code, consider removing the BIND step from the StmtStep enum and this conditional block.
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.