Revamp retry and remove unintended exports #69

simonz-bq · 2021-04-14T20:52:21Z

Refactored retry logic
Handle retrying on failures during StartSession
Removed some exports and accessibility to non intentional and undocumented functions and types
Bumped aws-sdk dependency requirement to include CapacityExceededException
Bumped driver version to 2.2.0 and added changelog

byronlin13 · 2021-04-15T20:33:07Z

package.json

  },
  "peerDependencies": {
-    "aws-sdk": "^2.815.0",
+    "aws-sdk": "^2.841.0",


any reason we aren't bumping the devDependency aws-sdk to the same verison?

byronlin13 · 2021-04-15T20:37:45Z

CHANGELOG.md

+  * Removed `Transaction` from the exports list.
+  * Removed modules being accessible when importing from `amazon-qldb-driver-nodejs/src`.
+  * TypeScript: Removed constructors in the API for some exported types.
+
 # 2.1.1


Can we add the date for when 2.1.1 was released

I think we stopped doing that. The release date is also available on npm repository.

byronlin13 · 2021-04-15T20:39:36Z

package.json

@@ -34,10 +34,10 @@
    "sinon": "^7.3.2",
    "ts-node": "^8.3.0",
    "typedoc": "^0.15.0",
-    "typescript": "^3.5.3"
+    "typescript": "^3.8.0"


Should this be called out in CHANGELOG?

It's a dev dependency so it doesn't matter to users

byronlin13 · 2021-04-15T20:52:52Z

src/LogUtil.ts

@@ -48,13 +54,15 @@ export function info(line: string): void {
 /**
 * @returns A boolean indicating whether a logger has been set within the AWS SDK.
 */


do we need @internal here?

Same with _prepend

Yes, because otherwise these are exported to the API

byronlin13 · 2021-04-15T21:37:45Z

src/QldbDriver.ts

+
+        retryConfig = (retryConfig == null) ? this._retryConfig : retryConfig;
+        let session: QldbSession;
+        let startNewSession: boolean = true;


Is this correct?

For the happy case where an execute is successful, the session is placed back into the pool. On another execute, it will create a new session instead of picking up the valid session based on getSession logic

was fixed already but I didnt push it up yet

byronlin13 · 2021-04-15T21:41:35Z

src/QldbDriver.ts

+                    if (e.isRetriable) {
+                        // Always retry on the first attempt if failure was caused by a stale session in the pool 
+                        if (retryAttempt == 1 && e.isISE) {
+                            debug("Initial session received from pool invalid. Retrtying...");


Initial session received from pool is invalid. Retrying...

byronlin13 · 2021-04-15T21:49:26Z

src/QldbHash.ts

@@ -19,6 +19,8 @@ const HASH_SIZE: number = 32



Should we add @internal to HASH SIZE? I see it in the docs

It's not exported in the typescript definition though.

byronlin13 · 2021-04-15T22:39:13Z

src/Transaction.ts

- * An InvalidSessionException indicates that the parent session is dead, and a new transaction cannot be created
- * without a new (Pooled)QldbSession being created from the parent driver.
+ * Every transaction is tied to a parent QldbSession, meaning that if the parent session is closed or invalidated, the 
+ * child transaction is automatically closed and cannot be used. Only one transaction can be active at  any given time 


* Fixed dev dependency version not matching peer dependency * Fixed a typo in docstrings * Updated README TypeScript versioning to match package.json

alpian · 2021-04-20T01:19:32Z

src/QldbDriver.ts

@@ -156,36 +169,95 @@ export class QldbDriver {
     * @throws {@linkcode InvalidSessionException} When a session expires either due to a long running transaction or session being idle for long time.
     * @throws {@linkcode BadRequestException} When Amazon QLDB is not able to execute a query or transaction.
     */
-    async executeLambda(
-        transactionLambda: (transactionExecutor: TransactionExecutor) => any,
+    async executeLambda<Type>(


Rather make this a Promise<T>. Using Type like this is confusing.

alpian · 2021-04-20T01:20:26Z

src/QldbDriver.ts

+        }
+
+        // Acquire semaphore and get a session from the pool
+        const getSession: (thisDriver: QldbDriver, startNewSession: boolean) => Promise<QldbSession> = 


This extra typing information is probably unnecessary/can be inferred.

alpian · 2021-04-20T01:20:58Z

src/QldbDriver.ts

+
+        // Release semaphore and if the session is alive return it to the pool and return true
+        const releaseSession: (thisDriver: QldbDriver, session: QldbSession) => boolean = 
+            function(thisDriver: QldbDriver, session: QldbSession): boolean {


Typing information can be inferred

alpian · 2021-04-20T01:22:17Z

src/QldbDriver.ts

+        retryConfig = (retryConfig == null) ? this._retryConfig : retryConfig;
+        let session: QldbSession;
+        let startNewSession: boolean = false;
+        for (let retryAttempt: number = 1; true; retryAttempt++) {


Eh? Why not a simple while(true) or for (blah < whatever).

Well we want to instantiate the retryAttempt and have it increment on every loop, so why not?

It looks unusual to me, which doesn't seem worth it for saving one line of code. Your call to change or not.

alpian · 2021-04-20T01:23:10Z

src/QldbDriver.ts

+                return await session.executeLambda(transactionLambda);
+            } catch (e) {
+                if (e instanceof ExecuteError) {
+                    if (e.isRetriable) {


If we introduced this odd spelling here, let's not do that, otherwise I'll just be sad for a bit :)

I'll immediately investigate if this can be done (see if we don't have any public facing API functions that say "retriable") after this PR is completed.

Thanks - can you create a GitHub issue to track this improvement?

alpian · 2021-04-20T01:24:52Z

src/QldbDriver.ts

+                        }
+                        await new Promise(resolve => setTimeout(resolve, backoffDelay));
+
+                        info(`A recoverable error has occured. Attempting retry #${retryAttempt}.`);


Debug-level I think

The rest of the drivers have this on the info level.

alpian · 2021-04-20T01:25:37Z

src/QldbSession.ts

-                throw new StartTransactionError(e);
+            const isRetriable: boolean = isRetriableException(e);
+            const isISE: boolean = isInvalidSessionException(e);
+            if (isISE && !isTransactionExpiredException(e)) {


Again expand the acronym please

We cant do that here because isInvalidSessionException is imported from another module, so it's already defined.

What @alpian is referring to is the isISE variable, we should expand that to not be the abbrevation.

What was missed in the response here is that we cannot expand the name to match the function due to name resolution, and the feeling of keeping this local variable as an acronym would be acceptable.

Confused. Why can't const isISE be const isSessionInvalid?

alpian · 2021-04-20T01:26:42Z

src/errors/Errors.ts

+        Object.setPrototypeOf(this, ExecuteError.prototype)
+        this.cause = cause;
+        this.isRetriable = isRetriable;
+        this.isISE = isISE;


Same thing here, what is ISE?

alpian · 2021-04-20T01:27:24Z

src/retry/BackoffFunction.ts

@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.


This is muddying the PR. Can we keep this cosmetic stuff separate please?

It was an intentional decision since almost all of the files were being touched anyways.

In the future will keep in a separate PR!

alpian · 2021-04-20T01:29:07Z

tsconfig.json

@@ -1,13 +1,14 @@
 {
    "compilerOptions": {
-        "target": "es6",
+        "target": "ES2015",


Why do this? These are the same afaik,

Consistency. We had a reference to es2015 and es6 in the same file, which can be somewhat confusing.

Official TS docs also says the "allowed" string is capitalized, so might as well do an easy clean-up.

kylepbit · 2021-04-20T04:53:28Z

README.md

@@ -15,12 +15,12 @@ from the ~/.aws/config file.

 See [Setting Region](https://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/setting-region.html) page for more information.

-### TypeScript 3.5.x
+### TypeScript 3.8.x


Is 3.8 needed, or can it stay at 3.5?

Honestly we don't even need 3.5. The real version we need is some undetermined previous version.

Figured we'd align up our documentation and all and 3.8 was what I was playing with testing hash private functions. This ultimately doesn't matter because it's an independent dev dependency.

kylepbit · 2021-04-20T04:57:05Z

src/QldbDriver.ts

+                session.endSession();
+            }
+        }
+        this._isClosed = true;


We should probably mark the driver as closed before starting to empty the pool so no new items can be opened (no matter that this shouldn't be possible given the single-threaded nature).

Done, but yes it doesn't matter in this case.

kylepbit · 2021-04-20T04:57:30Z

src/QldbDriver.ts

+     close(): void {
+        while (this._sessionPool.length > 0) {
+            const session: QldbSession = this._sessionPool.pop();
+            if (session != undefined) {


Is this possible, that there can be undefined items in the pool?

I'm not 100% sure as node is supposedly single threaded so this code block shouldn't have multiple things accessing it, but popping from an empty array gives you undefined. This safety check is a case of better safe than sorry.

kylepbit · 2021-04-20T04:59:08Z

src/QldbDriver.ts

+                    `Getting session. Current free session count: ${thisDriver._sessionPool.length}. ` +
+                    `Currently available permit count: ${thisDriver._availablePermits}.`
+                );
+                if (thisDriver._semaphore.tryAcquire()) {


This is purely stylistic, but when there's an error case my preference is to put it within the if block and have the throw there, and leave the rest of the code not indented. Here, this would mean checking on !tryAcquire() and throwing, while leaving the rest of the code not indented.

Purely stylistic.

kylepbit · 2021-04-20T05:01:00Z

src/QldbDriver.ts

+        let startNewSession: boolean = false;
+        for (let retryAttempt: number = 1; true; retryAttempt++) {
+            try {
+                session = null;


Why the assignment to null before assigning on getSession?

Prevents the session from the last loop being passed to releaseSession twice in the case that startSession fails

kylepbit · 2021-04-20T05:01:28Z

src/QldbDriver.ts

+                    if (e.isRetriable) {
+                        // Always retry on the first attempt if failure was caused by a stale session in the pool 
+                        if (retryAttempt == 1 && e.isISE) {
+                            debug("Initial session received from pool is invalid. Retrtying...");


Note the typo in Retrying...

kylepbit · 2021-04-20T05:02:57Z

src/QldbDriver.ts

+                            throw e.cause;
+                        }
+
+                        const backoffFunction: BackoffFunction = retryConfig.getBackoffFunction();


Deprecation would need a notice for at least a release, and can be done via: microsoft/TypeScript#390

kylepbit · 2021-04-20T05:05:12Z

src/QldbSession.ts

        } catch (e) {
-            if (isBadRequestException(e)) {
-                throw new StartTransactionError(e);
+            const isRetriable: boolean = isRetriableException(e);


I think this spelling makes Ian sad.

Indeed. Let me check after this PR whether or not we exposed any function names or parameters as "Retriable" to see if we can fix this.

kylepbit · 2021-04-20T05:07:28Z

src/QldbSession.ts

+        return new Transaction(this._communicator, startTransactionResult.TransactionId);
+    }
+
+    private async _tryAbort(): Promise<void> {


Generally a tryX function would return true or false depending on if it was successful at the try or not.

kylepbit · 2021-04-20T05:09:34Z

src/errors/Errors.ts

@@ -167,8 +165,20 @@ export function isBadRequestException(e: AWSError): boolean {
 * Is the exception a retriable exception?
 * @param e The client error caught.
 * @returns True if the exception is a retriable exception. False otherwise.
+ * 
+ * @internal
 */
 export function isRetriableException(e: AWSError): boolean {


Given that these are no longer exported, we're probably free to change the spelling?

Perhaps adding a separate PR for cosmetic items, as Ian suggested below (dates and spelling changes).

* Infer type of scoped functions in execute * Expand IsISE to full name * Rename tryAbort since it doesn't return a bool * Fix a typo in log message

kylepbit

+1

alpian

Approved with comments

alpian · 2021-04-23T17:56:08Z

src/QldbDriver.ts

+        retryConfig = (retryConfig == null) ? this._retryConfig : retryConfig;
+        let session: QldbSession;
+        let startNewSession: boolean = false;
+        for (let retryAttempt: number = 1; true; retryAttempt++) {


It looks unusual to me, which doesn't seem worth it for saving one line of code. Your call to change or not.

alpian · 2021-04-23T17:58:10Z

src/QldbSession.ts

-                throw new StartTransactionError(e);
+            const isRetriable: boolean = isRetriableException(e);
+            const isISE: boolean = isInvalidSessionException(e);
+            if (isISE && !isTransactionExpiredException(e)) {


Confused. Why can't const isISE be const isSessionInvalid?

Simon Zhao added 3 commits April 13, 2021 16:58

Refactor retry logic

0670a3f

Fix acessibility modifiers for TypeScript

a06d34f

Bump version and add changelog

9632e3d

byronlin13 reviewed Apr 15, 2021

View reviewed changes

Update unit tests and fix PR comments

977fa86

* Fixed dev dependency version not matching peer dependency * Fixed a typo in docstrings * Updated README TypeScript versioning to match package.json

alpian suggested changes Apr 20, 2021

View reviewed changes

kylepbit reviewed Apr 20, 2021

View reviewed changes

Address PR comments

843486e

* Infer type of scoped functions in execute * Expand IsISE to full name * Rename tryAbort since it doesn't return a bool * Fix a typo in log message

simonz-bq force-pushed the revamp-retry branch from 90781c0 to 843486e Compare April 20, 2021 22:48

simonz-bq marked this pull request as ready for review April 20, 2021 22:50

kylepbit reviewed Apr 21, 2021

View reviewed changes

Fix minor typo in "occurred"

f92e0cd

alpian approved these changes Apr 23, 2021

View reviewed changes

simonz-bq merged commit 0b5c02d into master Apr 23, 2021

simonz-bq deleted the revamp-retry branch April 23, 2021 21:34

		@@ -1,5 +1,5 @@
		/*
		* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.

Revamp retry and remove unintended exports #69

Revamp retry and remove unintended exports #69

Conversation

simonz-bq commented Apr 14, 2021

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

byronlin13 Apr 15, 2021 • 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

byronlin13 Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byronlin13 Apr 15, 2021 • 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

simonz-bq Apr 20, 2021 • 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

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

kylepbit left a comment

Choose a reason for hiding this comment

alpian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byronlin13 Apr 15, 2021 •

edited

Loading

byronlin13 Apr 15, 2021 •

edited

Loading

byronlin13 Apr 15, 2021 •

edited

Loading

simonz-bq Apr 20, 2021 •

edited

Loading