feat: enable retry for addItem and add more debug info #14

raymondfeng · 2018-09-20T17:30:47Z

Follow up #13 per @bajtos comments:

Enable retry for adding items to a shopping cart
Add more debug statements

b-admike · 2018-09-20T18:39:08Z

package-lock.json

@@ -100,6 +100,17 @@
        "globals": "^11.1.0",
        "invariant": "^2.2.0",
        "lodash": "^4.17.5"
+      },


Do we want package-lock.json in this repository?

Yes, package.lock is good for an application to make it repeatable. For example, we can use npm ci and npm audit fix.

b-admike · 2018-09-20T18:42:03Z

src/controllers/shopping-cart.controller.ts

+      await sleep(interval);
+    } else {
+      // No more retries, timeout
+      const msg = `Fail to ${task.description} after ${maxRetries *


nitpick: Fail to -> Failed to

I'm fine with either tense.

b-admike · 2018-09-20T19:38:59Z

src/controllers/shopping-cart.controller.ts

+ * @param maxRetries Maximum number of tries, default to 10
+ * @param interval Milliseconds to wait after each try, default to 100ms
+ */
+async function retry<T>(


do we want tests for this function?

We could add a unit test but not sure how to test it with redis.

Added tests.

bajtos · 2018-09-21T08:08:13Z

src/controllers/shopping-cart.controller.ts

+        description: `update the shopping cart for '${userId}'`,
+      },
+      10,
+      100,


I find these positional arguments difficult to understand. What is 10 and 100?

I am proposing to change retry API to accept an options object instead of positional parameters.

return retry<ShoppingCart>( { run: () => this.shoppingCartRepository.addItem(userId, item), description: `update the shopping cart for '${userId}'`, }, { maxRetries: 10, retryInterval: 100, });

bajtos · 2018-09-21T08:11:55Z

src/repositories/shopping-cart.repository.ts

@@ -42,6 +42,9 @@ export class ShoppingCartRepository extends DefaultKeyValueRepository<
   * @param userId User id
   * @param check A function that checks the current value and produces a new
   * value. It returns `null` to abort.
+   *
+   * @returns A promise of the updated ShoppingCart instance or `null` if the


I find this sentence ambiguous. If the transaction fails, is the function returning null or a Promise that's resolved to null?

Also: A transaction can fail for other reasons besides a race condition, for example the server can become unreachable or unresponsive. We are not returning null but rejecting the promise in those cases!

Proposed wording:

A promise that's resolved with the updated ShoppingCart instance or with null if the transaction failed because of a race condition.

bajtos · 2018-09-21T08:14:15Z

src/utils/retry.ts

+export interface Task<T> {
+  run(): Promise<T | null | undefined>;
+  // tslint:disable-next-line:no-any
+  isDone?(err: any, result: T | null | undefined): result is T;


KISS & YAGNI.

Let's remove isDone from this pull request since we don't need it (yet).

bajtos · 2018-09-21T08:19:49Z

src/controllers/shopping-cart.controller.ts

@@ -78,6 +85,14 @@ export class ShoppingCartController {
    @param.path.string('userId') userId: string,
    @requestBody({description: 'shopping cart item'}) item: ShoppingCartItem,
  ) {
-    await this.shoppingCartRepository.addItem(userId, item);
+    debug('Add item %j to shopping cart %s', item, userId);
+    return retry<ShoppingCart>(


IMO, it should be the responsibility of a repository to deal with the need to retry this transaction. To me, it's an implementation detail specific to Redis storage. If we use a different backend that supports atomic partial updates, then retry won't be necessary.

It feels very wrong to me that we have to change controller code when the backing database is changed. The repository should be the part of the application dealing with database complexities.

Please move retry to ShoppingCartRepository.

bajtos · 2018-09-21T08:25:57Z

src/utils/retry.ts

+
+// tslint:disable-next-line:no-any
+function taskFinished<T>(err: any, result: T | null | undefined): result is T {
+  return err == null && result != null;


Assuming we want to make retry generic: I think it's not safe to assume that null/undefined is always an invalid result signaling a need to retry.

I think we should remove result != null check and modify the shopping-cart code calling retry to provide isDone function too. (In which case my previous comment #14 (comment) should be ignored.)

Alternatively, it may be better & easier to combine these two function into one and change the return value into an object with two properties: done: boolean and value: T.

{done: true, value: /*result*/} - the task finished successfully

{done: false /* value can be omitted */} - a retry is needed

One more important point: your current implementation retries regardless of whether the transaction was successfully executed with rollback result or whether there was an error which may prevent retries from succeeding (the commands are not valid, the server is not responding, etc.). That does not feel right to me, as it can retry a request that's known to fail again, unnecessarily putting more pressure on the backend system.

IMO, this retry function should fail immediately when the task fails. Implementing a circuit-switcher behavior is out of scope of this work.

bajtos · 2018-09-21T08:27:39Z

test/acceptance/shopping-cart.controller.acceptance.ts

@@ -117,24 +117,26 @@ describe('ShoppingCartController', () => {
    await cartRepo.deleteAll();
  }

+  function givenAnItem(item?: Partial<ShoppingCartItem>) {
+    return new ShoppingCartItem(
+      item || {


Data builders typically use Object.assign, allowing users to leverage sensible defaults provided by the data builder and customize only the properties relevant to the test.

const defaultData = { // sensible property values }; return new ShoppingCartItem( Object.assign(defaultData, item) );

bajtos · 2018-09-21T08:28:12Z

test/acceptance/shopping-cart.controller.acceptance.ts

  function givenShoppingCart() {
    return new ShoppingCart({
      userId: 'user-0001',
      items: [
-        new ShoppingCartItem({
+        givenAnItem({
          productId: 'iPhone XS Max',
          quantity: 1,
          price: 1200,


Just use givenAnItem(), the specific property values are not important here.

bajtos · 2018-09-21T08:30:42Z

src/utils/retry.ts

+ * @param ms Number of milliseconds to wait
+ */
+export function sleep(ms: number) {
+  return promisify(setTimeout)(ms);


Please move promisify(setTimeout) out of this function, so that we don't have to create a new function whenever sleep is called.

const setTimeoutAsync = promisify(setTimeout); export function sleep(ms) { return setTimeoutAsync(ms); }

I think this can be further simplified as follows:

export const sleep = promisify(setTimeout);

raymondfeng · 2018-09-21T16:37:35Z

@bajtos Great feedback. PTAL.

bajtos · 2018-09-24T14:15:06Z

src/utils/retry.ts

+    // tslint:disable-next-line:no-any
+    err: any,
+    result: T | null | undefined,
+  ): {done: boolean; value: T | null | undefined};


IIUC, check is both signaling whether the task is finished and converting the outcome of run to the final value. That seems overly complicated to me, and at the same time not flexible enough, because check has T both on input and output, thus it cannot do that much.

In my original comment, I had the following interface in my mind:

export interface TaskFinished<T> { done: true; result: T; } export interface RetryNeeded { done: false; } export interface Task<T> { run(): Promise<TaskFinished<T> | RetryNeeded>; description?: string; }

On the second look, I see that your check method is accepting also err that may have been returned by run. What benefits do you see in this design?

I would personally prefer to use regular async/await and try/catch flow for error handling, instead of having to use run/check semantics.

run() { try { // do some work return {done: true, value: 'some data'}; } catch (err) { // if the error means I should retry return {done: false} else throw err; } }

Thoughts?

raymondfeng · 2018-09-24T19:25:05Z

@bajtos PTAL

Signed-off-by: Raymond Feng <[email protected]>

bajtos · 2018-09-25T07:51:30Z

src/utils/retry.ts

+
+export interface TaskStatus<T> {
+  done: boolean;
+  value?: T | null;


Nitpick: this type allows the developer to provide combinations of done/value that are not valid:

{done: true} (Missing value. What should be the outcome of the operation?)

{done: false, value: 'some data'}

bajtos · 2018-09-25T07:54:57Z

src/utils/retry.ts

+      maxTries,
+    );
+    const status = await task.run();
+    if (status.done) return status.value!;


status.value!

Please don't! The typings are designed in such way that task.run() can return a value that's undefined.

If you want to preserve TaskStatus interface then you need to change retry to return T | null and push the responsibility of handling null/undefined result to callers of retry. This is not a great UX to me.

I think a better solution is to follow my proposal outlined in #14 (comment), I believe it provides information to allow TypeScript understand that when status.done is true, then status.value must be defined.

export interface TaskFinished<T> { done: true; result: T; } export interface RetryNeeded { done: false; } export interface Task<T> { run(): Promise<TaskFinished<T> | RetryNeeded>; description?: string; }

bajtos · 2018-09-25T07:56:46Z

src/utils/retry.ts

+      const msg = `Failed to ${task.description} after ${maxTries *
+        interval} ms`;
+      debug('%s', msg);
+      throw new HttpErrors.RequestTimeout(msg);


Nitpick: Since retry is not used at REST layer, but inside a repository, I think we shouldn't be throwing an HttpError and use err.code to provide a machine-readable flag that can be used to detect this particular error.

bajtos

The current implementation of retry is good enough for the specific use case of our shopping example.

My last comments above would become important if we wanted to make retry more generic and move it to loopback-next.

raymondfeng force-pushed the improve-add-item branch from 8c0a512 to 1658026 Compare September 20, 2018 17:31

b-admike reviewed Sep 20, 2018

View reviewed changes

raymondfeng force-pushed the improve-add-item branch 2 times, most recently from f2f1805 to bf3c6e0 Compare September 20, 2018 21:55

bajtos reviewed Sep 21, 2018

View reviewed changes

bajtos suggested changes Sep 21, 2018

View reviewed changes

raymondfeng force-pushed the improve-add-item branch 2 times, most recently from e08b4de to 44f163a Compare September 21, 2018 16:37

raymondfeng mentioned this pull request Sep 21, 2018

Demo scenario: Shopping Cart Component loopbackio/loopback-next#1481

Closed

4 tasks

bajtos reviewed Sep 24, 2018

View reviewed changes

raymondfeng force-pushed the improve-add-item branch from 44f163a to b82d847 Compare September 24, 2018 19:24

raymondfeng mentioned this pull request Sep 24, 2018

feat: add a default home page #16

Merged

feat: enable retry for addItem and add more debug info

3ed03d9

Signed-off-by: Raymond Feng <[email protected]>

raymondfeng force-pushed the improve-add-item branch from b82d847 to 3ed03d9 Compare September 24, 2018 22:13

bajtos reviewed Sep 25, 2018

View reviewed changes

bajtos approved these changes Sep 25, 2018

View reviewed changes

raymondfeng merged commit 429a6ae into master Sep 25, 2018

raymondfeng deleted the improve-add-item branch September 25, 2018 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable retry for addItem and add more debug info #14

feat: enable retry for addItem and add more debug info #14

raymondfeng commented Sep 20, 2018

b-admike Sep 20, 2018

raymondfeng Sep 20, 2018

b-admike Sep 20, 2018

raymondfeng Sep 20, 2018

b-admike Sep 20, 2018

raymondfeng Sep 20, 2018

raymondfeng Sep 20, 2018

bajtos Sep 21, 2018

bajtos Sep 21, 2018

bajtos Sep 21, 2018

bajtos Sep 21, 2018

bajtos Sep 21, 2018 •

edited

Loading

bajtos Sep 21, 2018

bajtos Sep 21, 2018

bajtos Sep 21, 2018

raymondfeng commented Sep 21, 2018

bajtos Sep 24, 2018

raymondfeng commented Sep 24, 2018

bajtos Sep 25, 2018

bajtos Sep 25, 2018

bajtos Sep 25, 2018

bajtos left a comment

feat: enable retry for addItem and add more debug info #14

feat: enable retry for addItem and add more debug info #14

Conversation

raymondfeng commented Sep 20, 2018

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

bajtos Sep 21, 2018 • 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

raymondfeng commented Sep 21, 2018

Choose a reason for hiding this comment

raymondfeng commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

bajtos Sep 21, 2018 •

edited

Loading