feat: enable write retry and nack pending writes on reconnect #443

alvarowolfx · 2024-04-22T19:16:57Z

This PR fix an issue that can happen when an error is returned by the service side and we trigger a reconnect, in some cases where a in-flight/pending write is still waiting for response can be left without any ack/nack.

Towards internal b/329875851

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…/nodejs-bigquery-storage into fix-ack-on-reconnect

src/managedwriter/stream_connection.ts

src/managedwriter/writer_client.ts

system-test/managed_writer_client_test.ts

yirutang · 2024-04-23T22:03:09Z

system-test/managed_writer_client_test.ts

+    it('should manage to send data in scenario where every 10 request drops the connection', async () => {
+      bqWriteClient.initialize();
+      const client = new WriterClient();
+      client.enableWriteRetries(true);


You should also be able to test the nonretried behavior.

src/managedwriter/stream_connection.ts

yirutang · 2024-04-26T20:07:59Z

src/managedwriter/stream_connection.ts

-    this.trace('data arrived', response);
-    const pw = this.getNextPendingWrite();
-    if (!pw) {
+    this.trace('data arrived', response, this._pendingWrites.length);


This is probably too much logging, it will log one line per request. If it is debug logging then it is fine.

those trace calls are only on DEBUG mode.

Is there a LOG mode? I think those reconnect errors need to be in LOG mode. Even for debug mode, per request logging might be too much, but I am less concerned.

This might be a place that could benefit from the logger I'm working on elsewhere. (Probably said that already, but specifically here...)

gonna be the first user of that new logger @feywind 🚀

yirutang · 2024-04-26T20:08:10Z

src/managedwriter/stream_connection.ts

-    const pw = this.getNextPendingWrite();
-    if (!pw) {
+    this.trace('data arrived', response, this._pendingWrites.length);
+    if (!this.hasPendingWrites()) {
      this.trace('data arrived with no pending write available', response);


src/managedwriter/stream_connection.ts

src/managedwriter/writer.ts

feywind

Nothing super important, I think...

src/managedwriter/stream_connection.ts

feywind · 2024-04-30T19:15:37Z

src/managedwriter/stream_connection.ts

-    this.trace('data arrived', response);
-    const pw = this.getNextPendingWrite();
-    if (!pw) {
+    this.trace('data arrived', response, this._pendingWrites.length);


This might be a place that could benefit from the logger I'm working on elsewhere. (Probably said that already, but specifically here...)

src/managedwriter/stream_connection.ts

src/managedwriter/writer.ts

system-test/managed_writer_client_test.ts

src/managedwriter/stream_connection.ts

leahecole · 2024-05-01T14:22:01Z

src/managedwriter/writer_client.ts

@@ -69,6 +74,10 @@ export class WriterClient {
      connectionList: [],
    };
    this._open = false;
+    this._retrySettings = {
+      enableWriteRetries: false,
+      maxRetryAttempts: 4,


Why these values? Can they be overridden?

they can be overwritten via the enableWriteRetries and setMaxRetryAttempts methods. The default values came from the Go ManagedWriter - https://github.com/googleapis/google-cloud-go/blob/5a8c88956ea11eaf3e8aa8cffadfca06bf58c51d/bigquery/storage/managedwriter/retry.go#L31

src/managedwriter/stream_connection.ts

alvarowolfx

answered some question and pushed updates to the PR cc @leahecole @feywind

src/managedwriter/stream_connection.ts

alvarowolfx · 2024-05-02T13:29:40Z

src/managedwriter/writer_client.ts

@@ -69,6 +74,10 @@ export class WriterClient {
      connectionList: [],
    };
    this._open = false;
+    this._retrySettings = {
+      enableWriteRetries: false,
+      maxRetryAttempts: 4,


they can be overwritten via the enableWriteRetries and setMaxRetryAttempts methods. The default values came from the Go ManagedWriter - https://github.com/googleapis/google-cloud-go/blob/5a8c88956ea11eaf3e8aa8cffadfca06bf58c51d/bigquery/storage/managedwriter/retry.go#L31

src/managedwriter/stream_connection.ts

fix: nack pending writes on reconnect

3cd69e9

alvarowolfx requested a review from a team as a code owner April 22, 2024 19:16

product-auto-label bot added size: m Pull request size is medium. api: bigquerystorage Issues related to the googleapis/nodejs-bigquery-storage API. labels Apr 22, 2024

fix: lint issues

7ba612c

alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 22, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 22, 2024

alvarowolfx and others added 3 commits April 22, 2024 15:31

fix: lint issues

70f6972

🦉 Updates from OwlBot post-processor

a751d77

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Merge branch 'fix-ack-on-reconnect' of https://github.com/alvarowolfx…

527ab79

…/nodejs-bigquery-storage into fix-ack-on-reconnect

yirutang reviewed Apr 22, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

yirutang reviewed Apr 23, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

feat: enable write retries on failed pending writes

4cfe085

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 23, 2024

fix: lint issue

474e237

shollyman reviewed Apr 23, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

yirutang reviewed Apr 23, 2024

View reviewed changes

src/managedwriter/writer_client.ts Show resolved Hide resolved

yirutang reviewed Apr 23, 2024

View reviewed changes

fix: only reconnect on server-side close event

50f2ae7

yirutang reviewed Apr 24, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

yirutang reviewed Apr 24, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

fix: only reconnect on close if there are pending writes

63ac809

yirutang reviewed Apr 24, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

alvarowolfx changed the title ~~fix: nack pending writes on reconnect~~ feat: enable write retry and nack pending writes on reconnect Apr 24, 2024

feat: resend on retryable error

b60db53

alvarowolfx added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Apr 25, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 25, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 25, 2024

yirutang reviewed Apr 26, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Show resolved Hide resolved

feat: log number of pending writes on reconnect

2f2e600

shollyman reviewed Apr 29, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

fix: reconnect trace msg

f35bc48

yirutang reviewed Apr 29, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

src/managedwriter/writer.ts Outdated Show resolved Hide resolved

alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 30, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 30, 2024

alvarowolfx requested a review from feywind April 30, 2024 15:02

fix: don't close conn on flush

62ff5dd

alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 30, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 30, 2024

fix: rename var/properties for clarity

a921b9f

alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 30, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 30, 2024

feywind approved these changes Apr 30, 2024

View reviewed changes

leahecole reviewed May 1, 2024

View reviewed changes

fix: address review comments

1e4e851

alvarowolfx requested a review from a team as a code owner May 2, 2024 13:38

alvarowolfx requested a review from agrawal-siddharth May 2, 2024 13:38

alvarowolfx commented May 2, 2024

View reviewed changes

leahecole reviewed May 2, 2024

View reviewed changes

src/managedwriter/stream_connection.ts Show resolved Hide resolved

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

src/managedwriter/stream_connection.ts Outdated Show resolved Hide resolved

alvarowolfx added 2 commits May 2, 2024 11:49

fix: return after nack pending write due to max retries

4c01180

fix: change connect error msg and code

de07922

alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label May 2, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 2, 2024

alvarowolfx merged commit ce4f88c into googleapis:main May 3, 2024
14 checks passed

release-please bot mentioned this pull request May 3, 2024

chore(main): release 4.6.0 #448

Merged

This was referenced May 6, 2024

How should errors be handled? #454

Closed

14 UNAVAILABLE: Request 'AppendRows' from role 'cloud-dataengine-globalrouting' throttled: Task is overloaded (memory-protection) go/tr-t #442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable write retry and nack pending writes on reconnect #443

feat: enable write retry and nack pending writes on reconnect #443

alvarowolfx commented Apr 22, 2024

yirutang Apr 23, 2024

yirutang Apr 26, 2024

alvarowolfx Apr 29, 2024

yirutang Apr 29, 2024

feywind Apr 30, 2024

alvarowolfx Apr 30, 2024

yirutang Apr 26, 2024

feywind left a comment

feywind Apr 30, 2024

leahecole May 1, 2024

alvarowolfx May 2, 2024

alvarowolfx left a comment

alvarowolfx May 2, 2024

feat: enable write retry and nack pending writes on reconnect #443

feat: enable write retry and nack pending writes on reconnect #443

Conversation

alvarowolfx commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feywind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarowolfx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment