feat(util-waiter): add waiter utilities package by alexforsyth · Pull Request #1736 · aws/aws-sdk-js-v3

alexforsyth · 2020-12-01T21:13:47Z

Issue #, if available:

Adding waiter support for JS SDK V3. This is a collection of utility functions that make up the meat and bones of JS SDK waiters.
Internal design feedback
smithy-typescript codegen PR.
Unit Tests

Description of changes:
Example of how clients may use these utils:

const waitForTableExists = async (params: DynamoDBWaiter, input: DescribeTableInput): Promise<WaiterResult> => {
  params = {
    ...waiterServiceDefaults,
    ...params
  };
  validateWaiterOptions(params);

  const exitConditions = [
    waiterTimeout(params.maxWaitTime), 
    runPolling<DynamoDBClient, DescribeTableInput> (params, params.client, input, checkState)
  ];
  if(params.abortController){
    exitConditions.push(abortTimeout(params.abortController.signal));
  }
  return Promise.race(exitConditions);
}

Smithy PR (WIP): smithy-lang/smithy-typescript#243

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov-io · 2020-12-04T18:11:25Z

Codecov Report

Merging #1736 (88ac270) into master (de75f7e) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1736      +/-   ##
==========================================
+ Coverage   79.77%   79.83%   +0.05%     
==========================================
  Files         325      336      +11     
  Lines       12087    12623     +536     
  Branches     2553     2679     +126     
==========================================
+ Hits         9643    10078     +435     
- Misses       2444     2545     +101

Impacted Files	Coverage Δ
...tocol_tests/aws-restxml/commands/XmlMapsCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...ocol_tests/aws-restxml/commands/XmlBlobsCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...ocol_tests/aws-restxml/commands/XmlEnumsCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...ocol_tests/aws-restxml/commands/XmlListsCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...col_tests/aws-restjson/commands/JsonMapsCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...ol_tests/aws-restjson/commands/JsonBlobsCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...ol_tests/aws-restjson/commands/JsonEnumsCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...ol_tests/aws-restjson/commands/JsonListsCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...tests/aws-restxml/commands/XmlAttributesCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
...tests/aws-restxml/commands/XmlNamespacesCommand.ts	`95.65% <0.00%> (-4.35%)`	⬇️
... and 133 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fab1c1f...88ac270. Read the comment docs.

packages/util-waiter/src/poller.ts

packages/util-waiter/package.json

packages/util-waiter/src/poller.ts

packages/util-waiter/src/utils/sleep.spec.ts

packages/util-waiter/src/utils/sleep.ts

packages/util-waiter/src/utils/sleep.spec.ts

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

packages/util-waiter/src/poller.ts

mtdowling · 2020-12-08T18:55:46Z

packages/util-waiter/src/utils/sleep.ts

