Drop GetSupportedVersions in favor of versioned proto package #204
Conversation
This patch also removes the `GetSupportedVersions` and per request Version field because of the newly introduced version in the package name.
|
@saad-ali @julian-hj @cpuguy83 PTAL I squashed and rebased. Squash is because there are too many conflicts. cc @jdef |
|
LGTM |
|
Wow, this seems drastic and potentially harmful. Why remove this RPC that is in fact quite useful? API changes cannot be completely dictated by the potential language bindings. While I agree that changes to support bindings are good, removing a feature because a specific binding may not utilize is in fact removing that utility for all languages that can make use of it. A CSI endpoint could be in any language and so could a client. A client, for example, that supports multiple API versions. How is a client now able to query the endpoint about what versions are supported? That is completely gone? This is not a good change. Even post 1.0 there will be breaking changes going forward, and now there is simply no way to know what an endpoint supports. While it's a good idea to version the proto package, the ability to query what versions are supported was fundamental. I'm gone for one meeting and this happens? sigh :) Also, I've mentioned this several times to no avail, but I'll try again. Why was the following omitted? option go_package = "csi";
option java_package="csi";
option csharp_namespace="csi";This is frustrating. I recommended this change in #195 and it was ignored. It was recommended and ignored offline as well. Recommendations deserve at least an acknowledgement, even if they are ultimately declined. |
|
Hi @jieyu, Also, please simplify the packaging path from |
|
Hi @jieyu, I am really concerned that you neglected to separate the CSI proto/packaging changes and the RPC/Version message changes into multiple commits. I would see the latter completely reverted as it serves no discernible purpose as far as I can tell. However it also requires reverting the changes of value in this PR. I think everyone in this project could do well to start paying more attention to good Git practice. There have been a few PRs lately where commits should be separate. |
|
@akutz we removed it with the intention of working towards version negotiation. We didn't feel the existing rpc was good enough and wanted to lump the breaking change into 0.2 and add something else (or potentially even the same thing, who knows) as an additive/non-breaking change before 1.0. |
|
Hi @cpuguy83,
Ah. That aspect of your discussion is not represented clearly in the PR description. Thank you very much. That makes sense. |
|
Hi @cpuguy83 / @jieyu / @saad-ali, I just want to add that all other reductive changes to strip the spec down should be managed ASAP. The fact is that these changes have broad, downstream impact. Entire types are removed, rendering libraries and programs unable to even compile until refactoring occurs. |
|
Hi @cpuguy83, One quick update on your comment regarding version negotiation. Last year I recommended moving the version negotiation to the gRPC metadata:
This change would allow multiple versions of the API to still handle version negotiation. There is a reason that this method is quite common in HTTP-based APIs. Headers are not bound to the API model and are useful for handling contract negotiation. cc @saad-ali |
|
I thought that I explained why I hadn't included the option package name
declarations for java, etc. Maybe that was on another thread. Reason was
because we don't host bindings for those langs yet and can't see/test the
impact of such lang specific options in the repo.
That said, I'm open to the idea of the spec repo hosting bindings for other
langs.
…On Feb 25, 2018 1:48 PM, "Andrew Kutz" ***@***.***> wrote:
Wow, this seems drastic and potentially harmful.
Why remove this RPC that is in fact quite useful? API changes cannot be
completely dictated by the potential language bindings. While I agree that
changes to support bindings are good, removing a feature because a specific
binding may not utilize is in fact removing that utility for all languages
that *can* make use of it.
A CSI endpoint could be in any language and so could a client. A client,
for example, that supports multiple API versions. How is a client now able
to query the endpoint about what versions are supported? That is completely
gone? This is not a good change. Even post 1.0 there will be breaking
changes going forward, and now there is simply no way to know what an
endpoint supports.
While it's a good idea to version the proto package, the ability to query
what versions are supported was fundamental. I'm gone for one meeting and
this happens? *sigh* :)
Also, I've mentioned this several times to no avail, but I'll try again.
Why was the following omitted?
option go_package = "csi";option java_package="csi";option csharp_namespace="csi";
This is frustrating. I recommended this change in #195
<#195> and it was
ignored. It was recommended and ignored offline as well. Recommendations
deserve at least an acknowledgement, even if they are ultimately ignored.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#204 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLJm3Ixnabb35wa4TKdW7A0zQdBc5ks5tYasPgaJpZM4SRkl4>
.
|
|
Hi @jdef, If you did then I missed it. It’s likely you did, and I just overlooked your response. Thank you, and while I agree with the reasoning, if you add the suggested options then people consuming the proto in its raw form won’t have to modify it to get the equivalent package name. However, I see your viewpoint as well. |
|
Let’s rememeber that the proto isn’t bound to the rules/results of this repo or what it hosts, but rather that the proto should assume anyone may wish to consume it and generate language bindings for any language. |
|
Also, java package names use reverse domain name notation, I'm not sure
that "csi" is appropriate.
And .NET has its own set of rules for namespaces:
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-namespaces
-- again, I'm not sure that "csi" is appropriate there either.
…On Sun, Feb 25, 2018 at 7:26 PM, Andrew Kutz ***@***.***> wrote:
Let’s rememeber that the proto isn’t bound to the rules/results of this
repo or what it hosts, but rather that the proto should assume anyone may
wish to consume it and generate language bindings for any language.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#204 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLMu22tkFav6lYq5f_3dhTdBupiqSks5tYfo8gaJpZM4SRkl4>
.
|
|
Additionally, Java doesn't support package aliasing (and neither do all
.NET langs) so I'm not convinced that we should use any package naming
options for those platforms.
On Mon, Feb 26, 2018 at 11:05 AM, James DeFelice <[email protected]>
wrote:
… Also, java package names use reverse domain name notation, I'm not sure
that "csi" is appropriate.
And .NET has its own set of rules for namespaces: https://docs.
microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-namespaces
-- again, I'm not sure that "csi" is appropriate there either.
On Sun, Feb 25, 2018 at 7:26 PM, Andrew Kutz ***@***.***>
wrote:
> Let’s rememeber that the proto isn’t bound to the rules/results of this
> repo or what it hosts, but rather that the proto should assume anyone may
> wish to consume it and generate language bindings for any language.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#204 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACPVLMu22tkFav6lYq5f_3dhTdBupiqSks5tYfo8gaJpZM4SRkl4>
> .
>
|
|
I'm aware of each. i figured it was just the final path component like Go since Java's notation is filesystem based as well as .NET's. |
|
I'm also a bit sad that the commit stream was squashed for this. Let's try to do better going forward. |
|
Hi @jdef, So I'm blaming lack of sleep, because I have six years of experience with .NET (C#) and five-ish with Java, and I apparently completely forgot both required the full namespace path at the top of their respective source files. I even double checked my own sources to verify. I thought both were completely file-system based like Go, with each source reflecting the name of the contextual namespace component. FWIW, both are like that with respect to project directory structure. Not that it matters. I'm just saying that must have been what I was considering given my recent tenure with Go. Damn. (take a step away from the laptop Andrew) Thanks for doing the due diligence on this I neglected to do. |
As per the CSI discussion today regarding versioning:
Versionedservice later if needed; doing it now isn't critical and it will be hard to remove later on if we decide we don't need it.Version version = 1field specsbased on and supersedes #202