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 2022/04/15 05:31:57 UTC

[GitHub] [kafka] dengziming opened a new pull request, #12053: MINOR: Displaying default configs better in MetadataShell

dengziming opened a new pull request, #12053:
URL: https://github.com/apache/kafka/pull/12053

   *More detailed description of your change*
   When debugging some bugs related to configs, I find we are unable to show default broker/topic configs since the resourceName="".
   I changed it to <default> similar to how we trait default client quotas.
   
   *Summary of testing strategy (including rationale)*
   Unit test
   
   ### 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] dengziming commented on pull request #12053: MINOR: Displaying default configs better in MetadataShell

Posted by GitBox <gi...@apache.org>.
dengziming commented on PR #12053:
URL: https://github.com/apache/kafka/pull/12053#issuecomment-1099869359

   Change output from:
   ```
   [ Kafka Metadata Shell ]
   >> ls /configs/broker
   
   >> 
   ```
   
   to
   ```
   [ Kafka Metadata Shell ]
   >> ls /configs/broker/
   <default>
   >> 
   ```
   
   Hello @cmccabe , PTAL.


-- 
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] showuon commented on pull request #12053: MINOR: Displaying default entity name better in MetadataShell

Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12053:
URL: https://github.com/apache/kafka/pull/12053#issuecomment-1231098370

   backport to 3.3 since it's related to Kraft metadata shell. cc @jsancio 


-- 
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] showuon commented on pull request #12053: MINOR: Displaying default configs better in MetadataShell

Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12053:
URL: https://github.com/apache/kafka/pull/12053#issuecomment-1231096214

   Failed tests are unrelated:
   ```
   Build / JDK 11 and Scala 2.13 / kafka.api.TransactionsExpirationTest.testTransactionAfterProducerIdExpires(String).quorum=kraft
   ```


-- 
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] dengziming commented on a diff in pull request #12053: MINOR: Displaying default configs better in MetadataShell

Posted by GitBox <gi...@apache.org>.
dengziming commented on code in PR #12053:
URL: https://github.com/apache/kafka/pull/12053#discussion_r957001101


##########
shell/src/main/java/org/apache/kafka/shell/MetadataNodeManager.java:
##########
@@ -268,7 +269,7 @@ private void handleCommitImpl(MetadataRecordType type, ApiMessage message)
                             "Can't handle ConfigResource.Type " + record.resourceType());
                 }
                 DirectoryNode configDirectory = data.root.mkdirs("configs").
-                    mkdirs(typeString).mkdirs(record.resourceName());
+                    mkdirs(typeString).mkdirs(Utils.isBlank(record.resourceName()) ? "<default>" : record.resourceName());

Review Comment:
   You are right, we should not allow a null value here, btw, I think `isBlank` is not right, we should use `isEmpty` here.



-- 
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] showuon commented on a diff in pull request #12053: MINOR: Displaying default configs better in MetadataShell

Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12053:
URL: https://github.com/apache/kafka/pull/12053#discussion_r956926934


##########
shell/src/main/java/org/apache/kafka/shell/MetadataNodeManager.java:
##########
@@ -268,7 +269,7 @@ private void handleCommitImpl(MetadataRecordType type, ApiMessage message)
                             "Can't handle ConfigResource.Type " + record.resourceType());
                 }
                 DirectoryNode configDirectory = data.root.mkdirs("configs").
-                    mkdirs(typeString).mkdirs(record.resourceName());
+                    mkdirs(typeString).mkdirs(Utils.isBlank(record.resourceName()) ? "<default>" : record.resourceName());

Review Comment:
   nit: `Utils.isBlank` will be true when `str == null || str.trim().isEmpty()`. Although I don't think the resource name will be null here, I think it'd better not to allow null resource name here. So, maybe:
   ```
   record.resourceName().isBlank() ? "<default>" : record.resourceName()
   ```



-- 
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] showuon commented on a diff in pull request #12053: MINOR: Displaying default configs better in MetadataShell

Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12053:
URL: https://github.com/apache/kafka/pull/12053#discussion_r957008851


##########
shell/src/main/java/org/apache/kafka/shell/MetadataNodeManager.java:
##########
@@ -268,7 +269,7 @@ private void handleCommitImpl(MetadataRecordType type, ApiMessage message)
                             "Can't handle ConfigResource.Type " + record.resourceType());
                 }
                 DirectoryNode configDirectory = data.root.mkdirs("configs").
-                    mkdirs(typeString).mkdirs(record.resourceName());
+                    mkdirs(typeString).mkdirs(Utils.isBlank(record.resourceName()) ? "<default>" : record.resourceName());

Review Comment:
   Make sense!



-- 
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] showuon merged pull request #12053: MINOR: Displaying default configs better in MetadataShell

Posted by GitBox <gi...@apache.org>.
showuon merged PR #12053:
URL: https://github.com/apache/kafka/pull/12053


-- 
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