From badea78c81757ae714c3aa13b608dea634c00e42 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 12 Jun 2024 15:58:53 -0700 Subject: [PATCH 1/6] feat(Bigtable): add bigtable-attempt header --- .../BigtableDataOperationException.php | 2 +- Bigtable/src/ResumableStream.php | 16 +++++----- Bigtable/tests/Unit/SmartRetriesTest.php | 29 ++++++++++++++++++- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/Bigtable/src/Exception/BigtableDataOperationException.php b/Bigtable/src/Exception/BigtableDataOperationException.php index bb717ac9c7ee..960f30caa8c3 100644 --- a/Bigtable/src/Exception/BigtableDataOperationException.php +++ b/Bigtable/src/Exception/BigtableDataOperationException.php @@ -41,7 +41,7 @@ public function __construct( ) { $this->metadata = $metadata; - parent::__construct($message, $code); + parent::__construct($message, $code ?? 0); } /** diff --git a/Bigtable/src/ResumableStream.php b/Bigtable/src/ResumableStream.php index 597a8fa391a0..81abced76dcd 100644 --- a/Bigtable/src/ResumableStream.php +++ b/Bigtable/src/ResumableStream.php @@ -126,19 +126,22 @@ public function __construct( */ public function readAll() { - $tries = 0; - $argumentFunction = $this->argumentFunction; - $retryFunction = $this->retryFunction; + $attempt = 0; do { $ex = null; - list($this->request, $this->callOptions) = $argumentFunction($this->request, $this->callOptions); + list($this->request, $this->callOptions) = + ($this->argumentFunction)($this->request, $this->callOptions); $completed = $this->pluck('requestCompleted', $this->callOptions, false); if ($completed !== true) { + // Send in "bigtable-attempt" header on each request + $headers = $this->callOptions['headers'] ?? []; + $headers['bigtable-attempt'] = ++$attempt; + $stream = call_user_func_array( [$this->gapicClient, $this->method], - [$this->request, $this->callOptions] + [$this->request, ['headers' => $headers] + $this->callOptions] ); try { @@ -148,8 +151,7 @@ public function readAll() } catch (\Exception $ex) { } } - $tries++; - } while ((!$this->retryFunction || $retryFunction($ex)) && $tries <= $this->retries); + } while ((!$this->retryFunction || ($this->retryFunction)($ex)) && $attempt <= $this->retries); if ($ex !== null) { throw $ex; } diff --git a/Bigtable/tests/Unit/SmartRetriesTest.php b/Bigtable/tests/Unit/SmartRetriesTest.php index f6c52136da17..8a0c92e18a3a 100644 --- a/Bigtable/tests/Unit/SmartRetriesTest.php +++ b/Bigtable/tests/Unit/SmartRetriesTest.php @@ -136,6 +136,33 @@ public function testReadRowsShouldRetryForProvidedAttempts() $iterator->getIterator()->current(); } + public function testReadRowsContainsAttempsHeader() + { + $attempt = 1; + $expectedArgs = $this->options; + $retryingApiException = $this->retryingApiException; + $phpunit = $this; + $this->serverStream->readAll() + ->shouldBeCalledTimes(2) + ->will(function () use (&$attempt, $retryingApiException) { + // throw a retriable exception on the first call + return 1 === $attempt++ ? throw $retryingApiException : []; + }); + $this->bigtableClient->readRows( + Argument::type(ReadRowsRequest::class), + Argument::that(function ($callOptions) use (&$attempt) { + $this->assertEquals($attempt, $callOptions['headers']['bigtable-attempt'] ?? null); + return true; + }) + )->shouldBeCalledTimes(2) + ->willReturn( + $this->serverStream->reveal() + ); + + $iterator = $this->table->readRows(); + $iterator->getIterator()->current(); + } + public function testReadRowsPartialSuccess() { $expectedArgs = $this->options; @@ -186,7 +213,7 @@ public function testReadRowsWithRowsLimit() $this->generateRowsResponse(3, 4) ) ); - + $allowedRowsLimit = ['5' => 1, '3' => 1]; $this->bigtableClient->readRows( Argument::that(function ($request) use (&$allowedRowsLimit) { From 45dcc81dc64932e5bd22411f060832b0ec4ca247 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 12 Jun 2024 17:51:06 -0700 Subject: [PATCH 2/6] ensure metadata value is array --- Bigtable/src/ResumableStream.php | 2 +- Bigtable/tests/Unit/SmartRetriesTest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Bigtable/src/ResumableStream.php b/Bigtable/src/ResumableStream.php index 81abced76dcd..ebc9dd76b589 100644 --- a/Bigtable/src/ResumableStream.php +++ b/Bigtable/src/ResumableStream.php @@ -137,7 +137,7 @@ public function readAll() if ($completed !== true) { // Send in "bigtable-attempt" header on each request $headers = $this->callOptions['headers'] ?? []; - $headers['bigtable-attempt'] = ++$attempt; + $headers['bigtable-attempt'] = [++$attempt]; $stream = call_user_func_array( [$this->gapicClient, $this->method], diff --git a/Bigtable/tests/Unit/SmartRetriesTest.php b/Bigtable/tests/Unit/SmartRetriesTest.php index 8a0c92e18a3a..059dc07b1a1f 100644 --- a/Bigtable/tests/Unit/SmartRetriesTest.php +++ b/Bigtable/tests/Unit/SmartRetriesTest.php @@ -136,7 +136,7 @@ public function testReadRowsShouldRetryForProvidedAttempts() $iterator->getIterator()->current(); } - public function testReadRowsContainsAttempsHeader() + public function testReadRowsContainsAttemptHeader() { $attempt = 1; $expectedArgs = $this->options; @@ -151,7 +151,7 @@ public function testReadRowsContainsAttempsHeader() $this->bigtableClient->readRows( Argument::type(ReadRowsRequest::class), Argument::that(function ($callOptions) use (&$attempt) { - $this->assertEquals($attempt, $callOptions['headers']['bigtable-attempt'] ?? null); + $this->assertEquals($attempt, $callOptions['headers']['bigtable-attempt'][0] ?? null); return true; }) )->shouldBeCalledTimes(2) From 1ef5c85edf2819097c3f38ddaedf9afbec2d9b3a Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Thu, 13 Jun 2024 00:58:17 +0000 Subject: [PATCH 3/6] ensure header is string --- Bigtable/src/ResumableStream.php | 2 +- Bigtable/tests/Unit/SmartRetriesTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Bigtable/src/ResumableStream.php b/Bigtable/src/ResumableStream.php index ebc9dd76b589..7327644d1c63 100644 --- a/Bigtable/src/ResumableStream.php +++ b/Bigtable/src/ResumableStream.php @@ -137,7 +137,7 @@ public function readAll() if ($completed !== true) { // Send in "bigtable-attempt" header on each request $headers = $this->callOptions['headers'] ?? []; - $headers['bigtable-attempt'] = [++$attempt]; + $headers['bigtable-attempt'] = [(string) ++$attempt]; $stream = call_user_func_array( [$this->gapicClient, $this->method], diff --git a/Bigtable/tests/Unit/SmartRetriesTest.php b/Bigtable/tests/Unit/SmartRetriesTest.php index 059dc07b1a1f..d3ae3d3e51ac 100644 --- a/Bigtable/tests/Unit/SmartRetriesTest.php +++ b/Bigtable/tests/Unit/SmartRetriesTest.php @@ -151,7 +151,7 @@ public function testReadRowsContainsAttemptHeader() $this->bigtableClient->readRows( Argument::type(ReadRowsRequest::class), Argument::that(function ($callOptions) use (&$attempt) { - $this->assertEquals($attempt, $callOptions['headers']['bigtable-attempt'][0] ?? null); + $this->assertSame((string) $attempt, $callOptions['headers']['bigtable-attempt'][0] ?? null); return true; }) )->shouldBeCalledTimes(2) From 51f360cd1dbdb59f0592d4a0087ff145680c0966 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Mon, 17 Jun 2024 23:28:10 -0700 Subject: [PATCH 4/6] update to only send attempt header on retry --- Bigtable/src/ResumableStream.php | 5 ++++- Bigtable/tests/Unit/SmartRetriesTest.php | 12 +++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/Bigtable/src/ResumableStream.php b/Bigtable/src/ResumableStream.php index 7327644d1c63..6ac7daaab60d 100644 --- a/Bigtable/src/ResumableStream.php +++ b/Bigtable/src/ResumableStream.php @@ -137,7 +137,10 @@ public function readAll() if ($completed !== true) { // Send in "bigtable-attempt" header on each request $headers = $this->callOptions['headers'] ?? []; - $headers['bigtable-attempt'] = [(string) ++$attempt]; + if ($attempt > 0) { + $headers['bigtable-attempt'] = [(string) $attempt]; + } + $attempt++; $stream = call_user_func_array( [$this->gapicClient, $this->method], diff --git a/Bigtable/tests/Unit/SmartRetriesTest.php b/Bigtable/tests/Unit/SmartRetriesTest.php index d3ae3d3e51ac..265cb931b87e 100644 --- a/Bigtable/tests/Unit/SmartRetriesTest.php +++ b/Bigtable/tests/Unit/SmartRetriesTest.php @@ -138,7 +138,7 @@ public function testReadRowsShouldRetryForProvidedAttempts() public function testReadRowsContainsAttemptHeader() { - $attempt = 1; + $attempt = 0; $expectedArgs = $this->options; $retryingApiException = $this->retryingApiException; $phpunit = $this; @@ -146,12 +146,18 @@ public function testReadRowsContainsAttemptHeader() ->shouldBeCalledTimes(2) ->will(function () use (&$attempt, $retryingApiException) { // throw a retriable exception on the first call - return 1 === $attempt++ ? throw $retryingApiException : []; + return 0 === $attempt++ ? throw $retryingApiException : []; }); $this->bigtableClient->readRows( Argument::type(ReadRowsRequest::class), Argument::that(function ($callOptions) use (&$attempt) { - $this->assertSame((string) $attempt, $callOptions['headers']['bigtable-attempt'][0] ?? null); + $attemptHeader = $callOptions['headers']['bigtable-attempt'][0] ?? null; + if ($attempt === 0) { + $this->assertNull($attemptHeader); + } else { + $this->assertSame((string) $attempt, $attemptHeader); + } + return true; }) )->shouldBeCalledTimes(2) From 62fe4193c715c1acc45e26d05ef1644072c26509 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Tue, 18 Jun 2024 10:16:38 -0700 Subject: [PATCH 5/6] Update Bigtable/tests/Unit/SmartRetriesTest.php --- Bigtable/tests/Unit/SmartRetriesTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/Bigtable/tests/Unit/SmartRetriesTest.php b/Bigtable/tests/Unit/SmartRetriesTest.php index 265cb931b87e..c29accd4060a 100644 --- a/Bigtable/tests/Unit/SmartRetriesTest.php +++ b/Bigtable/tests/Unit/SmartRetriesTest.php @@ -141,7 +141,6 @@ public function testReadRowsContainsAttemptHeader() $attempt = 0; $expectedArgs = $this->options; $retryingApiException = $this->retryingApiException; - $phpunit = $this; $this->serverStream->readAll() ->shouldBeCalledTimes(2) ->will(function () use (&$attempt, $retryingApiException) { From 5aab453c4e3bd6720bdffab608ead5ce7250a7ff Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 19 Jun 2024 12:40:52 -0700 Subject: [PATCH 6/6] Update Bigtable/src/ResumableStream.php --- Bigtable/src/ResumableStream.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Bigtable/src/ResumableStream.php b/Bigtable/src/ResumableStream.php index 6ac7daaab60d..fb344157a4b5 100644 --- a/Bigtable/src/ResumableStream.php +++ b/Bigtable/src/ResumableStream.php @@ -135,7 +135,7 @@ public function readAll() $completed = $this->pluck('requestCompleted', $this->callOptions, false); if ($completed !== true) { - // Send in "bigtable-attempt" header on each request + // Send in "bigtable-attempt" header on retry request $headers = $this->callOptions['headers'] ?? []; if ($attempt > 0) { $headers['bigtable-attempt'] = [(string) $attempt];