Are there any other concerns with the proposal or should we start a
VOTE thread?
Eduard
On Wed, Jul 10, 2024 at 5:20 PM Dmitri Bourlatchkov
<dmitri.bourlatch...@dremio.com.invalid> wrote:
Re: remote signing, I agree that it does not look like a
server capability that a client can / should discover. It
is more like something that the server instructs /
configures the client to do.
While a server can control this behavior and instruct the
client to use remote signing, technically nothing is
preventing a client from configuring
s3.remote-signing-enabled=true. In such a case it seems more
appropriate to indicate that this capability isn't supported
rather than a generic 501, because not every server will
support remote signing.
Good point regarding clients taking initiative and using request
singing without an explicit server-provided config. It moves the
client operations into a mode where the server has more control
(over having longer term client-side credentials), so it looks
like a reasonable mode to support from the security perspective.
Let's keep that capability flag.
Cheers,
Dmitri.
On Wed, Jul 10, 2024 at 5:48 AM Eduard Tudenhöfner
<etudenhoef...@apache.org> wrote:
Hey everyone,
I've added a few inline comments below.
Re: remote signing, I agree that it does not look like a
server capability that a client can / should discover. It
is more like something that the server instructs /
configures the client to do.
While a server can control this behavior and instruct the
client to use remote signing, technically nothing is
preventing a client from configuring
s3.remote-signing-enabled=true. In such a case it seems more
appropriate to indicate that this capability isn't supported
rather than a generic 501, because not every server will
support remote signing.
The *vended-credentials* capability on the other hand is more
informative in its nature and a server indeed configures a
client. I think that was also one of the reasons I removed
this capability but added it later back due to a comment from
Jack.
I'm ok either way in terms of removing / keeping
*vended-credentials* as a capability but given that we'd want
to include *actionable* capabilities at this point, I'd just
remove it (nothing is preventing us from adding it later if
necessary).
In that case, why do we need all these other capabilities
like tables, remote-signing, etc. in the first place?
Given that capabilities also carry versioning information,
clients can make more informed decisions on which endpoints to
call. One could argue that generally throwing a 501 on
everything that isn't supported might be sufficient, but that
doesn't necessarily help a client in knowing which versions of
a capability are safe to call/use.
Regarding the control of client-side fallback behavior:
I think the default fallback behavior should be *tables* (with
version 1) with a property in the REST catalog that allows
configuring this to e.g.
*rest-default-capabilities=tables,views,abc,xyz* (all of them
defaulting to version 1).
Eduard
On Tue, Jul 9, 2024 at 7:00 PM Jack Ye <yezhao...@gmail.com>
wrote:
Yes I agree that sounds like a valid use case. So the
criteria so far is that capabilities are used for:
- controlling client-side fallback behavior
- failing expensive operations early if we know it will
eventually fail due to missing capability
Do we agree if this is the criteria we should use? What
about the other capabilities, namly tables,
remote-signing, credential-vending?
-Jack
On Tue, Jul 9, 2024 at 9:27 AM Ryan Blue
<b...@databricks.com.invalid> wrote:
> does it make a difference if I declare the
capability or not?
I think that it does in other cases. Multi-table
commits, for example, are a building block for
multi-statement transactions. If a service doesn't
support multi-table commits then we ideally want
clients to know that ahead of time so that they don't
run a big transaction and then fail because the commit
is not supported.
On Tue, Jul 9, 2024 at 9:12 AM Dmitri Bourlatchkov
<dmitri.bourlatch...@dremio.com.invalid> wrote:
Re: remote signing, I agree that it does not look
like a server capability that a client can /
should discover. It is more like something that
the server instructs / configures the client to do.
Cheers,
Dmitri.
On Tue, Jul 9, 2024 at 12:05 PM Jack Ye
<yezhao...@gmail.com> wrote:
I was reconciling the discussion yesterday,
one point that was interesting to me was that
we agreed the purpose of these capabilities is
to "control client-side fallback behavior", or
at least the client should behave differently
based on these capabilities. However, this
seems to be only needed so far for views, or
more specifically, for loadView API only
because it impacts the fallback behavior to
resolve the identifier as a table or not.
For all the other capabilities listed, and
even the other endpoints in view, because a
server can decide to implement it partially
anyway and just document the behavior, does it
make a difference if I declare the capability
or not? The client will not stop the request,
the server will just error out if it is not
supported. Maybe the error is not in the
expected code or message, but it is still an
error. In that case, why do we need all these
other capabilities like tables,
remote-signing, etc. in the first place?
Maybe it is too extreme of a thought, but
could anyone help describe how the other
capabilities could be used beyond potentially
returning an error earlier?
-Jack
On Tue, Jul 9, 2024 at 8:02 AM Dmitri
Bourlatchkov
<dmitri.bourlatch...@dremio.com.invalid> wrote:
Hi Eduard,
> I've also added the 501 error to the
response of the respective endpoints but
worth mentioning that *HEAD* / *GET
*requests must not return a 501
<https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501> (this
implies that the server impl would e.g.
return a *404* in such a case).
My reading on the Mozilla page makes me
think that it is phrased too narrowly.
Reading RFC 2616 [1] I believe that it
does not preclude responding with 501 to
GET and HEAD requests. I think it means
that GET and HEAD methods must be
supported by "general purpose" servers.
The Iceberg REST server is not a general
purpose server for resources. So, I think
it should be fine to respond with 501 to
unimplemented endpoints.
Cheers,
Dmitri.
[1]
https://www.rfc-editor.org/rfc/rfc2616#section-5.1.1
On Tue, Jul 9, 2024 at 9:44 AM Eduard
Tudenhöfner <etudenhoef...@apache.org> wrote:
Hey everyone,
I watched the catalog sync recording
today and updated the PR
<https://github.com/apache/iceberg/pull/9940>
to remove fine-grained capabilities
like *register-table / table-metrics*.
The current capabilities (with
versioning information) in the PR are:
* tables
* views
* remote-signing
* vended-credentials
* multi-table-commit
For servers that only
*partially* implement endpoints under
a capability the spec requires the
server to throw a *501 Not
Implemented*. I've also added the 501
error to the response of the
respective endpoints but worth
mentioning that *HEAD* / *GET
*requests must not return a 501
<https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501> (this
implies that the server impl would
e.g. return a *404* in such a case).
Regards
Eduard
On Thu, Jul 4, 2024 at 3:59 PM
Jean-Baptiste Onofré <j...@nanthrax.net>
wrote:
Hi Eduard,
It makes sense to return 501 for
servers which don't implement all
endpoints. It means that the
server will at least have to implement
empty endpoints if needed (that
makes sense to me).
I think we should focus on only
"identified capabilities". I think
that I proposed before that the
capabilities can be
overridden/provided by server
implementation. Else, I'm afraid we
won't be flexible enough or always
behind the implementation (if an
implementation wants to add
"my-foo-cap").
Regards
JB
On Thu, Jul 4, 2024 at 9:32 AM
Eduard Tudenhöfner
<etudenhoef...@apache.org> wrote:
>
> I have clarified the wording in
#9940 around the requirement on
having to implement all endpoints
under a particular capability.
>
> For servers that only partially
implement endpoints under a
capability the spec requires the
server to throw a 501 Not
Implemented. This was suggested by
Jack and it seems reasonable to do
that.
>
> Regarding the inclusion of
table-spec / view-spec as a
capability: I think this might
make sense for the next iteration
of the REST spec but as I
mentioned earlier I don't see any
clear benefit for the current REST
spec as the client wouldn't do
anything with that information.
> If there is a clear benefit of
having this, then this can still
be added later to the current REST
spec but I believe we should
rather have a few well-defined and
actionable capabilities rather
than too many.
>
> Eduard
>
> On Wed, Jul 3, 2024 at 5:44 AM
Renjie Liu
<liurenjie2...@gmail.com> wrote:
>>>
>>> Spec is an interesting topic
we did not discuss. Robert, how do
you envision this to be used?
>>> In my mind, if a new table
format v3 is launched, there are 2
approaches we can go with, taking
CreateTable as an example:
>>> (1) increment the related
operation version, which means
that POST
/v2/{prefix}/namespaces/{ns}/tables
will be created and allow creating
tables in the v3 version.
>>> (2) update the existing table
metadata model to support both v2
and v3 fields, and the server
enforces the payload differently
based on the
TableMetadata.format-version
field. If the server does not
support v3, it can return
unsupported at that time.
>>> Either way we go, the
table-spec version does not need
to be a capability. (1) seems to
be cleaner, but has some overhead
in provisioning a new endpoint
compared to (2).
>>> Do you see another way to do
this leveraging the table-spec
version?
>>
>>
>> 2 is cleaner but maybe
inconsistent with current
behavior, since /v1/tables
operation supports both v1 and v3.
We should only go to 2 only when
we have incompatible fields/break
changes according to discussion.
>>
>> Generally I agree with adding
table-spec into capabilities. For
example, we can expose this to
user in api so that user could
choose a supported table format
version without throwing exception.
>>
>> On Wed, Jul 3, 2024 at 12:18 AM
Jack Ye <yezhao...@gmail.com> wrote:
>>>
>>> Spec is an interesting topic
we did not discuss. Robert, how do
you envision this to be used?
>>>
>>> In my mind, if a new table
format v3 is launched, there are 2
approaches we can go with, taking
CreateTable as an example:
>>>
>>> (1) increment the related
operation version, which means
that POST
/v2/{prefix}/namespaces/{ns}/tables
will be created and allow creating
tables in the v3 version.
>>>
>>> (2) update the existing table
metadata model to support both v2
and v3 fields, and the server
enforces the payload differently
based on the
TableMetadata.format-version
field. If the server does not
support v3, it can return
unsupported at that time.
>>>
>>> Either way we go, the
table-spec version does not need
to be a capability. (1) seems to
be cleaner, but has some overhead
in provisioning a new endpoint
compared to (2).
>>>
>>> Do you see another way to do
this leveraging the table-spec
version?
>>>
>>> -Jack
>>>
>>>
>>>
>>>
>>>
>>> On Tue, Jul 2, 2024 at 6:03 AM
Eduard Tudenhöfner
<eduard.tudenhoef...@databricks.com.invalid>
wrote:
>>>>
>>>>
>>>> I couldn't make it to the
catalog sync meeting yesterday but
I watched the recording today
(thanks for providing that).
>>>>
>>>>> The missing piece is how
(new, capabilities-aware) clients
handle the case when a service
does _not_ return the capabilities
field (absent). My proposal would
be that a client should in this
case assume that all _currently_
existing capabilities are supported.
>>>>>
>>>>> - tables: [1]
>>>>> - views: [1]
>>>>> - remote-signing: [1]
>>>>> - multi-table-commit: [1]
>>>>> - register-table: [1]
>>>>> - table-metrics: [1]
>>>>> - table-spec: [1,2]
>>>>> - view-spec: [1,2]
>>>>>
>>>>>
>>>> The one thing I would like to
add here is that the current PR
uses the tables capability (as
version 1) as the default when a
server doesn't return capabilities
but it might be also ok to include
views (as version 1) because the
current client impl has some code
to deal with errors in case
endpoints don't exist.
>>>>
>>>> Unless we agree that the
currently existing functionality
in the REST spec is the default
behavior to be assumed for older
server, I'm not sure about
including remote-signing /
multi-table-commit /
register-table / table-metrics as
it has been indicated in earlier
comments on the PR/ML that not
every REST server supports these.
>>>>
>>>> That being said, we should
discuss whether we want the
default behavior (when an older
server doesn't send back
capabilities) to be
>>>> a) tables (version 1) only
>>>> b) the currently existing
functionality as defined in the
REST spec (as version 1)
>>>>
>>>>
>>>> On another note: Including
table-spec / view-spec seems to be
more informative in its nature as
I don't think a client would act
differently right now when seeing
these.
>>>>
>>>> Thanks
>>>> Eduard
>>>>
--
Ryan Blue
Databricks
