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 2020/05/04 09:35:47 UTC
[GitHub] [kafka] d8tltanc opened a new pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
d8tltanc opened a new pull request #8610:
URL: https://github.com/apache/kafka/pull/8610
*More detailed description of your change,
The client-side passes an empty string "" to the server controller indicating that the quota changes apply to the default entity. However, the server logic fails to convert the empty string to "<default>", which will lead to the query for zk node "/config/" and cause an exception.
*Summary of testing strategy (including rationale)
Tested all the failed commands in the corresponding Jira ticket, except alter the default quota configs for entity type user, as the API hasn't been implemented.
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] d8tltanc commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-623785708
Thanks @cmccabe for the initial review. I'll remove all the warning logs after the tests pass.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bdbyrne edited a comment on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
bdbyrne edited a comment on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624225725
So I believe the error to be here:
https://github.com/apache/kafka/pull/8610/files#diff-faf8cea6a3a0fab5e056ad5fee22ff3eR369-R375
(In case the link doesn't work, it's where the ConfigCommand constructs the ClientQuotaEntity.)
It should be translating the default entity type into `null`, but it's actually passing the name along as-is. The server-side logic should be correct.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-623768582
Thanks for this, @d8tltanc . It seems like there are still some debug log message left in?
Also, we definitely need a test for this one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] d8tltanc edited a comment on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
d8tltanc edited a comment on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624421790
I think `ClientQuotaEntity` is only used in client quota alternation. But the description doesn't use that.
Update: I've pushed the my latest local changes if that helps
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] d8tltanc commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-623863462
Two tests fail because they assume that an empty string is a bad client / user id. I deleted them. Can I get an “ok to test” again? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] d8tltanc edited a comment on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
d8tltanc edited a comment on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624308593
Hi @bdbyrne , thanks for the comment. The link seems not working. I guess you mean this part we should replace the empty string "" by null?
> private[admin] def entityNames(): List[String] = {
> val namesIterator = options.valuesOf(entityName).iterator
> options.specs.asScala
> .filter(spec => spec.options.contains("entity-name") || spec.options.contains("entity-default"))
> .map(spec => if (spec.options.contains("entity-name")) namesIterator.next else "").toList ++
> entityFlags
> .filter(entity => options.has(entity._1))
> .map(entity => options.valueOf(entity._1)) ++
> entityDefaultsFlags
> .filter(entity => options.has(entity._1))
> .map(_ => "")
> }
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bdbyrne commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
bdbyrne commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624318905
> > Hi @bdbyrne , thanks for the comment. The link seems not working. I guess you mean this part we should replace the empty string "" by null?
> > > ```
> > > private[admin] def entityNames(): List[String] = {
> > > val namesIterator = options.valuesOf(entityName).iterator
> > > options.specs.asScala
> > > .filter(spec => spec.options.contains("entity-name") || spec.options.contains("entity-default"))
> > > .map(spec => if (spec.options.contains("entity-name")) namesIterator.next else "").toList ++
> > > entityFlags
> > > .filter(entity => options.has(entity._1))
> > > .map(entity => options.valueOf(entity._1)) ++
> > > entityDefaultsFlags
> > > .filter(entity => options.has(entity._1))
> > > .map(_ => "")
> > > }
> > > ```
>
> That's correct, yes. You could do it in the provided code, however keep in mind that the ZK path currently uses it as well and would expect the empty string. The alternative is to do the substitution (`"" -> null`) when creating the `ClientQuotaEntity`, which is exclusive to the `AdminClient` path.
Oh wait, there's also other places where the default is used and expected to be the empty string (broker config), so it may be best to only make the change at the `ClientQuotaEntity`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] d8tltanc edited a comment on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
d8tltanc edited a comment on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624308593
Hi @bdbyrne , thanks for the comment. The link seems not working. I guess you mean this part we should replace the empty string "" by null?
private[admin] def entityNames(): List[String] = {
val namesIterator = options.valuesOf(entityName).iterator
options.specs.asScala
.filter(spec => spec.options.contains("entity-name") || spec.options.contains("entity-default"))
.map(spec => if (spec.options.contains("entity-name")) namesIterator.next else "").toList ++
entityFlags
.filter(entity => options.has(entity._1))
.map(entity => options.valueOf(entity._1)) ++
entityDefaultsFlags
.filter(entity => options.has(entity._1))
.map(_ => "")
}
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#discussion_r419794980
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -473,10 +474,11 @@ object ConfigCommand extends Config {
}
private def describeClientQuotasConfig(adminClient: Admin, entityTypes: List[String], entityNames: List[String]) = {
+ warn(s"entityNames = $entityNames")
getAllClientQuotasConfigs(adminClient, entityTypes, entityNames).foreach { case (entity, entries) =>
val entityEntries = entity.entries.asScala
- val entityStr = (entityEntries.get(ClientQuotaEntity.USER).map(u => s"user-principal '${u}'") ++
- entityEntries.get(ClientQuotaEntity.CLIENT_ID).map(c => s"client-id '${c}'")).mkString(", ")
+ val entityStr = (entityEntries.get(ClientQuotaEntity.USER).map(u => s"user-principal '${Option(u).filterNot(_.isEmpty).getOrElse("default")}'") ++
Review comment:
this is clever, but I'd really rather have a simple `if (u.isEmpty) "default" else u`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-626324149
resolved by #8628
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] d8tltanc edited a comment on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
d8tltanc edited a comment on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624421790
I think `ClientQuotaEntity` is only used in client quota alternation. But the description doesn't use that.
Update: I've pushed my latest local changes if that helps. My patch is assuming the server is treating `match` value null as the default. The original design might be assuming `match` value Optional.empty() as the default and there might be some inconsistencies. The failed tests are **testDescribeClientQuotasMatchExact** and **testDescribeClientQuotasMatchPartial**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#discussion_r419793778
##########
File path: core/src/main/scala/kafka/zk/KafkaZkClient.scala
##########
@@ -329,11 +329,13 @@ class KafkaZkClient private[zk] (zooKeeperClient: ZooKeeperClient, isSecure: Boo
def getEntityConfigs(rootEntityType: String, sanitizedEntityName: String): Properties = {
val getDataRequest = GetDataRequest(ConfigEntityZNode.path(rootEntityType, sanitizedEntityName))
val getDataResponse = retryRequestUntilConnected(getDataRequest)
-
+ warn(s"rootEntityType = $rootEntityType sanitizedEntityName = $sanitizedEntityName")
Review comment:
should be removed, right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] d8tltanc commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624421790
I think `ClientQuotaEntity` is only used in client quota alternation. But the description doesn't use that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bdbyrne commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
bdbyrne commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624225725
So I believe the error to be here:
https://github.com/apache/kafka/pull/8610/files#diff-faf8cea6a3a0fab5e056ad5fee22ff3eR369-R375
It should be translating the default entity type into `null`, but it's actually passing the name along as-is. The server-side logic should be correct.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] d8tltanc commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
d8tltanc commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624308593
Hi @bdbyrne , thanks for the comment. The link seems not working. I guess you mean this part we should replace the empty string "" by null?
` private[admin] def entityNames(): List[String] = {
val namesIterator = options.valuesOf(entityName).iterator
options.specs.asScala
.filter(spec => spec.options.contains("entity-name") || spec.options.contains("entity-default"))
.map(spec => if (spec.options.contains("entity-name")) namesIterator.next else "").toList ++
entityFlags
.filter(entity => options.has(entity._1))
.map(entity => options.valueOf(entity._1)) ++
entityDefaultsFlags
.filter(entity => options.has(entity._1))
.map(_ => "")
}`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bdbyrne commented on pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
bdbyrne commented on pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#issuecomment-624318293
> Hi @bdbyrne , thanks for the comment. The link seems not working. I guess you mean this part we should replace the empty string "" by null?
>
> > ```
> > private[admin] def entityNames(): List[String] = {
> > val namesIterator = options.valuesOf(entityName).iterator
> > options.specs.asScala
> > .filter(spec => spec.options.contains("entity-name") || spec.options.contains("entity-default"))
> > .map(spec => if (spec.options.contains("entity-name")) namesIterator.next else "").toList ++
> > entityFlags
> > .filter(entity => options.has(entity._1))
> > .map(entity => options.valueOf(entity._1)) ++
> > entityDefaultsFlags
> > .filter(entity => options.has(entity._1))
> > .map(_ => "")
> > }
> > ```
That's correct, yes. You could do it in the provided code, however keep in mind that the ZK path currently uses it as well and would expect the empty string. The alternative is to do the substitution (`"" -> null`) when creating the `ClientQuotaEntity`, which is exclusive to the `AdminClient` path.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #8610: KAKFA-9942: --entity-default flag is not working for alternating / describing configs in AdminClient
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8610:
URL: https://github.com/apache/kafka/pull/8610#discussion_r419793865
##########
File path: core/src/main/scala/kafka/zk/KafkaZkClient.scala
##########
@@ -329,11 +329,13 @@ class KafkaZkClient private[zk] (zooKeeperClient: ZooKeeperClient, isSecure: Boo
def getEntityConfigs(rootEntityType: String, sanitizedEntityName: String): Properties = {
val getDataRequest = GetDataRequest(ConfigEntityZNode.path(rootEntityType, sanitizedEntityName))
val getDataResponse = retryRequestUntilConnected(getDataRequest)
-
+ warn(s"rootEntityType = $rootEntityType sanitizedEntityName = $sanitizedEntityName")
getDataResponse.resultCode match {
case Code.OK =>
ConfigEntityZNode.decode(getDataResponse.data)
- case Code.NONODE => new Properties()
+ case Code.NONODE =>
+ warn(s"Code.NONODE reached, creating empty properties")
Review comment:
should be removed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org