-
Notifications
You must be signed in to change notification settings - Fork 464
fix(spanner): preserve user call options in execute/read and consolidate test databases #9329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
22560dc
1a41e94
e53ebe5
9d572ec
a6759ec
7f8dbc4
cd9613c
2838cc6
726a60a
7d4b223
8f7633e
10ec3d2
58acf32
094f4a1
eee3376
45fab2e
30d2a64
35c12a3
ccd9e90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| # -*- protobuffer -*- | ||
| # proto-file: google3/devtools/kokoro/config/proto/build.proto | ||
| # proto-message: BuildConfig | ||
|
|
||
| # Path to Dockerfile, PREFIXED with the SCM name from the Job Config | ||
| dockerfile_path: "build-dir/.kokoro/docs/docker/Dockerfile" | ||
|
|
||
| container_artifact { | ||
| destination: "gcr.io/cloud-devrel-kokoro-resources/google-cloud-php-docs:latest" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,7 +202,7 @@ public function getOperationsClient() | |
| */ | ||
| public function resumeOperation($operationName, $methodName = null) | ||
| { | ||
| $options = $this->descriptors[$methodName]['longRunning'] ?? []; | ||
| $options = isset($methodName) ? ($this->descriptors[$methodName]['longRunning'] ?? []) : []; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I do not see a functional difference between these two lines which were changed. I think this needs to be reverted. It's also a generated file so it shouldn't be part of this PR anyway, but would need to be a change in the GAPIC generator
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this is related to the changes in googleapis/gapic-generator-php#842 (review). I've left a comment there too - I think this is a hallucination |
||
| $operation = new OperationResponse($operationName, $this->getOperationsClient(), $options); | ||
| $operation->reload(); | ||
| return $operation; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,7 +203,7 @@ public function getOperationsClient() | |
| */ | ||
| public function resumeOperation($operationName, $methodName = null) | ||
| { | ||
| $options = $this->descriptors[$methodName]['longRunning'] ?? []; | ||
| $options = isset($methodName) ? ($this->descriptors[$methodName]['longRunning'] ?? []) : []; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, I think this change should be reverted |
||
| $operation = new OperationResponse($operationName, $this->getOperationsClient(), $options); | ||
| $operation->reload(); | ||
| return $operation; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -965,6 +965,17 @@ public function runTransaction(callable $operation, array $options = []): mixed | |
| $this->isRunningTransaction = true; | ||
| try { | ||
| $res = call_user_func($operation, $transaction); | ||
| } catch (\Throwable $e) { | ||
| $active = $transaction->state() === Transaction::STATE_ACTIVE; | ||
| $singleUse = $transaction->type() === Transaction::TYPE_SINGLE_USE; | ||
| if ($active && !$singleUse) { | ||
| try { | ||
| $transaction->rollback($options); | ||
| } catch (\Throwable $rollbackException) { | ||
| // ignore rollback failure and bubble up the original exception | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is great! |
||
| throw $e; | ||
| } finally { | ||
| $this->isRunningTransaction = false; | ||
| } | ||
|
|
@@ -1676,12 +1687,13 @@ public function execute($sql, array $options = []): Result | |
|
|
||
| $session = $options['session'] ?? $this->session; | ||
| $executeOptions = $this->pluckArray(['parameters', 'types'], $options); | ||
| $callOptions = $this->pluckArray(['requestOptions', 'timeoutMillis'], $options); | ||
| return $this->operation->execute($session, $sql, $executeOptions + [ | ||
| 'transaction' => $txnOptions, | ||
| 'transactionContext' => $txnContext, | ||
| 'directedReadOptions' => $directedReadOptions, | ||
| 'route-to-leader' => $txnContext === Database::CONTEXT_READWRITE | ||
| ]); | ||
| ] + $callOptions); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -2061,9 +2073,11 @@ public function read($table, KeySet $keySet, array $columns, array $options = [] | |
| 'transaction' => $txnOptions, | ||
| ]; | ||
|
|
||
| $callOptions = $this->pluckArray(['requestOptions', 'timeoutMillis'], $options); | ||
|
|
||
| return $this->operation->read($this->session, $table, $keySet, $columns, $readOptions + [ | ||
| 'route-to-leader' => $txnContext === Database::CONTEXT_READ | ||
| ]); | ||
| ] + $callOptions); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these can all be simplied