+
+export const waiterTimeout = async (seconds: number): Promise<WaiterResult> => {
+  await sleep(seconds);
+  return { state: WaiterState.RETRY };


This would be a timeout right? Since you added an ABORTED state, did you consider a TIMEOUT state?

mtdowling · 2020-12-08T21:07:52Z

packages/util-waiter/src/poller.ts

+import { WaiterOptions, WaiterResult, WaiterState } from "./waiter";
+
+/**
+ * Reference: https://github.com/awslabs/smithy/pull/656


I think it's better to link to the waiters spec rather than the PR to update the spec. Maybe link here since this link will be relevant when we release the next version of Smithy that includes jitter in waiters: https://awslabs.github.io/smithy/1.0/spec/waiters.html#waiter-retries

mtdowling · 2020-12-08T21:12:48Z

packages/util-waiter/src/poller.ts

+ * `maxWaitTime`
+ */
+const exponentialBackoffWithJitter = (floor: number, ciel: number, attempt: number) =>
+  Math.floor(Math.min(ciel, randomInRange(floor, floor * 2 ** (attempt - 1))));


floor * 2 ** (attempt - 1)

I think this will overflow your number type for large values of attempt. To address this, you can compute the maximum attempt ceiling that doesn't exceed your ceiling.

Pseudo code:

attemptCeiling = (log(maxDelay / minDelay) / log(2)) + 1 if attempt > attemptCeiling: delay = maxDelay else: delay = minDelay * 2 ** (attempt - 1) delay = random(minDelay, delay) if remainingTime - delay <= minDelay: delay = remainingTime - minDelay

As I mentioned in the comment, this is not possible because it is in fact limited by maxWaitTime in the promise race in other abstraction.

I dont think Javascript has number overflow: https://www.algotech.solutions/blog/javascript/handle-number-overflow-javascript/

@alexforsyth It's possible to overflow the JS Number:

> Number.MAX_VALUE * 2 > Infinity

mtdowling · 2020-12-08T21:15:29Z

packages/util-waiter/src/waiter.ts

+export interface WaiterConfiguration {
+  /**
+   * The amount of time in seconds a user is willing to wait for a waiter to complete. This
+   * defaults to 300 (5 minutes).


I don't recommend setting a default value for this. This is hands-down the most important waiter setting there is, and customers should tell us how long they're willing to wait.

Oh this is actually old documentation. We dont set this anywhere. I'll change

mtdowling · 2020-12-08T21:16:43Z

packages/util-waiter/src/poller.ts

+  while (true) {
+    await sleep(exponentialBackoffWithJitter(minDelay, maxDelay, currentAttempt));
+    const { state } = await acceptorChecks(client, input);
+    if (state === WaiterState.SUCCESS || state === WaiterState.FAILURE) {


Probably better to say if (state !== WaiterState.RETRY) {

mtdowling · 2020-12-08T21:22:37Z

packages/util-waiter/src/utils/sleep.ts

+  return new Promise((resolve) => setTimeout(resolve, seconds * 1000));
+};
+
+export const waiterTimeout = async (seconds: number): Promise<WaiterResult> => {


This method and abortTimeout seem like they're not really useful on their own and publicly expose implementation details of waiters. Did you consider maybe moving these into poller as non-exported functions? You could, for example, export a function like createWaiter or something that takes the maxWaittime, abortController, waiter settings, etc, and it returns a "waiter" or whatever the abstraction is (a promise I guess), that automatically hooks up the promise race to timeout a waiter and checking the abort controller. I suspect this will make codegen simpler and limit the amount of public API surface area y'all have to maintain.

Yeah I thought about just exposing the race. This gets kinda messy cause you have to deal with the generics exposed in the client waiter and pass them all through twice. Once to createWaiter and once again through to the underlying clientWaiter. The function def gets silly.

My 2c is that this is probably too much abstraction and the code starts to be much less clean, but I can totally see what you're saying and I agree that it does reduce API surface area. @AllanZhengYP what do you think?

alexforsyth · 2020-12-10T02:59:02Z

#1759

github-actions · 2021-01-08T00:24:43Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

alexforsyth requested a review from AllanZhengYP December 1, 2020 21:54

alexforsyth changed the title ~~WIP: JS SDK waiters in V3~~ JS SDK waiters in V3 Dec 3, 2020

alexforsyth requested review from kstich, mtdowling and trivikr December 3, 2020 19:13

aws deleted a comment from codecov-io Dec 3, 2020

alexforsyth changed the title ~~JS SDK waiters in V3~~ feat(clients): waiters in JS SDK V3 Dec 4, 2020

trivikr changed the title ~~feat(clients): waiters in JS SDK V3~~ feat(util-waiter): waiters in JS SDK V3 Dec 4, 2020

alexforsyth changed the title ~~feat(util-waiter): waiters in JS SDK V3~~ feat(util-waiter): adding waiter utilities package Dec 4, 2020

trivikr changed the title ~~feat(util-waiter): adding waiter utilities package~~ feat(util-waiter): add waiter utilities package Dec 4, 2020

AllanZhengYP reviewed Dec 4, 2020

View reviewed changes

packages/util-waiter/src/poller.ts Outdated Show resolved Hide resolved

packages/util-waiter/src/poller.ts Outdated Show resolved Hide resolved

packages/util-waiter/src/poller.ts Outdated Show resolved Hide resolved

packages/util-waiter/src/poller.ts Outdated Show resolved Hide resolved

trivikr reviewed Dec 4, 2020

View reviewed changes

alexforsyth and others added 14 commits December 7, 2020 19:11

feat: set up waiters package

bbfc196

feat: adding validation and sleep functions

4f79dbb

feat: waiter interface

2bf531f

feat: generic forever polling

a620107

feat: exports

013953c

fix: wording update for maxWaitTime

05acce5

fix: clean up and updates per design review

cd39e22

polling not working as intended

7702314

fix: poller.spec.ts

6b74244

fix: adding tests to polling

5f622b5

fix: empty changelog

40a6460

fix: update naming to remove references to xml builder

1fc129f

fix(util-watier): apply feedbacks

2b52619

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

feat(util-waiter): add jitter to waiters exponential backoff

88ac270

AllanZhengYP force-pushed the alexforsyth/waiters branch from 36310d3 to 88ac270 Compare December 7, 2020 21:57

AllanZhengYP approved these changes Dec 7, 2020

View reviewed changes

AllanZhengYP requested a review from trivikr December 7, 2020 22:53

alexforsyth commented Dec 7, 2020

View reviewed changes

packages/util-waiter/src/poller.ts Outdated Show resolved Hide resolved

fix(util-waiter): rename exponentialBackoffWithJitter()

4fbdb04

alexforsyth merged commit 02ad0a4 into aws:master Dec 8, 2020

mtdowling reviewed Dec 8, 2020

View reviewed changes

AllanZhengYP mentioned this pull request Dec 10, 2020

feat(util-waiter): add createWaiter() #1759

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jan 8, 2021

Conversation

alexforsyth commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexforsyth commented Dec 10, 2020

Uh oh!

github-actions bot commented Jan 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexforsyth commented Dec 1, 2020 •

edited

Loading

codecov-io commented Dec 4, 2020 •

edited

Loading