[coordinator] Return HTTP 400 if all sent samples encounter too old/new or other bad request error #1692
Conversation
|
Need to add tests still. |
robskillington
force-pushed
the
r/return-400-on-bad-request-writes
branch
from
32bf1e0 to
cf94bf8
Compare
| for _, err := range errs { | ||
| switch { | ||
| case client.IsBadRequestError(err): | ||
| fallthrough |
There was a problem hiding this comment.
Might be worth having a different output error here if it's bad requests vs invalid params specifically?
There was a problem hiding this comment.
We need both to be HTTP 400 (since local aggregation can also cause a "too old" error, which will be invalidparams not a client bad request error).
The error actually returned to the client I haven't changed, it will always be the "last error".
…e write on too far in past
|
|
||
| Storage() storage.Storage | ||
| } | ||
|
|
||
| // BatchError allows for access to individual errors. | ||
| type BatchError interface { | ||
| error |
There was a problem hiding this comment.
Considering we’re only returning the last error in this path, can we add a new implementation of multierr which doesn’t accumulate?
There was a problem hiding this comment.
So I need to actually look at each error and determine 100% of them are bad request like errors, otherwise if there was one real error, the request needs to be retried.
I use Errors() to first establish all real errors are bad request like errors, then I use LastError() just to grab the one for logging purposes. I could probably remote LastError() tbh from this interface.
| @@ -183,7 +186,7 @@ echo "Validating topology" | |||
| echo "Done validating topology" | |||
|
|
|||
| echo "Waiting until shards are marked as available" | |||
| ATTEMPTS=10 TIMEOUT=2 retry_with_backoff \ | |||
| ATTEMPTS=100 TIMEOUT=2 retry_with_backoff \ | |||
There was a problem hiding this comment.
Yes! Much nicer haha, can we do the same for integration tests?
There was a problem hiding this comment.
True yeah, will update the common ones.
robskillington
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1692 +/- ##
=========================================
- Coverage 71.9% 57.1% -14.8%
=========================================
Files 976 968 -8
Lines 81530 81092 -438
=========================================
- Hits 58693 46378 -12315
- Misses 18996 31113 +12117
+ Partials 3841 3601 -240
Continue to review full report at Codecov.
|
What this PR does / why we need it:
This should fix issue #1709.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: