Added retry function #63

ilyakharev · 2023-06-04T19:47:54Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Pull request type

Please check the type of change your PR introduces:

Issue numbers: #61, #62

rekby · 2023-06-07T08:43:08Z

src/Exceptions/FastBackoffRetryableException.php

+
+namespace YdbPlatform\Ydb\Exceptions;
+
+class FastBackoffRetryableException extends RetryableException


No need Fast/Slow backoff in exceptions hierarchy.
Because it will difficult to change backoff type in future.

It may be property of exception, or (better) some function, which detect retry delay

rekby · 2023-06-07T08:48:05Z

src/Traits/RequestTrait.php

+     * @return mixed|void|null
+     * @throws \Exception
+     */
+    public function makeRequest(Closure $closure, bool $retryable){


Is the function need?

rekby · 2023-06-07T08:48:18Z

src/Traits/RetryTrait.php

+
+namespace YdbPlatform\Ydb\Traits;
+
+trait RetryTrait


Is empty trait need?

rekby · 2023-06-07T08:49:01Z

src/Session.php

-            $result = $closure($this);
-            $this->commitTransaction();
-            return $result;
+            return $this->makeRequest(function ($session) use ($closure) {


no need the changes, retry only in retry function

rekby · 2023-06-07T08:50:13Z

src/Table.php

@@ -421,4 +424,54 @@ protected function streamRequest($method, array $data = [])
        return $this->doStreamRequest('Table', $method, $data);
    }

+    /**
+     * @param Closure $closure пользовательская функция, которую должен выполнить sdk.
+     * Функция принимает новую сессию


Need example:

retrySession(function($session){...})

rekby · 2023-06-07T08:52:42Z

src/Table.php

+     * Функция принимает новую сессию
+     * @return mixed результат выполнения функции
+     */
+    public function retrySession(Closure $closure, int $waitTime = 100, bool $independent = false){


What unit of waitTime?
independent -> idempotent

rekby · 2023-06-07T08:53:45Z

src/Table.php

+        $waitBefore = microtime(true)+$waitTime/1000;
+        $requestCount = 0;
+        $session = $this->session();
+        while ($waitBefore>microtime(true))


it will good to reuse $Ydb->retry in the session instead of copy paste code.

rekby · 2023-06-07T08:55:17Z

src/Ydb.php

        return $this->scripting;
    }

+    public function retry(Closure $closure, int $waitTime = 100, bool $independent = false){
+        $waitBefore = microtime(true)+$waitTime*1e-3;


$waitTime*1e-3 - difficult to read and parse by human:

science form is unusual in programs

is that "($waitTime1e)-3" or "$waitTime(1e-3)"?

rekby · 2023-06-07T08:56:14Z

tests/classes/FakeCredentials.php

+use YdbPlatform\Ydb\Auth\Auth;
+use YdbPlatform\Ydb\Auth\TokenInfo;
+
+class FakeCredentials extends Auth {


Why the file need in the PR?

rekby · 2023-06-07T09:00:03Z

examples/app/Commands/CreateTableCommand.php

+            $output->writeln('Table ' . $table_name . ' has been created.');
+
+            $session->transaction(function($session) use ($table_name, $columns) {
+                $session->query('upsert into `' . $table_name . '` (`' . implode('`, `', array_keys($columns)) . '`) values (' . implode('), (', $this->getData()) . ');');


insert data in text of query id bad example.

I don't see is it your changes or original code.

rekby · 2023-06-14T07:50:40Z

src/Table.php

+                $sessionId = $session->id();
+                $session->beginTransaction();


What about use transaction function?

rekby · 2023-06-23T07:41:46Z

examples/app/Commands/BasicExampleCommand.php

+                    ->primaryKey(['series_id', 'season_id'])
+            );
+
+            $this->print('Table `seasons` has been created.');


here and in other places:

print should be out of retry

rekby · 2023-06-23T07:45:54Z

examples/app/Commands/CreateTableCommand.php


-        $output->writeln('Table ' . $table_name . ' has been created.');
+            $output->writeln('Table ' . $table_name . ' has been created.');


$output->writeln mat be called multiply times if transaction will retry.

need separate create tables from requests to the tables.

rekby · 2023-06-23T07:50:26Z

examples/app/Commands/ListDirectoryCommand.php

-        {
-            $output->writeln('Empty directory');
-        }
+        $ydb->retry(function (Ydb $ydb) use ($output, $dirname) {


retry should include only logic for work with ydb, for prevent bad examples.

prepare
retry (func (){work with database})
show/return result, render, etc.

rekby · 2023-06-23T07:54:43Z

src/Exceptions/Ydb/AbortedException.php

+
+class AbortedException extends RetryableException
+{
+    public function isFastBackoff(): bool


Is the function need?

rekby · 2023-06-23T07:56:20Z

src/Retry/Retry.php

+    }
+
+    /**
+     * @throws NonRetryableException


may be simpler - ydb/Exception?

rekby · 2023-06-23T08:01:49Z

src/Traits/RequestTrait.php

@@ -158,9 +151,13 @@ protected function doStreamRequest($service, $method, $data = [])
     */
    protected function checkStatus($service, $method, $status)


better rename to checkGrpcStatus, for prevent mistakes with check server status code.

rekby · 2023-06-23T08:03:47Z

examples/app/Commands/BasicExampleCommand.php

-        );
-
-        $this->print('Table `episodes` has been created.');
+        $this->ydb->table()->retrySession(function (Session $session) {


need set idempotend = true for this and other idempotent operations

…ry-function

ilyakharev self-assigned this Jun 4, 2023

ilyakharev requested review from rekby and asmyasnikov as code owners June 4, 2023 19:47

rekby reviewed Jun 7, 2023

View reviewed changes

ilyakharev and others added 12 commits June 12, 2023 22:30

Init

54e920b

Update tests

09ad561

Update retry

387762f

Added retryTransaction function

bfd7361

Update examples

e47e223

Update composer.json

e8abcdd

Update composer.json

da43f31

Added exceptions

996511d

Merge branch 'main' into add-retry-function

a008365

Update Retry.php, Table.php, RetryOnBadSessionTest.php

19e0b7e

Remove debug print in CheckTypeTest.php

efed1c1

Update tests

e13535b

rekby reviewed Jun 23, 2023

View reviewed changes

ilyakharev and others added 9 commits June 23, 2023 13:37

Update exceptions, examples

78af75c

Merge branch 'main' into add-retry-function

a748a2e

Update examples

0abe1c0

Merge remote-tracking branch 'origin/add-retry-function' into add-ret…

da3165d

…ry-function

Update ListEndpointsCommand.php

b299454

Update MakeDirectoryCommand.php

a953936

Update RemoveDirectoryCommand.php

7c71a46

Update

3c01213

Merge branch 'main' into add-retry-function

94d37ce

rekby approved these changes Jun 26, 2023

View reviewed changes

ilyakharev added 2 commits June 27, 2023 11:04

Update CHANGELOG.md

0d4817c

Update Ydb.php

dc96c2d

ilyakharev merged commit a9ea4c4 into ydb-platform:main Jun 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added retry function #63

Added retry function #63

ilyakharev commented Jun 4, 2023 •

edited

Loading

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 7, 2023

rekby Jun 14, 2023

rekby Jun 23, 2023

rekby Jun 23, 2023

rekby Jun 23, 2023

rekby Jun 23, 2023

rekby Jun 23, 2023

rekby Jun 23, 2023

rekby Jun 23, 2023


		namespace YdbPlatform\Ydb\Exceptions;

		class FastBackoffRetryableException extends RetryableException


		$output->writeln('Table ' . $table_name . ' has been created.');
		$output->writeln('Table ' . $table_name . ' has been created.');

		@@ -158,9 +151,13 @@ protected function doStreamRequest($service, $method, $data = [])
		*/
		protected function checkStatus($service, $method, $status)

Added retry function #63

Added retry function #63

Conversation

ilyakharev commented Jun 4, 2023 • edited Loading

Pull request type

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

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

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

ilyakharev commented Jun 4, 2023 •

edited

Loading