> On Julho 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] > > } > > } > > ``` > > Parth Brahmbhatt wrote: > I haven't changed it for now dont really think to .toSet will be that bad.
Agree! All the other options leave the code more obfuscated than it needs to be, imo. Only if there was some strict code guideline to use "pure" Functional Programming we would need to resort to one of those options I described. - Edward ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92345 ----------------------------------------------------------- On Ago. 11, 2015, 1:32 a.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated Ago. 11, 2015, 1:32 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. > > > Now addressing Ismael's comments. Case sensitive checks. > > > 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/network/RequestChannel.scala > 20741281dcaa76374ea6f86a2185dad27b515339 > 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 > 7ea509c2c41acc00430c74e025e069a833aac4e7 > core/src/main/scala/kafka/server/KafkaConfig.scala > dbe170f87331f43e2dc30165080d2cb7dfe5fdbf > core/src/main/scala/kafka/server/KafkaServer.scala > 84d4730ac634f9a5bf12a656e422fea03ad72da8 > 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/ResourceTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
