Thank you Eduard for sharing this version of the proposal. Looks simple,
functional, and extensible.
On Thu, Aug 15, 2024 at 1:10 PM Ryan Blue <b...@databricks.com.invalid>
wrote:
> I think I'm fine either way. I lean toward the simplicity of the strings
> in the proposal but would not complain if we went with Yufei's suggestion.
>
> On Thu, Aug 15, 2024 at 12:12 PM Yufei Gu <flyrain...@gmail.com> wrote:
>
>> The current proposal lists endpoints as plain strings, and I still
>> believe we could make things a bit smoother by adding some structure to
>> them. Here's the example if the previous one throws you off.
>>
>> *Before:*
>>
>> "GET /v1/{prefix}/namespaces","POST /v1/{prefix}/namespaces","GET
>> /v1/{prefix}/namespaces/{namespace}","DELETE
>> /v1/{prefix}/namespaces/{namespace}"
>>
>> *After:*
>>
>> {"/v1/{prefix}/namespaces": { "GET": {}, "POST": {} }
>> },{"/v1/{prefix}/namespaces/{namespace}": { "GET": {}, "DELETE": {} } }
>>
>> I think this approach would make the definitions more machine-readable
>> and easier to expand in the future. It's always good to plan for potential
>> changes, and this structure should help us adapt without much hassle.
>> Plus, we could use a trimmed OpenAPI schema for this, so we wouldn’t need
>> to create a new schema from scratch.
>>
>> Yufei
>>
>>
>> On Thu, Aug 15, 2024 at 11:02 AM Jack Ye <yezhao...@gmail.com> wrote:
>>
>>> > But I propose to use a trimmed openAPI's format directly.
>>> Looking at the example, this feels quite complicated to me.
>>>
>>> > For example, it is easier if we want to include operationID
>>> I don't think we need to consider accommodating both, since operationId
>>> is an alternative to "<HTTP VERB> <resource path from REST spec>".
>>>
>>> > or adding feature flags, or adding parameters
>>> For more advanced feature flags, those could likely directly go to
>>> overrides just like today, but given we limited the scope to service
>>> endpoint discovery, I think that is another discussion later.
>>>
>>> So the current proposed solution from Eduard still feels better to me.
>>> And I think the argument for ambiguity is pretty strong so I am good with
>>> the proposed approach to use "<HTTP VERB> <resource path from REST spec>".
>>>
>>> -Jack
>>>
>>>
>>>
>>>
>>>
>>> On Thu, Aug 15, 2024 at 9:46 AM Yufei Gu <flyrain...@gmail.com> wrote:
>>>
>>>> +1 for the proposal. In terms of the format, the current solution is
>>>> simple enough. But I propose to use a trimmed openAPI's format directly. It
>>>> won't add much cost as we can just take the minimum fields we want. But it
>>>> opens a window to extend it in the future. For example, it is easier if we
>>>> want to include operationID, or adding feature flags, or adding parameters.
>>>> Here is an example:
>>>>
>>>> {
>>>>
>>>> "resources": [
>>>>
>>>> {
>>>>
>>>> "/v1/{prefix}/namespaces":
>>>>
>>>> {
>>>>
>>>> "GET": {
>>>>
>>>> "description": "List all namespaces"
>>>>
>>>> }
>>>>
>>>> },
>>>>
>>>> {
>>>>
>>>> "POST": {
>>>>
>>>> "description": "Create a new namespace"
>>>>
>>>> }
>>>>
>>>> }
>>>>
>>>> },
>>>>
>>>> {
>>>>
>>>> "path2": {}
>>>>
>>>> }
>>>>
>>>> ...
>>>>
>>>> ]
>>>>
>>>> }
>>>>
>>>>
>>>> Yufei
>>>>
>>>>
>>>> On Thu, Aug 15, 2024 at 8:47 AM Russell Spitzer <
>>>> russell.spit...@gmail.com> wrote:
>>>>
>>>>> I'm on board for this proposal. I was in the off-mail chats and I
>>>>> think this is probably our simplest approach going forward.
>>>>>
>>>>> On Thu, Aug 15, 2024 at 10:39 AM Dmitri Bourlatchkov
>>>>> <dmitri.bourlatch...@dremio.com.invalid> wrote:
>>>>>
>>>>>> OpenAPI tool will WARN a lot if Operation IDs overlap. Generated
>>>>>> code/html may also look odd in case of overlaps.
>>>>>>
>>>>>> All-in-all, I think the best practice is to define unique Operation
>>>>>> IDs up front.
>>>>>>
>>>>>> For Iceberg REST API, the yaml file is the API definition, so it
>>>>>> should not be a problem to ensure that Operation IDs are unique, I guess.
>>>>>>
>>>>>> Cheers,
>>>>>> Dmitri.
>>>>>>
>>>>>> On Thu, Aug 15, 2024 at 11:32 AM Eduard Tudenhöfner <
>>>>>> etudenhoef...@apache.org> wrote:
>>>>>>
>>>>>>> Hey Jack,
>>>>>>>
>>>>>>> thanks for the feedback. I replied in the doc but I can reiterate my
>>>>>>> answer here too: The *path* is unique and required so that feels
>>>>>>> more appropriate than requiring to have an optional *operationId*
>>>>>>> in the OpenAPI spec.
>>>>>>> Additionally, using the path is more straight-forward when we
>>>>>>> introduce v2 endpoints, while you would have to make sure that all
>>>>>>> *operationIds* are unique across endpoints (and I'm not sure if
>>>>>>> OpenAPI tools actually enforce uniqueness).
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Aug 15, 2024 at 5:20 PM Jack Ye <yezhao...@gmail.com> wrote:
>>>>>>>
>>>>>>>> Hi Eduard,
>>>>>>>>
>>>>>>>> In general I agree with this proposal, thanks for putting this up!
>>>>>>>> Just one question (which I also added in the design), what are the
>>>>>>>> thoughts
>>>>>>>> behind using "<HTTP VERB> <resource path from REST spec>", vs using the
>>>>>>>> operationId defined in the OpenAPI?
>>>>>>>>
>>>>>>>> The operationId approach definitely looks much cleaner to me, but
>>>>>>>> (1) in OpenAPI it is not a requirement to define it, and (2) right now
>>>>>>>> there are some inconsistent operationIds, for example UpdateTable is
>>>>>>>> the
>>>>>>>> operationId, but CommitTable is used for all request and response
>>>>>>>> models.
>>>>>>>> But these are all pretty solvable issues because we can enforce
>>>>>>>> operationId
>>>>>>>> to be required in the Iceberg spec, and fix it to be consistent,
>>>>>>>> assuming
>>>>>>>> nobody is taking a dependency on these operationIds right now.
>>>>>>>>
>>>>>>>> Personally speaking, I am pretty neutral on this topic, but curious
>>>>>>>> what everyone thinks.
>>>>>>>>
>>>>>>>> Best,
>>>>>>>> Jack Ye
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Aug 14, 2024 at 9:20 AM Eduard Tudenhöfner <
>>>>>>>> etudenhoef...@apache.org> wrote:
>>>>>>>>
>>>>>>>>> Hey Dmitri,
>>>>>>>>>
>>>>>>>>> this proposal is the result of the community feedback from the
>>>>>>>>> Capabilities proposal. Ultimately the capabilities turned out to
>>>>>>>>> entail
>>>>>>>>> more complexity than necessary and so this proposal solves the core
>>>>>>>>> problem
>>>>>>>>> while keeping complexity and spec changes to an absolute minimum.
>>>>>>>>>
>>>>>>>>> Eduard
>>>>>>>>>
>>>>>>>>> On Wed, Aug 14, 2024 at 5:15 PM Dmitri Bourlatchkov
>>>>>>>>> <dmitri.bourlatch...@dremio.com.invalid> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Eduard,
>>>>>>>>>>
>>>>>>>>>> How is this proposal related to the Server Capabilities
>>>>>>>>>> discussion?
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Dmitri.
>>>>>>>>>>
>>>>>>>>>> On Wed, Aug 14, 2024 at 5:14 AM Eduard Tudenhöfner <
>>>>>>>>>> etudenhoef...@apache.org> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hey everyone,
>>>>>>>>>>>
>>>>>>>>>>> I'd like to propose a way for REST servers to communicate to
>>>>>>>>>>> clients what endpoints it supports via a new *endpoints* field
>>>>>>>>>>> in the *CatalogConfig* of the *v1/config* endpoint.
>>>>>>>>>>>
>>>>>>>>>>> This enables clients to make better decisions and clearly signal
>>>>>>>>>>> that a particular endpoint isn’t supported.
>>>>>>>>>>>
>>>>>>>>>>> I opened #10937 <https://github.com/apache/iceberg/issues/10937> to
>>>>>>>>>>> track the proposal in GH. Please find the proposal doc here
>>>>>>>>>>> <https://docs.google.com/document/d/1krcIaLfxtBFDABU5ssLmf64zyHgE8BRncpGPIMTWlxo/edit?usp=sharing>
>>>>>>>>>>> (estimated
>>>>>>>>>>> read time: 5 minutes). The proposal requires a Spec change, which
>>>>>>>>>>> can be
>>>>>>>>>>> seen in #10928 <https://github.com/apache/iceberg/pull/10928>.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Eduard
>>>>>>>>>>>
>>>>>>>>>>
>
> --
> Ryan Blue
> Databricks
>
