> On July 21, 2015, 1:30 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 71 > > <https://reviews.apache.org/r/34492/diff/8/?file=1017296#file1017296line71> > > > > Disclaimer: I am not claiming that you should change the code commented > > here. > > > > Okay, for getting rid of the dreaded > > ``collection.mutable.HashSet[Acl]()``, you have two options, afaik: > > > > 1. use ``(for (i <- list) yield i).toSet``. In the current code it > > would be something like: > > > > ``` > > val acls = (for (item <- aclSet) { > > val principals: List[KafkaPrincipal] = > > item(PrincipalKey).asInstanceOf[List[String]].map(principal => > > KafkaPrincipal.fromString(principal)) > > val permissionType: PermissionType = > > PermissionType.fromString(item(PermissionTypeKey).asInstanceOf[String]) > > val operations: List[Operation] = > > item(OperationKey).asInstanceOf[List[String]].map(operation => > > Operation.fromString(operation)) > > val hosts: List[String] = item(HostsKey).asInstanceOf[List[String]] > > > > yield new Acl(principals.toSet, permissionType, hosts.toSet, > > operations.toSet) > > }).toSet > > ``` > > > > The surrounding parenthesis around the ``for`` comprehesion are > > important as ``yield`` would return the same Collection type from aclSet (a > > List in this case). > > > > > > 2. To use a (private) helper recursive function like, for example: > > > > ``` > > private def listToSet(list: List[Map[String, Any]]): Set[Acl] = { > > list match { > > case item::tail => { > > > > // L#72 - L#75 processing over `item` > > > > Set(new Acl(...)) ++ listToSet(tail) > > } > > case Nil => Set.empty[Acl] > > } > > } > > ``` > > > > can call it from ``fromJson`` on ``aclSet`` instead of doing a > > ``foreach``. > > > > > > In fact, most of lines L#72 - L#75 are composed of Lists that > > eventually get converted to set (principals, hosts, operations and acls), > > so you could "generify" the helper function above, so that you could pass a > > 'convertion' function, but here I am wary of the complexity of the code > > starting to outweight the benefits (?) of not using mutable data > > structures... Nevertheless, it would look like: > > > > ``` > > def listToSet[T,K](list: List[T], f: T => K): Set[K] = { > > list match { > > case head::tail => Set(f(head)) ++ listToSet(tail, f) > > case Nil => Set.empty[K] > > } > > } > > ```
I haven't changed it for now dont really think to .toSet will be that bad. - Parth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92345 ----------------------------------------------------------- On July 22, 2015, 12:08 a.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated July 22, 2015, 12:08 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2210 > https://issues.apache.org/jira/browse/KAFKA-2210 > > > Repository: kafka > > > Description > ------- > > Addressing review comments from Jun. > > > Adding CREATE check for offset topic only if the topic does not exist already. > > > Addressing some more comments. > > > Removing acl.json file > > > Moving PermissionType to trait instead of enum. Following the convention for > defining constants. > > > Adding authorizer.config.path back. > > > Addressing more comments from Jun. > > > Addressing more comments. > > > Diffs > ----- > > core/src/main/scala/kafka/api/OffsetRequest.scala > f418868046f7c99aefdccd9956541a0cb72b1500 > core/src/main/scala/kafka/common/AuthorizationException.scala PRE-CREATION > core/src/main/scala/kafka/common/ErrorMapping.scala > c75c68589681b2c9d6eba2b440ce5e58cddf6370 > core/src/main/scala/kafka/security/auth/Acl.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Authorizer.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Operation.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/PermissionType.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Resource.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/ResourceType.scala PRE-CREATION > core/src/main/scala/kafka/server/KafkaApis.scala > 18f5b5b895af1469876b2223841fd90a2dd255e0 > core/src/main/scala/kafka/server/KafkaConfig.scala > dbe170f87331f43e2dc30165080d2cb7dfe5fdbf > core/src/main/scala/kafka/server/KafkaServer.scala > 18917bc4464b9403b16d85d20c3fd4c24893d1d3 > core/src/test/scala/unit/kafka/security/auth/AclTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/KafkaPrincipalTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/OperationTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/PermissionTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/ResourceTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/ResourceTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > 04a02e08a54139ee1a298c5354731bae009efef3 > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
