Core: Send Idempotency-Key on mutation requests when advertised#14740
Core: Send Idempotency-Key on mutation requests when advertised#14740huaxingao merged 4 commits intoapache:mainfrom
Conversation
| tableClient, | ||
| paths.table(ident), | ||
| Map::of, | ||
| mutationHeaders, |
There was a problem hiding this comment.
are we missing the same change for L806?
There was a problem hiding this comment.
Fixed L806 to use mutationHeaders
| tableClient, | ||
| paths.table(finalIdentifier), | ||
| Map::of, | ||
| mutationHeaders, |
There was a problem hiding this comment.
not adjusting new RESTMetricsReporter(restClient, metricsEndpoint, Map::of); is probably fine for now as well, since we didn't add the param to the Spec and because typically we don't care too much whether metrics actually arrive or not, since it's best-effort and we don't want to interfere with the read/write path and add additional latency
|
@huaxingao I think for |
@nastra I split read vs mutation headers in both |
| Set<Endpoint> supportedEndpoints) { | ||
| return new RESTTableOperations(restClient, path, headers, fileIO, current, supportedEndpoints); | ||
| return new RESTTableOperations( | ||
| restClient, path, Map::of, headers, fileIO, current, supportedEndpoints); |
There was a problem hiding this comment.
I think we might want to add the non-mutating headers as params to newTableOps and newViewOps as well since those methods basically just mimic the parameters of the underlying constructor
There was a problem hiding this comment.
Thanks for the suggestion! Done!
| Mockito.spy( | ||
| new RESTCatalogAdapter(backendCatalog) { | ||
| @Override | ||
| public <T extends RESTResponse> T handleRequest( |
There was a problem hiding this comment.
instead of handling this globally for all tests and introducing idempHeaderExpectation and advertiseIdempInConfig it's probably better to use a custom RESTCatalogAdapter for the 2 new tests that you're adding. One example where this is done is in verifyNamespaceExistsFallbackToGETRequest and we could use the same approach in the new tests as well
There was a problem hiding this comment.
Sounds good! Fixed!
huaxingao
force-pushed
the
idempotency_3
branch
from
8e06266 to
3ce7a25
Compare
nastra
changed the title
|
Thanks @nastra a lot for your review! |
…he#14740) * Core: REST client – send Idempotency-Key on mutation requests when advertised * rebase * address comments * address comments
2 participants
This is the 3rd PR for Idempotency Key.
ConfigResponse; if present, useRESTUtil.idempotencyHeaders, elseMap::of.RESTTableOperations/RESTViewOperations.First PR: #14649
SecondPR: #14700
Follow-ups (separate PRs)