Conversation
… request handling
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 robustness and availability of the TDengine Node.js WebSocket Connector. It introduces comprehensive multi-address support, enabling the connector to distribute load across multiple server instances and automatically failover to healthy alternatives when connections are lost. This includes sophisticated retry mechanisms for in-flight requests, improved connection pool management based on DSNs, and enhanced concurrency control, ensuring a more resilient and transparent experience for applications interacting with TDengine. 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. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #109 +/- ##
==========================================
+ Coverage 80.21% 81.10% +0.88%
==========================================
Files 30 33 +3
Lines 2487 3271 +784
Branches 437 589 +152
==========================================
+ Hits 1995 2653 +658
- Misses 378 473 +95
- Partials 114 145 +31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nd WebSocketConnector
…ame in schemaless test
…cross the codebase
…ethod in WebSocketConnector
…ect method in WebSocketConnector
…on check and renaming variables
…ordered tracking and replay functionality
…subscription, failover, and token handling - Implement tests for normal connection and error scenarios in `tmq.test.ts` - Add tests for configuration handling in `tmq.config.test.ts` - Introduce failover tests to ensure resilience in `tmq.failover.test.ts` - Create a dedicated test suite for cloud interactions in `tmq.cloud.test.ts` - Validate token-based authentication and URL token handling in `tmq.test.ts` - Ensure proper cleanup of test databases and topics after tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
nodejs/src/client/wsConnector.ts:267
WsEventCallback.handleEventCallback()is async and can throw (e.g. when no callback is registered). In_onmessage, it is invoked withoutawait/.catch, which can lead to unhandled promise rejections at runtime. Consider prefixing withvoid ...handleEventCallback(...).catch(err => logger.error(...))(or otherwise handling the promise) to avoid process-level unhandled rejection behavior.
private _onmessage(event: any) {
let data = event.data;
logger.debug("wsClient._onMessage()====" + Object.prototype.toString.call(data));
if (Object.prototype.toString.call(data) === "[object ArrayBuffer]") {
let id = new DataView(data, 26, 8).getBigUint64(0, true);
WsEventCallback.instance().handleEventCallback(
{ id: id, action: "", req_id: BigInt(0) },
OnMessageType.MESSAGE_TYPE_ARRAYBUFFER,
data
);
} else if (Object.prototype.toString.call(data) === "[object String]") {
let msg = JSON.parse(data);
logger.debug("[_onmessage.stringType]==>:" + data);
WsEventCallback.instance().handleEventCallback(
{ id: BigInt(0), action: msg.action, req_id: msg.req_id },
OnMessageType.MESSAGE_TYPE_STRING,
msg
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… corresponding test
…nnector for better organization
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
nodejs/src/client/wsConnector.ts:289
WsEventCallback.handleEventCallback()is async, but_onmessagecalls it without awaiting or attaching a.catch(). IfhandleEventCallbackthrows/rejects (e.g., callback timed out/was unregistered), this becomes an unhandled promise rejection. Wrap these calls withvoid ... .catch(err => logger.error(...))(or similar) to avoid process-level unhandled rejection behavior.
private _onmessage(event: any) {
let data = event.data;
logger.debug("wsClient._onMessage()====" + Object.prototype.toString.call(data));
if (Object.prototype.toString.call(data) === "[object ArrayBuffer]") {
let id = new DataView(data, 26, 8).getBigUint64(0, true);
WsEventCallback.instance().handleEventCallback(
{ id: id, action: "", req_id: BigInt(0) },
OnMessageType.MESSAGE_TYPE_ARRAYBUFFER,
data
);
} else if (Object.prototype.toString.call(data) === "[object String]") {
let msg = JSON.parse(data);
logger.debug("[_onmessage.stringType]==>:" + data);
WsEventCallback.instance().handleEventCallback(
{ id: BigInt(0), action: msg.action, req_id: msg.req_id },
OnMessageType.MESSAGE_TYPE_STRING,
msg
);
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 54 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- nodejs/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- nodejs/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
nodejs/test/tmq/tmq.test.ts:300
expect([104]).toContain(e.code)is equivalent to a direct equality check and is harder to read. Since only one code is expected now, preferexpect(e.code).toBe(104)(or keep multiple codes if the error can legitimately vary).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
• ## Review Findings
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- nodejs/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zitsen
left a comment
There was a problem hiding this comment.
Code Review Summary
All 8 CI checks pass. Coverage increased from 80.21% → 81.10%.
Previously Reported Issues — All Resolved
-
[P1] Pool key auth collision — Fixed. Auth fields are now JSON-serialized + SHA-256 hashed in
buildAuthScope(), eliminating collisions likeuser='a:b',pwd='c'vsuser='a',pwd='b:c'. -
[P2] DSN parsing
@in query params — Fixed. The parser now isolates the authority section before/and?prior to looking for@, sobearer_token=a@bis handled correctly. Test coverage confirms. -
[P2] Pool key missing retry params — Intentional design. Retry parameters are excluded from the pool key; instead,
refreshRetryConfig()updates retry settings when reusing pooled connections. This avoids unnecessary pool fragmentation. -
[P2] Session recovery default DB — Fixed.
normalizeConnectedDatabase()returnsinformation_schemawhen no explicit DB is provided for SQL paths.recoverSqlSessionContext()uses this during reconnect.
Additional Verification
- Reconnect lock:
_reconnectLockcorrectly deduplicates concurrent reconnect triggers. - Inflight tracking: Requests are properly removed on resolve/reject; all inflight requests are failed and cleared when reconnect fails.
- Address selection: Least-connected strategy with proper increment/decrement lifecycle.
- No memory leaks: Callbacks auto-unregister, inflight store clears on close/failure.
LGTM ✅
Description
feat: impl load balancing and failover
Issue(s)
Checklist
Please check the items in the checklist if applicable.