MINOR: Replace ApiVersion by auto-generated protocol#9746
MINOR: Replace ApiVersion by auto-generated protocol#9746chia7712 merged 4 commits intoapache:trunkfrom
Conversation
chia7712
changed the title
chia7712
force-pushed
the
MINOR-9746
branch
2 times, most recently
from
9732583 to
fc66801
Compare
chia7712
changed the title
|
@chia7712 Sorry for the delay. I will take a look at it next week. |
|
fix conflicting files |
chia7712
force-pushed
the
MINOR-9746
branch
2 times, most recently
from
b53bb93 to
81dd7d6
Compare
|
rewrite whole PR due to recent changes ... |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Thanks for the PR. Left a few minor comments.
|
|
||
| public static Optional<ApiVersionsResponseData.ApiVersion> intersect(ApiVersionsResponseData.ApiVersion thisVersion, | ||
| ApiVersionsResponseData.ApiVersion other) { | ||
| if (other == null) return Optional.empty(); |
There was a problem hiding this comment.
It's a bit weird that we only check if other is null. Do we need this check at all?
There was a problem hiding this comment.
From the usage of this method, only other can be null. So current check is enough.
There was a problem hiding this comment.
That's not a good way to design apis, right? This kind of thing should be symmetric. Why is other sometimes null?
There was a problem hiding this comment.
Why is other sometimes null?
The api key is supported by client-side but Controller doesn't.
That's not a good way to design apis, right? This kind of thing should be symmetric.
make sense. will address it and add unit tests.
|
Also, one file has conflicts that need to be resolved. |
3 participants
This PR includes following changes.
ApiVersionsResponseKeytoApiVersionApiVersionCommitter Checklist (excluded from commit message)