Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- Review request for kafka. Bugs: KAFKA-2210 https://issues.apache.org/jira/b

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review85791 --- Thanks for that patch. A few comments below. Also, two common types

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated June 3, 2015, 11:34 p.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated June 3, 2015, 11:36 p.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review86267 --- core/src/main/scala/kafka/security/auth/Acl.scala

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated June 4, 2015, 11:07 p.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

> On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, lines 57-62 > > > > > > I tried exactly that but it tunrs out our current Json parser does not > > work when

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review86541 --- Hi Parth, thank you for your patch of authorization, and could you u

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review87532 --- core/src/main/scala/kafka/server/KafkaConfig.scala

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated July 10, 2015, 1 a.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

> On June 9, 2015, 7:13 a.m., Dapeng Sun wrote: > > core/src/main/scala/kafka/common/ErrorMapping.scala, line 57 > > > > > > Why not use 23 here? I have changed it so it now uses 23. > On June 9, 2015, 7:13 a.m., Dap

Re: Review Request 34492: Patch for KAFKA-2210

> On June 11, 2015, 7:19 a.m., Dapeng Sun wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, line 160 > > > > > > Why add a new config file path? could authorization related config > > options be merged in

Re: Review Request 34492: Patch for KAFKA-2210

> On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, lines 57-62 > > > > > > I tried exactly that but it tunrs out our current Json parser does not > > work when

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated July 14, 2015, 5:02 p.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91638 --- core/src/main/scala/kafka/security/auth/Acl.scala (line 24)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated July 14, 2015, 9:13 p.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91671 --- core/src/main/scala/kafka/security/auth/Operation.scala (line 35) <

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91672 --- core/src/main/scala/kafka/security/auth/Acl.scala (line 24)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91675 --- core/src/main/scala/kafka/security/auth/PermissionType.scala (line

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91676 --- core/src/main/scala/kafka/security/auth/Resource.scala (line 25)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91679 --- core/src/main/scala/kafka/security/auth/Acl.scala (line 110)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91680 --- core/src/test/scala/unit/kafka/security/auth/AclTest.scala (line 24

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91682 --- core/src/main/scala/kafka/security/auth/ResourceType.scala (line 45

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91836 --- Thanks for the patch. A few comments below. Also, 1. For making use

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review91980 --- core/src/main/scala/kafka/security/auth/Operation.scala (lines 43 -

Re: Review Request 34492: Patch for KAFKA-2210

> On July 16, 2015, 2:16 a.m., Jun Rao wrote: > > Thanks for the patch. A few comments below. > > > > Also, > > 1. For making user/group case-insensitive, could you start a discussion > > thread on the dev mailing list to see if anyone has concerns on this? > > 2. Got the following compilation

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated July 20, 2015, 11:42 p.m.) Review request for kafka. Bugs: KAFKA-221

Re: Review Request 34492: Patch for KAFKA-2210

> On July 16, 2015, 10:32 p.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Operation.scala, lines 43-44 > > > > > > +1. As of today, it spills out a Match error that can be confusing to > > fig

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92341 --- core/src/main/scala/kafka/security/auth/Acl.scala (line 86)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92345 --- core/src/main/scala/kafka/security/auth/Acl.scala (line 71)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92347 --- core/src/main/scala/kafka/security/auth/PermissionType.scala (line

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92349 --- core/src/main/scala/kafka/security/auth/Acl.scala (line 136)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92350 --- core/src/main/scala/kafka/server/KafkaApis.scala (line 620)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92351 --- core/src/main/scala/kafka/security/auth/Acl.scala (line 86)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92352 --- core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92353 --- core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92354 --- core/src/main/scala/kafka/security/auth/Operation.scala (line 43) <

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92355 --- core/src/main/scala/kafka/server/KafkaApis.scala (line 148)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92356 --- core/src/main/scala/kafka/server/KafkaApis.scala (line 536)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92357 --- core/src/main/scala/kafka/server/KafkaApis.scala (line 136)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92358 --- core/src/main/scala/kafka/server/KafkaApis.scala (line 163)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92359 --- core/src/main/scala/kafka/security/auth/PermissionType.scala (line

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review92361 --- core/src/main/scala/kafka/security/auth/PermissionType.scala (line

Re: Review Request 34492: Patch for KAFKA-2210

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

Re: Review Request 34492: Patch for KAFKA-2210

> On June 1, 2015, 1:11 a.m., Jun Rao wrote: > > Thanks for that patch. A few comments below. > > > > Also, two common types of users are consumers and publishers. Currently, if > > you want to allow a user to consume from topic t in consumer group g, you > > have to grant (1) read permission

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 2:13 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/PermissionType.scala, line 21 > > > > > > the semi-colon is trying to scape here, catch it! :) Indeed it was, removed.

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 2:15 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/PermissionType.scala, line 21 > > > > > > Just kidding, please remove it. removed. - Parth

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 2:09 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 166 > > > > > > Please, put a space between ``if`` and ``(``. Fixed. - Parth --

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:57 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 151 > > > > > > Please, put a space between ``if`` and ``(`` here. Fixed. - Parth -

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 2:02 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 139 > > > > > > Put a space between ``if`` and ``(``. Fixed. - Parth --

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:43 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 624 > > > > > > Lines L#620 and L#621 could be merged (with a &&) into a single > > if-condition. No n

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:55 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Operation.scala, line 43 > > > > > > The ``return`` here is redundant. Fixed. - Parth -

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:50 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 28 > > > > > > Please, put a space between ``if`` and ``(``. fixed. - Parth

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:59 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 540 > > > > > > Please, put a space between ``if`` and ``(``. Fixed. - Parth --

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:49 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 55 > > > > > > the parenthesis after the ``!`` is not required. fixed. - Parth -

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:47 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 86 > > > > > > Or maybe an Map.empty[String, Any] fixed. - Parth --

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:40 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 136 > > > > > > The return is redundant here. fixed. - Parth -

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:37 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/PermissionType.scala, line 47 > > > > > > The ``return`` here is redundant. In fact the L#46 - L#48 could be > > rewrit

Re: Review Request 34492: Patch for KAFKA-2210

> On July 14, 2015, 11:26 p.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 110 > > > > > > As the values are fixed you could have written toMap() as below so that > > we can sa

Re: Review Request 34492: Patch for KAFKA-2210

> On July 14, 2015, 11:01 p.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 24 > > > > > > nit: what about switch this lines 23 and 24 and then use WildCardHost > > as replacemen

Re: Review Request 34492: Patch for KAFKA-2210

> On July 14, 2015, 10:59 p.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Operation.scala, line 35 > > > > > > Scala's match is a powerful mechanism but using it to decode as below > > seems b

Re: Review Request 34492: Patch for KAFKA-2210

> On July 14, 2015, 11:09 p.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/PermissionType.scala, line 40 > > > > > > Same comment of Operation.scala also applies here. In addition, the > > retu

Re: Review Request 34492: Patch for KAFKA-2210

> On July 14, 2015, 11:12 p.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Resource.scala, line 25 > > > > > > In KafkaPrincipal you split like: > > > > val arr: Array[String] = str.spl

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 1:30 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 71 > > > > > > Disclaimer: I am not claiming that you should change the code commented > > here. > >

Re: Review Request 34492: Patch for KAFKA-2210

> On July 21, 2015, 12:49 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 86 > > > > > > It's better to return None here, no? Can't return None or nil where a Map[String, Any]

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review93302 --- I did an initial pass and left some comments. Some of them are quest

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated Aug. 11, 2015, 1:32 a.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

> On July 28, 2015, 5:18 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/common/AuthorizationException.scala, line 24 > > > > > > Exceptions without a message are discouraged, so I would remove the > > no-args

Re: Review Request 34492: Patch for KAFKA-2210

> On Julho 21, 2015, 1:30 a.m., Edward Ribeiro wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 71 > > > > > > Disclaimer: I am not claiming that you should change the code commented > > here. > >

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review95801 --- Thanks for the patch. A few comments below. There is a unit test fai

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review95934 --- Thanks for addressing the issues raised in the previous review. We a

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review95942 --- One more thing: it may be a good idea to rebase against trunk since

Re: Review Request 34492: Patch for KAFKA-2210

> On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/security/auth/Operation.scala, line 42 > > > > > > Generally a good idea to set the result type for public methods. This > > makes it

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated Aug. 20, 2015, 6:27 p.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

> On Aug. 20, 2015, 10:29 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/network/RequestChannel.scala, line 48 > > > > > > Normally one would use `Option[Session]` here. Are we using `null` due > > to effici

Re: Review Request 34492: Patch for KAFKA-2210

> On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, lines 84-87 > > > > > > Do we need the case match here since acls is always a Set? Fixed. > On Aug. 20, 2015, 1:

Re: Review Request 34492: Patch for KAFKA-2210

> On Aug. 20, 2015, 10:30 a.m., Ismael Juma wrote: > > One more thing: it may be a good idea to rebase against trunk since the > > large SSL/TLS patch has now been merged (not sure if there are any > > conflicts). Done. - Parth --- Th

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review96129 --- core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated Aug. 26, 2015, 12:59 a.m.) Review request for kafka. Bugs: KAFKA-221

Re: Review Request 34492: Patch for KAFKA-2210

> On Aug. 23, 2015, 9:29 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala, line 21 > > > > > > There is a java version of KafkaPrincipal. Could we consolidate? consolidated. Conv

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated Aug. 26, 2015, 9:29 p.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review96909 --- Thanks for the patch. We are getting pretty close. A few more commen

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated Sept. 1, 2015, 10:36 p.m.) Review request for kafka. Bugs: KAFKA-221

Re: Review Request 34492: Patch for KAFKA-2210

> On Aug. 31, 2015, 6:47 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, line 99 > > > > > > I had a comment on this earlier that the current ACL representation > > makes it a bit hard to

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review97431 --- core/src/main/scala/kafka/server/KafkaApis.scala (line 296)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review97432 --- Hi Parth, I finally had a bit of time to look into this in more deta

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review97466 --- Ship it! Parth, thanks a lot for the latest patch. Overall, it look

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated Sept. 2, 2015, 9:50 p.m.) Review request for kafka. Bugs: KAFKA-2210

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review97481 --- core/src/main/scala/kafka/server/KafkaApis.scala (line 296)

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review97554 --- Hmm, did you attach the right patch? It doesn't seem to apply to tru

Re: Review Request 34492: Patch for KAFKA-2210

> On Sept. 2, 2015, 11:13 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 676 > > > > > > Could we change the test to the following to make it more consistent > > with the rest of pl

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated Sept. 3, 2015, 12:32 a.m.) Review request for kafka. Bugs: KAFKA-221

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/ --- (Updated Sept. 3, 2015, 12:36 a.m.) Review request for kafka. Bugs: KAFKA-221

Re: Review Request 34492: Patch for KAFKA-2210

> On Sept. 2, 2015, 11:13 p.m., Jun Rao wrote: > > Hmm, did you attach the right patch? It doesn't seem to apply to trunk and > > my last round of minor comments didn't seem to get addressed. Also, one > > more suggestion below. Can you take a look now? I am not sure why the patch looked weird

Re: Review Request 34492: Patch for KAFKA-2210

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review97582 --- Ship it! Thanks Parth, LGTM. - Ismael Juma On Sept. 3, 2015, 12:

Re: Review Request 34492: Patch for KAFKA-2210

> On Sept. 2, 2015, 1:11 p.m., Ismael Juma wrote: > > Hi Parth, I finally had a bit of time to look into this in more detail and > > I pushed a commit with my suggestions to make the `KafkaApis` code a bit > > more readable: > > > > https://github.com/ijuma/kafka/commit/7737a9feb0c6d8cb4be3fe2

Re: Review Request 34492: Patch for KAFKA-2210

> On Sept. 2, 2015, 4:36 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/common/ErrorMapping.scala, lines 55-57 > > > > > > Could you add the missing error cords 23-28 in the comment? copied the error constant fr

Re: Review Request 34492: Patch for KAFKA-2210

> On Sept. 2, 2015, 1:11 p.m., Ismael Juma wrote: > > Hi Parth, I finally had a bit of time to look into this in more detail and > > I pushed a commit with my suggestions to make the `KafkaApis` code a bit > > more readable: > > > > https://github.com/ijuma/kafka/commit/7737a9feb0c6d8cb4be3fe2