Return chunks fetched by GetChunks on error#791
Conversation
|
Gentle nudge for a unit test perhaps? |
So we can run a test where some operations succeed and then subsequent ones fail.
Some kludging required to (a) make sure it runs only on DynamoDB and (b) access the error parameters which are on a non-exported type. Also lots of magic numbers. It does the job.
If DynamoDB is overloaded, GetChunks() may time out, but we should return the chunks fetch so they can be put into the cache. Then if the client retries we will get a bit further next time.
bboreham
force-pushed
the
return-partial-chunk-fetch
branch
from
52f1cda to
39debda
Compare
|
@tomwilkie that was a good nudge - turned out the code I'd written still didn't work. The groundwork to be able to inject partial errors was rather tortuous, and the test is a bit "carefully crafted", but it does the job. Bonus: the backoff parameters are now configurable. I only really needed to set them faster so the test would time out in under a minute, but it seemed natural to add command-line options. |
| func TestChunksPartialError(t *testing.T) { | ||
| forAllFixtures(t, func(t *testing.T, client chunk.StorageClient) { | ||
| // This test is currently very specialised for DynamoDB | ||
| if !strings.Contains(t.Name(), "DynamoDB") { |
There was a problem hiding this comment.
Perhaps this test belongs in aws package instead then? We already put the dynamodb specific table manager tests there.
There was a problem hiding this comment.
I tried. It would need more mechanism to get to the fixture stuff, while avoiding circular dependencies.
There was a problem hiding this comment.
Oh well, never mind then.
There was a problem hiding this comment.
I had another go; after 2 hours it's still not passing tests so going to merge this one and post the refactor as a separate PR.
|
Thanks @bboreham, LGTM! One minor nit. |
2 participants
We will put the chunks fetched so far into the cache, so if the client retries we will probably get a bit further next time.
This change was already made by #605, but it was broken about an hour later by merging #603 which was created before.