Implement and test configurable transaction timeouts #197

flyingsilverfin · 2021-12-21T22:00:27Z

What is the goal of this PR?

Transactions now have a transaction timeouts, which are configurable via transaction options (typedb/typedb#6487). We implemented the new transaction option, and test them via new BDD scenarios. This also caused us to simplify and make more obvious when transaction errors occur when using an open transaction.

What are the changes implemented in this PR?

Add option for transaction timeout (default: 5 mins)
when errors are received from GRPC, we record them in the transaction stream to return immediately, rather than waiting to pull them from the transaction stream queue
add missing BDD steps for session and transaction option configuration
Add missing await class to various test steps in BDD, plus importantly make the close() methods have the right asyncronous/promise signature that ensures users wanting to close() will know to await as well.
update grpc and protobuf versions to the latest

flyingsilverfin

Still want to see if I can reduce size of package.lock diff... or is it ok that's 14k lines change?? Manually updated yarn.lock because i didn't want a 10k differ there too :(

update: done!

flyingsilverfin · 2022-01-10T18:48:50Z

api/connection/TypeDBOptions.ts

@@ -33,6 +33,7 @@ namespace Opts {
        prefetchSize?: number;
        prefetch?: boolean;
        sessionIdleTimeoutMillis?: number;
+        transactionTimeoutMillis?: number;


new transaction option: millis before timeout

flyingsilverfin · 2022-01-10T18:49:11Z

api/connection/TypeDBOptions.ts

+    get transactionTimeoutMillis() {
+        return this._transactionTimeoutMillis;
+    }
+
+    set transactionTimeoutMillis(millis: number) {
+        if (millis < 1) {
+            throw new TypeDBClientError(NEGATIVE_VALUE_NOT_ALLOWED.message(millis));
        }
-        this._sessionIdleTimeoutMillis = value;
+        this._transactionTimeoutMillis = millis;


setter/getter for transaction timeouts

flyingsilverfin · 2022-01-10T18:49:33Z

common/errors/ErrorMessage.ts

-        export const CLUSTER_TOKEN_CREDENTIAL_INVALID = new Client(16, (args: Stringable[]) => `Invalid token credential.`);
-        export const CLUSTER_INVALID_ROOT_CA_PATH = new Client(17, (args: Stringable[]) => `The provided Root CA path '${args[0]}' does not exist`);
-        export const UNRECOGNISED_SESSION_TYPE = new Client(18, (args: Stringable[]) => `Session type '${args[0]}' was not recognised.`);
+        export const TRANSACTION_CLOSED_WITH_ERRORS = new Client(4, (args) => `The transaction has been closed with error(s): \n${args[0]}.`)


new exception to report a transaction is closed with errors

Just for consistency with other constants in this file let's add the type annotation of args:

Suggested change

export const TRANSACTION_CLOSED_WITH_ERRORS = new Client(4, (args) => `The transaction has been closed with error(s): \n${args[0]}.`)

export const TRANSACTION_CLOSED_WITH_ERRORS = new Client(4, (args: Stringable[]) => `The transaction has been closed with error(s): \n${args[0]}.`)

flyingsilverfin · 2022-01-10T18:51:09Z

connection/TypeDBTransactionImpl.ts

+import {Transaction} from "typedb-protocol/common/transaction_pb";
+import {ConceptManager} from "../api/concept/ConceptManager";
+import {TypeDBOptions} from "../api/connection/TypeDBOptions";
+import {TransactionType, TypeDBTransaction} from "../api/connection/TypeDBTransaction";
+import {LogicManager} from "../api/logic/LogicManager";
+import {QueryManager} from "../api/query/QueryManager";
+import {ErrorMessage} from "../common/errors/ErrorMessage";
+import {TypeDBClientError} from "../common/errors/TypeDBClientError";
+import {RequestBuilder} from "../common/rpc/RequestBuilder";
+import {Stream} from "../common/util/Stream";
+import {ConceptManagerImpl} from "../concept/ConceptManagerImpl";
+import {LogicManagerImpl} from "../logic/LogicManagerImpl";
+import {QueryManagerImpl} from "../query/QueryManagerImpl";
+import {BidirectionalStream} from "../stream/BidirectionalStream";
+import {TypeDBSessionImpl} from "./TypeDBSessionImpl";
+import assert = require("assert");


auto format....

flyingsilverfin · 2022-01-10T18:51:35Z

connection/TypeDBTransactionImpl.ts

+    private throwTransactionClosed(): void {
+        assert(!this.isOpen());
+        const errors = this._bidirectionalStream.getErrors();
+        if (errors.length == 0) throw new TypeDBClientError(TRANSACTION_CLOSED);
+        else throw new TypeDBClientError(TRANSACTION_CLOSED_WITH_ERRORS.message(errors));
+    }


we can differentiate between a transaction closed with errors and one without errors

flyingsilverfin · 2022-01-10T19:13:42Z

test/behaviour/connection/ConnectionStepsCluster.ts

+Before(async () => {
+    before();
+    setSessionOptions(TypeDBOptions.cluster({"infer": true}));
+    setTransactionOptions(TypeDBOptions.cluster({"infer": true}));
+});


initialising a cluster scenario involves the normal before() and then setting various session/transaction options we always have on

flyingsilverfin · 2022-01-10T19:13:50Z

test/behaviour/connection/ConnectionStepsCore.ts

+Before(async () => {
+    before();
+    setSessionOptions(TypeDBOptions.core({"infer": true}));
+    setTransactionOptions(TypeDBOptions.core({"infer": true}));
+});


same thing here

flyingsilverfin · 2022-01-10T19:14:09Z

test/behaviour/connection/ConnectionStepsCluster.ts

+    setTransactionOptions(TypeDBOptions.cluster({"infer": true}));
+});
+
+After(async () => after());


for symmetry we also call the after() in the ConnectionStepsBase.ts class here

flyingsilverfin · 2022-01-10T19:14:25Z

test/behaviour/connection/session/SessionSteps.ts

+Given('set session option {word} to: {word}', async function (option: string, value: string) {
+    if (!(option in optionSetters)) {
+        throw ("Unrecognised option: " + option);
+    } else {
+        optionSetters[option](sessionOptions, value);
+    }
+});


setter for connection options

flyingsilverfin · 2022-01-10T19:19:03Z

test/behaviour/cucumber_test.sh

@@ -48,7 +48,7 @@ echo Successfully unarchived TypeDB $PRODUCT distribution.
 echo Starting TypeDB $PRODUCT Server
 mkdir ./typedb-distribution/"$DIRECTORY"/typedb_test
 if [[ $PRODUCT == "Core" ]]; then
-  ./typedb-distribution/"$DIRECTORY"/typedb server --data typedb_test &
+  ./typedb-distribution/"$DIRECTORY"/typedb server --storage.data typedb_test &


new test server argument format!

flyingsilverfin · 2022-01-11T10:45:00Z

BUILD

@@ -79,6 +79,7 @@ behaviour_steps_common = [
    "//test/behaviour/connection/session:SessionSteps.ts",
    "//test/behaviour/connection/transaction:TransactionSteps.ts",
    "//test/behaviour/typeql:TypeQLSteps.ts",
+    "//test/behaviour/util:UtilSteps.ts",


following other clients' we extract the wait steps into their own UtilSteps file

alexjpwalker · 2022-01-11T12:39:58Z

common/errors/ErrorMessage.ts

-        export const CLUSTER_TOKEN_CREDENTIAL_INVALID = new Client(16, (args: Stringable[]) => `Invalid token credential.`);
-        export const CLUSTER_INVALID_ROOT_CA_PATH = new Client(17, (args: Stringable[]) => `The provided Root CA path '${args[0]}' does not exist`);
-        export const UNRECOGNISED_SESSION_TYPE = new Client(18, (args: Stringable[]) => `Session type '${args[0]}' was not recognised.`);
+        export const TRANSACTION_CLOSED_WITH_ERRORS = new Client(4, (args) => `The transaction has been closed with error(s): \n${args[0]}.`)


Just for consistency with other constants in this file let's add the type annotation of args:

Suggested change

export const TRANSACTION_CLOSED_WITH_ERRORS = new Client(4, (args) => `The transaction has been closed with error(s): \n${args[0]}.`)

export const TRANSACTION_CLOSED_WITH_ERRORS = new Client(4, (args: Stringable[]) => `The transaction has been closed with error(s): \n${args[0]}.`)

alexjpwalker · 2022-01-11T13:12:02Z

connection/TypeDBTransactionImpl.ts

+import {QueryManagerImpl} from "../query/QueryManagerImpl";
+import {BidirectionalStream} from "../stream/BidirectionalStream";
+import {TypeDBSessionImpl} from "./TypeDBSessionImpl";
+import assert = require("assert");


We've never used assert before in Client NodeJS's production code.

Unlike in Java, there is no clean way to turn off assertions in NodeJS. (Methods typically used include hot module replacement - literally stripping the statements out of the source code before running it - and modifying the assert prototype to replace assert with a no-op function at runtime!)

I think we're better off replacing it with throw new TypeDBClientError. We've done this elsewhere (see TypeDBClientImpl.session which throws a Client NodeJS-only error if the session ID already exists).

package.json

alexjpwalker · 2022-01-11T13:22:24Z

stream/ResponseCollector.ts

-                }
-            }
+            else if (element.isDone() && !this._error) throw new TypeDBClientError(TRANSACTION_CLOSED);
+            else if (element.isDone() && this._error) throw new TypeDBClientError(TRANSACTION_CLOSED_WITH_ERRORS.message(this._error));


I remember this came up in conversation: did we agree to leave the error there even after reading it for the first time? (seems like the right thing to do, just confirming intent)

Yep we did!

alexjpwalker · 2022-01-11T13:36:17Z

test/behaviour/connection/ConnectionStepsCluster.ts


 BeforeAll(async () => {
    setClient(await TypeDB.clusterClient([TypeDB.DEFAULT_ADDRESS], new TypeDBCredential("admin", "password", process.env.ROOT_CA)));
 });
+
+Before(async () => {
+    before();


We're emulating inheritance here. The Cucumber structure doesn't allow for actual inheritance because it's based on top-level functions, not class functions.

So it might be clearer to call these methods beforeBase and afterBase. It makes it more obvious that they are only a partial implementation of Before - the equivalent of calling super() in Java (or JS).

alexjpwalker · 2022-01-11T13:42:22Z

test/behaviour/util/BUILD

 load("@vaticle_dependencies//tool/checkstyle:rules.bzl", "checkstyle_test")

+exports_files(["Util.ts", "UtilSteps.ts"])


Hmm, I wonder if it's weird having a Util and a UtilSteps in the same package, given that (almost) every other package in behaviour contains only one .ts file, namely <packageName>Steps.ts.

Might be ok!

eh this is how it is in the other client's i'm keeping it!

## What is the goal of this PR? We align with Client NodeJS version 2.6.1 (some of the work in typedb/typedb-driver-nodejs#197), which implements a better error propagation mechanism: when an exception occurs, we store it against all the transaction's active transmit queues to retrieve whenever the user tries to perform an operation in the transaction anywhere. ## What are the changes implemented in this PR? * store errors received from gRPC against each receive queue * return a new exception type, transaction is closed with errors, which throws the errors from all queues (note that this can be duplicate if there are multiple open transmit queues that have been given the same error) * we clean up queues that are no longer needed, so we minimise the number of times the user sees the same exception

## What is the goal of this PR? We align with Client NodeJS version 2.6.1 (mostly the work in typedb/typedb-driver-nodejs#197), which implements a better error propagation mechanism: when an exception occurs, we store it against all the transaction's active transmit queues to retrieve whenever the user tries to perform an operation in the transaction anywhere, ensuring the user always sees the originating error. ## What are the changes implemented in this PR? * store errors received from gRPC against each receive queue * return a new exception type, `transaction is closed with errors`, which throws the errors from all queues (note that this can be duplicate if there are multiple open transmit queues that have been given the same error) * we clean up queues that are no longer needed, so we minimise the number of times the user sees the same exception * update terminology around `ResponseCollector`, calling internal collectors `response_queues` instead of `collectors` (a collector made of collectors is a bit strange)

flyingsilverfin added 5 commits December 21, 2021 19:42

update deps

213fae6

Implement timeout option and error draining

5caed4a

Implement new steps for session and transaction options

1ad353b

fix test runner and imports

5edba1b

use property setters

929d7f8

flyingsilverfin force-pushed the transaction-timeout branch from 7eca957 to 929d7f8 Compare December 21, 2021 22:10

Errors are captured on receive

892b74a

flyingsilverfin marked this pull request as ready for review January 10, 2022 17:32

flyingsilverfin changed the title ~~Transaction timeout~~ Implement and test transaction timeouts Jan 10, 2022

flyingsilverfin added 4 commits January 10, 2022 18:35

fix linter

a93032e

cleanup and bump to protocol from master

7eed859

revert yarn.lock

e77b84a

update yarn.lock

4de4eb9

flyingsilverfin commented Jan 10, 2022

View reviewed changes

flyingsilverfin requested a review from alexjpwalker January 10, 2022 19:19

flyingsilverfin self-assigned this Jan 10, 2022

grabl assigned alexjpwalker Jan 10, 2022

flyingsilverfin added priority: blocker type: feature type: refactor labels Jan 10, 2022

flyingsilverfin added 2 commits January 11, 2022 10:17

create UtilSteps to match other clients

ec036f2

fix build

b1fc1ae

flyingsilverfin commented Jan 11, 2022

View reviewed changes

flyingsilverfin added 4 commits January 11, 2022 10:46

regenerate yarn.lock

7ab502d

update grpc and other deps

adba8df

Revert updated deps

5b7781e

add assertions

0a66f3e

alexjpwalker suggested changes Jan 11, 2022

View reviewed changes

flyingsilverfin added 2 commits January 11, 2022 13:59

Address comments

c074d17

use 2.6.1 tag release

f14db6e

flyingsilverfin added 12 commits January 14, 2022 14:41

use property getter

513ca53

trigger ci

1935793

Use server that logs database operations

70b1a52

update dependencies of grpc and protobuf

3f31ea6

remove extra check on db create

9c5ac16

Add sleep to only core tests

e05c95a

add sandbox_debug

3efdfa9

Add sandbox debug

dbdff45

trigger ci

2e77538

Only run match test

2291c2d

trigger ci

64305c9

Uncomment line

c994ed9

flyingsilverfin force-pushed the transaction-timeout branch from b496285 to c994ed9 Compare January 14, 2022 18:09

flyingsilverfin added 4 commits January 14, 2022 18:11

add more logging to server side

6000f1e

Add print to databaseimpl delete()

7937d34

add await() to Before() and After() methods and restore all ci tests

81b6d02

cleanup console logs for debugging

d0f135f

flyingsilverfin changed the title ~~Implement and test transaction timeouts~~ Implement and test configurable transaction timeouts Jan 14, 2022

flyingsilverfin force-pushed the transaction-timeout branch from d0ff562 to d0f135f Compare January 14, 2022 20:31

flyingsilverfin modified the milestones: Bug Fixes, Technical Debt, TypeDB Storage Optimisation Jan 14, 2022

flyingsilverfin merged commit d476865 into typedb:master Jan 14, 2022

flyingsilverfin deleted the transaction-timeout branch January 14, 2022 20:41

flyingsilverfin mentioned this pull request Jan 20, 2022

Errors propagate through transaction typedb/typedb-driver-python#247

Merged

flyingsilverfin mentioned this pull request Jan 20, 2022

Propagate exceptions throughout transactions typedb/typedb-driver#367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement and test configurable transaction timeouts #197

Implement and test configurable transaction timeouts #197

flyingsilverfin commented Dec 21, 2021 •

edited

Loading

flyingsilverfin left a comment •

edited

Loading

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 10, 2022

alexjpwalker Jan 11, 2022

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 10, 2022

flyingsilverfin Jan 11, 2022

alexjpwalker Jan 11, 2022

alexjpwalker Jan 11, 2022

alexjpwalker Jan 11, 2022

flyingsilverfin Jan 11, 2022

alexjpwalker Jan 11, 2022

flyingsilverfin Jan 11, 2022

alexjpwalker Jan 11, 2022

flyingsilverfin Jan 11, 2022

	export const TRANSACTION_CLOSED_WITH_ERRORS = new Client(4, (args) => `The transaction has been closed with error(s): \n${args[0]}.`)
	export const TRANSACTION_CLOSED_WITH_ERRORS = new Client(4, (args: Stringable[]) => `The transaction has been closed with error(s): \n${args[0]}.`)

		load("@vaticle_dependencies//tool/checkstyle:rules.bzl", "checkstyle_test")

		exports_files(["Util.ts", "UtilSteps.ts"])

Implement and test configurable transaction timeouts #197

Implement and test configurable transaction timeouts #197

Conversation

flyingsilverfin commented Dec 21, 2021 • edited Loading

What is the goal of this PR?

What are the changes implemented in this PR?

flyingsilverfin left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyingsilverfin commented Dec 21, 2021 •

edited

Loading

flyingsilverfin left a comment •

edited

Loading