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>
<mailto: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>
<mailto: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>
<mailto: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>
<mailto: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>
<mailto: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
