-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-17051: ApiKeys#toHtml should exclude the APIs having unstable latest version #16480
Conversation
@@ -332,6 +332,7 @@ public static EnumSet<ApiKeys> controllerApis() { | |||
public static EnumSet<ApiKeys> clientApis() { | |||
List<ApiKeys> apis = Arrays.stream(ApiKeys.values()) | |||
.filter(apiKey -> apiKey.inScope(ApiMessageType.ListenerType.ZK_BROKER) || apiKey.inScope(ApiMessageType.ListenerType.BROKER)) | |||
.filter(apiKey -> !apiKey.messageType.latestVersionUnstable()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't add this change to clientApis
since it is used by other callers. Please add the filter into toHtml
. Also, please have test for it
@chia7712, Thanks for your review, PTAL |
@chia7712, Thanks for your comments, PTAL |
docs/protocol.html
Outdated
@@ -200,7 +200,7 @@ <h5 class="anchor-heading"><a id="protocol_error_codes" class="anchor-link"></a> | |||
<!--#include virtual="generated/protocol_errors.html" --> | |||
|
|||
<h5 class="anchor-heading"><a id="protocol_api_keys" class="anchor-link"></a><a href="#protocol_api_keys">Api Keys</a></h5> | |||
<p>The following are the numeric codes that the ApiKey in the request can take for each of the below request types.</p> | |||
<p>The following are the numeric codes that the ApiKey which last version is stable in the request can take for each of the below request types.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe stable ApiKey
is good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, If ApiKey only have last version, I agree that use stable ApiKey
is good enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1a2st thanks for your patch
String html = ApiKeys.toHtml(); | ||
for (ApiKeys apiKeys : ApiKeys.clientApis()) { | ||
if (apiKeys.messageType.latestVersionUnstable()) { | ||
assertFalse(html.contains(apiKeys.name), "Html should not contain unstable api: " + apiKeys.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use "The_Messages_" + apiKeys.name
? that is more accurate. If the api name is "too" normal, it could be used by other APIs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestion, I updated the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…test version (apache#16480) Reviewers: Chia-Ping Tsai <[email protected]>
…test version (apache#16480) Reviewers: Chia-Ping Tsai <[email protected]>
https://issues.apache.org/jira/browse/KAFKA-17051
we should exclude those APIs having make "latestVersionUnstable=true".
Committer Checklist (excluded from commit message)