mapbox / dyno

simple dynamodb client
MIT License
78 stars 28 forks source link

Add backoff to queryStream, scanStream #129

Closed hugojosefson closed 1 year ago

hugojosefson commented 7 years ago

A reasonable user of these streaming functions would expect them to stream data as long as there is data (paging, already implemented), and at the rate it can be fetched.

This commit takes the specific errors ProvisionedThroughputExceededException and ThrottlingException, and handles them as signals to temporarily back off.

It starts retrying after 200 ms, then doubles that each time it retries without a successful response, up to 60000 ms. Then it retries every 60000 ms.

hugojosefson commented 7 years ago

Many tests on Travis CI report this same error:

{
  [TimeoutError: Missing credentials in config]
  message: 'Missing credentials in config',
  code: 'CredentialsError',
  time: Thu Dec 01 2016 19:36:25 GMT+0000 (UTC),
  retryable: true,
  originalError: {
    code: 'CredentialsError',
    message: 'Could not load credentials from any providers',
    originalError: {
      code: 'TimeoutError',
      message: 'Connection timed out after 1000ms',
      retryable: true,
      time: Thu Dec 01 2016 19:36:25 GMT+0000 (UTC)
    },
    retryable: true,
    time: Thu Dec 01 2016 19:36:25 GMT+0000 (UTC)
  }
}

Is this a problem with the build setup, rather than this PR?

rclark commented 7 years ago

ProvisionedThroughputExceededException and ThrottlingException errors are retried with backoff by the aws-sdk-js itself. I think a more straightforward way to accomplish what you're after here would be to

  1. Remove the error and success event handlers
  2. Replace them with a complete event handler that parses the response in order to emit an error or handle the success case.

The use of .on('error') prior to your PR is indeed problematic, since this event is fired as soon as a single error occurs, without allowing the SDK to invoke its internal retry logic. By moving to the complete event, we'd allow the backoff/retry already provided by the SDK. This event should be fired with an .error only after all the configured retry attempts have been made.

rclark commented 7 years ago

wrt test failures, Travis runs require the use of some encrypted information that mean only mapbox-org members PRs will pass tests. Once this looks good (and you should be able to run a subset of the tests locally without issue), we'll deal with travis.

Thanks for pointing out this oversight!

hugojosefson commented 7 years ago

Thank you @rclark for your feedback! Those are really good suggestions.

Sorry about my late response. I had an accident and suffered a concussion, which apparently can take a good while to recover from. Please expect me to not be able to contribute here for at least a few more weeks.

In the meantime, if anyone else is interested in implementing this, go right ahead, and do mention this pull request in your pull request, so it shows up here.

Thanks, Hugo