You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/12/13 19:35:44 UTC
[GitHub] [kafka] mimaison opened a new pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
mimaison opened a new pull request #11599:
URL: https://github.com/apache/kafka/pull/11599
Implements KIP-784
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r793362056
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/DescribeLogDirsResponse.java
##########
@@ -52,6 +52,7 @@ public int throttleTimeMs() {
@Override
public Map<Errors, Integer> errorCounts() {
Map<Errors, Integer> errorCounts = new HashMap<>();
+ errorCounts.put(Errors.forCode(data.errorCode()), 1);
Review comment:
Do we need to cover it in `testErrorCountsIncludesNone`?
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -2681,8 +2681,11 @@ public void handleResponse(AbstractResponse abstractResponse) {
if (descriptions.size() > 0) {
future.complete(descriptions);
} else {
- // descriptions will be empty if and only if the user is not authorized to describe cluster resource.
- future.completeExceptionally(Errors.CLUSTER_AUTHORIZATION_FAILED.exception());
+ // Up to v3 DescribeLogDirsResponse did not have an error code field, hence it defaults to None
+ Errors error = response.data().errorCode() == Errors.NONE.code()
Review comment:
@mimaison What would happen if DescribeLogDirsResponse v3 actually returns NONE as an error (the default value)? It seems to be that we need to take the version into account here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r770373619
##########
File path: clients/src/main/resources/common/message/DescribeLogDirsRequest.json
##########
@@ -19,7 +19,7 @@
"listeners": ["zkBroker", "broker"],
"name": "DescribeLogDirsRequest",
// Version 1 is the same as version 0.
- "validVersions": "0-2",
+ "validVersions": "0-3",
Review comment:
nit: Could we also put a comment for version 3?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r793437543
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -2681,8 +2681,11 @@ public void handleResponse(AbstractResponse abstractResponse) {
if (descriptions.size() > 0) {
future.complete(descriptions);
} else {
- // descriptions will be empty if and only if the user is not authorized to describe cluster resource.
- future.completeExceptionally(Errors.CLUSTER_AUTHORIZATION_FAILED.exception());
+ // Up to v3 DescribeLogDirsResponse did not have an error code field, hence it defaults to None
+ Errors error = response.data().errorCode() == Errors.NONE.code()
Review comment:
With v3, if `NONE` is returned there should be some logdirs in the response so we should have entered the if condition above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#issuecomment-1025549712
@mimaison Should we update `KafkaApis` to correct fill the error code when v3 is used by the client? At the moment, the field is not used at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r770373244
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -2681,8 +2681,11 @@ public void handleResponse(AbstractResponse abstractResponse) {
if (descriptions.size() > 0) {
future.complete(descriptions);
} else {
- // descriptions will be empty if and only if the user is not authorized to describe cluster resource.
- future.completeExceptionally(Errors.CLUSTER_AUTHORIZATION_FAILED.exception());
+ // Up to v3 DescribeLogDirsResponse did not have an error code field, hence it defaults to None
+ Errors error = response.data().errorCode() == Errors.NONE.code()
Review comment:
Should we extend unit tests in `KafkaAdminTest` to cover this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r770769605
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -2681,8 +2681,11 @@ public void handleResponse(AbstractResponse abstractResponse) {
if (descriptions.size() > 0) {
future.complete(descriptions);
} else {
- // descriptions will be empty if and only if the user is not authorized to describe cluster resource.
- future.completeExceptionally(Errors.CLUSTER_AUTHORIZATION_FAILED.exception());
+ // Up to v3 DescribeLogDirsResponse did not have an error code field, hence it defaults to None
+ Errors error = response.data().errorCode() == Errors.NONE.code()
Review comment:
Absolutely, I've updated the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r770769143
##########
File path: clients/src/main/resources/common/message/DescribeLogDirsRequest.json
##########
@@ -19,7 +19,7 @@
"listeners": ["zkBroker", "broker"],
"name": "DescribeLogDirsRequest",
// Version 1 is the same as version 0.
- "validVersions": "0-2",
+ "validVersions": "0-3",
Review comment:
Yes, good idea. Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
mimaison merged pull request #11599:
URL: https://github.com/apache/kafka/pull/11599
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r796347687
##########
File path: core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala
##########
@@ -223,7 +223,7 @@ class AuthorizerIntegrationTest extends BaseRequestTest {
.find(x => x.topicName == tp.topic).get.partitions.asScala
.find(p => p.partitionIndex == tp.partition).get.errorCode)),
ApiKeys.DESCRIBE_LOG_DIRS -> ((resp: DescribeLogDirsResponse) =>
- if (resp.data.results.size() > 0) Errors.forCode(resp.data.results.get(0).errorCode) else Errors.CLUSTER_AUTHORIZATION_FAILED),
+ Errors.forCode(if (resp.data.results.size() > 0) resp.data.results.get(0).errorCode() else resp.data.errorCode())),
Review comment:
nit: I would remove the parenthesis after `errorCode` to stay consistent with the surrounding code style.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r795490547
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -2681,8 +2681,11 @@ public void handleResponse(AbstractResponse abstractResponse) {
if (descriptions.size() > 0) {
future.complete(descriptions);
} else {
- // descriptions will be empty if and only if the user is not authorized to describe cluster resource.
- future.completeExceptionally(Errors.CLUSTER_AUTHORIZATION_FAILED.exception());
+ // Up to v3 DescribeLogDirsResponse did not have an error code field, hence it defaults to None
+ Errors error = response.data().errorCode() == Errors.NONE.code()
Review comment:
That makes sense, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#issuecomment-1025855287
> @mimaison Should we update `KafkaApis` to correct fill the error code when v3 is used by the client? At the moment, the field is not used at all.
@dajac Yes absolutely! I'm not sure why I did not include it earlier. I've just pushed an update
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#issuecomment-1022438866
@dajac Do you want to take another look? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #11599: KAFKA-13527: Add top-level error code field to DescribeLogDirsResponse
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #11599:
URL: https://github.com/apache/kafka/pull/11599#discussion_r793449173
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/DescribeLogDirsResponse.java
##########
@@ -52,6 +52,7 @@ public int throttleTimeMs() {
@Override
public Map<Errors, Integer> errorCounts() {
Map<Errors, Integer> errorCounts = new HashMap<>();
+ errorCounts.put(Errors.forCode(data.errorCode()), 1);
Review comment:
Yes, good point. I've pushed an update to cover it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org