Skip to content

Conversation

@tomwilkie
Copy link
Contributor

@tomwilkie tomwilkie commented Nov 23, 2016

Also, don't let the client library do retires for us, so we can correctly gauge capacity usage etc.

Part of #132

@tomwilkie tomwilkie mentioned this pull request Nov 23, 2016
@tomwilkie
Copy link
Contributor Author

Status: testing it, and plan on moving the backoff on chunk put into the chunk store using the same mechanism.

@tomwilkie
Copy link
Contributor Author

(Testing locally. Worked first time, so I wrote some unit tests)

Copy link
Contributor

@jml jml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but couldn't manage to do a thorough review.


// Not exported as only used by tests to inject mocks
dynamodb dynamodbClient
s3 s3Client
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to have a separate function for creating an AWSStore that takes a dynamodbClient, s3client, and chunk Cache directly. Separates the concern of parsing & validating config from constructing a usable store.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do a techdebt ticket to split this up; we basically have the beginning of our own dynamodb client now.

continue
}

// If we get provisionedThroughputExceededException, then no items we're processed,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"were"

@tomwilkie tomwilkie merged commit 6a02ecd into master Nov 24, 2016
@tomwilkie tomwilkie deleted the record-dynamodb-errors branch November 24, 2016 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants