Repository navigation
Conversation
| type: object | ||
| required: | ||
| - uri | ||
| - headers |
There was a problem hiding this comment.
I don't think headers need to be required.
There was a problem hiding this comment.
I agree, but I grandfathered this one in from the singular RemoteSignRequest
I know it is required in RemoteSignRequest for backward compatibility even after introducing remote-presigning (#18080) - do we want to divert away from that here? I was worried that it would be confusing to have different nullability semantic for the two
| description: The storage provider for every element of the batch. If this is not specified, the | ||
| provider is assumed to be `s3`, as in `RemoteSignRequest`. |
There was a problem hiding this comment.
Since this is a new endpoint, we don't need to carry forward this assumption. We should remove it.
There was a problem hiding this comment.
Okay noted - that makes sense
| SignStatus: | ||
| description: Status of signing one element of a batch | ||
| type: string | ||
| enum: ["completed", "failed"] |
There was a problem hiding this comment.
This feels like it introduces an awkward indirection around failure. If the request fails, the response should be a failure otherwise you get Resp[200] -> Failed, which creates confusing response codes.
| A server MAY | ||
| advertise its batch size limit as `sign-batch-max-size` in `CatalogConfig`. If not the value is not advertised, the server | ||
| SHOULD use a default batch size of 100 elements. A batch above the server's limit is rejected with 413. |
There was a problem hiding this comment.
I don't think we want to define the default batch size. This almost feels like something we should add in the remote sign config, so it's not a guess (the service configures the client for the right batch size).
There was a problem hiding this comment.
Okay - should we make the client fail instead if the sign-batch-max-size is not provided by the server when it attempts to use the batch signing API?
| @@ -6050,6 +5807,22 @@ components: | |||
| } | |||
| } | |||
|
|
|||
| ContentTooLargeResponse: | |||
There was a problem hiding this comment.
Maybe make this BatchContentTooLargeResponse so we don't burn a generic name for a specific response.
| - Catalog API | ||
| summary: Remotely signs a batch of requests to object storage | ||
| operationId: batchSignRequests | ||
| description: > |
There was a problem hiding this comment.
| description: > | |
| description: |
nit: don't fold
| A server MAY | ||
| advertise its batch size limit as `sign-batch-max-size` in `CatalogConfig`. If not the value is not advertised, the server |
There was a problem hiding this comment.
nit: strange formatting/line break here
advertise its batch size limit as
sign-batch-max-sizeinCatalogConfig
Should be an added field in RemoteSigningConfiguration (not a catalog config)
There was a problem hiding this comment.
I see what you are saying - max batch signing limit should be a table level configuration, instead of being defined at the catalog scope.
| RemoteSignBatchResponse: | ||
| description: The result of a batch remote request signing operation. | ||
| type: object | ||
| required: | ||
| - results | ||
| properties: | ||
| results: | ||
| description: Results for each element of the request, in request order | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/RemoteSignBatchResult' |
There was a problem hiding this comment.
Do we need to be able to allow partial results? What does it mean to be able to access some of the files? I think we need to decide if that's a reasonable model.
If we do allow this, I would prefer that we separate the successful and failed requests into two separate lists. It's easier to distinguish than a single list that's of mixed types. (Separate is more similar to how S3 do operations like DeleteObjects
There was a problem hiding this comment.
Thanks for the thoughtful review Dan.
Do we need to be able to allow partial results? What does it mean to be able to access some of the files? I think we need to decide if that's a reasonable model.
I have the prior art analysis on the proposal doc, and the dominant pattern I saw was to allow partial failure handling. I observed that all-or-none semantic was mostly used for a batch request that needed an atomic guarantee (e.g. create a batch of items together).
I think handling partial failures would technically allow the client to grab the failed ones, and retry them in the next batch rather than retrying the entire batch.
If we do allow this, I would prefer that we separate the successful and failed requests into two separate lists. It's easier to distinguish than a single list that's of mixed types. (Separate is more similar to how S3 do operations like DeleteObjects
I initially thought keeping it in the same list would work best because we dont have an identifier (a whole RemoteSignRequest uniquely identifies the singular entry), so we are relying on the ordered response to match the requested individual entry to its response.
But I think we can do the same by including the index number in the response - and require that the client map the index in the response to the position of the RemoteSignBatchElement from the original batch request.
Let me update the PR with this change
This branch has not been deployed
2 participants
Introduce a batch signing endpoint, as proposed in https://s.apache.org/irc-batch-signing
To be merged after #18080