Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review78897 --- Ship it! Thanks for the patch. +1. Committed to trunk after fixing

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated April 5, 2015, 5 a.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review78867 --- Could you also make a pass of unused imports? I saw unused imports a

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review78839 --- Thanks for the patch. Looks good. Just a few more minor comments bel

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review78842 --- Other than the comment from Jun and the question about the security

Re: Review Request 28769: Patch for KAFKA-1809

> On April 3, 2015, 12:41 a.m., Joel Koshy wrote: > > clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java, > > line 26 > > > > > > Can we go with TRACE(Short.MAX_VALUE, "TRACE") and start plai

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated April 3, 2015, 2:04 a.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

> On April 3, 2015, 12:41 a.m., Joel Koshy wrote: > > clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java, > > line 44 > > > > > > All caps Apparently "name" and "id" cannot be all caps. Our

Re: Review Request 28769: Patch for KAFKA-1809

> On April 3, 2015, 12:41 a.m., Joel Koshy wrote: > > system_test/replication_testsuite/testcase_0001/testcase_0001_properties.json, > > line 32 > > > > > > were these changes intentional? Very intentional. System_te

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review78739 --- Thanks for the patch and for putting in a ton of effort in to it. O

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated April 2, 2015, 10:12 p.m.) Review request for kafka. Bugs: KAFKA-180

Re: Review Request 28769: Patch for KAFKA-1809

> On March 29, 2015, 7:33 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/protocol/ApiVersion.java, > > line 28 > > > > > > Since this is for intra-broker communication, should we move this clas

Re: Review Request 28769: Patch for KAFKA-1809

> On March 29, 2015, 7:33 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 345-347 > > > > > > It seems that a 3-digit value like 0.8.2 is also valid? > > Gwen Shapira wrote: > y

Re: Review Request 28769: Patch for KAFKA-1809

> On March 29, 2015, 7:33 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/protocol/ApiVersion.java, > > line 28 > > > > > > Since this is for intra-broker communication, should we move this clas

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review78160 --- Thanks for the latest patch. A few more comments below. clients/sr

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated March 27, 2015, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-18

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated March 16, 2015, 4:41 p.m.) Review request for kafka. Bugs: KAFKA-180

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated March 16, 2015, 4:02 p.m.) Review request for kafka. Bugs: KAFKA-180

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Feb. 27, 2015, 4:31 a.m.) Review request for kafka. Changes ---

Re: Review Request 28769: Patch for KAFKA-1809

> On Feb. 12, 2015, 7:46 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/protocol/ApiVersion.java, > > lines 20-27 > > > > > > It seems that we rely on the lexicographical ordering of the enum n

Re: Review Request 28769: Patch for KAFKA-1809

> On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 70-72 > > > > > > Same comment as above, does a null string translate into a null in > > endPoint.host? > >

Re: Review Request 28769: Patch for KAFKA-1809

> On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 70-72 > > > > > > Same comment as above, does a null string translate into a null in > > endPoint.host? yes.

Re: Review Request 28769: Patch for KAFKA-1809

> On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/ConsumerConfig.scala, lines 184-186 > > > > > > I thought we won't support security in the old client? We don't, but I need to test

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review72226 --- clients/src/main/java/org/apache/kafka/common/protocol/ApiVersion.j

Re: Review Request 28769: Patch for KAFKA-1809

> On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote: > > Thanks for the new patch. Some more comments. > > > > 1. We should think through whether we need to add security protocol to > > existing tools like SimleConsumerShell and UpdateOffsetsInZk. > > 2. There are unused imports. > > 3. The patch need

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review71916 --- Thanks for the new patch. Some more comments. 1. We should think th

Re: Review Request 28769: Patch for KAFKA-1809

> On Feb. 2, 2015, 10:46 p.m., Joel Koshy wrote: > > It seems some of the recent trunk changes have crept into this diff. Seems > > to be an issue with the patch review script as you normally shouldn't need > > to git pull before submitting. Turned out that in addition to rebasing my branch, I

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Feb. 3, 2015, 6:52 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Feb. 3, 2015, 6:46 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Feb. 3, 2015, 6:45 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review70642 --- It seems some of the recent trunk changes have crept into this diff.

Re: Review Request 28769: Patch for KAFKA-1809

Good idea :) Fixed this, rebased the patch and uploaded a new diff. Gwen On Thu, Jan 29, 2015 at 9:27 AM, Don Bosco Durai wrote: > +1 > > I also feel, having security.* would be easy going forward. > > Thanks > > Bosco > > > On 1/29/15, 6:08 AM, "Jeff Holoman" wrote: > >> >> >>> On Jan. 23, 20

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Feb. 2, 2015, 7:55 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

+1 I also feel, having security.* would be easy going forward. Thanks Bosco On 1/29/15, 6:08 AM, "Jeff Holoman" wrote: > > >> On Jan. 23, 2015, 1:57 a.m., Jun Rao wrote: >> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 182-183 >> > >>

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 23, 2015, 1:57 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 182-183 > > > > > > Since this is also used for communication btw the controller and the > > brokers, perhap

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Jan. 28, 2015, 6:26 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review69804 --- Some of my comments are outside of the scope of this review, so it's

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 23, 2015, 1:57 a.m., Jun Rao wrote: > > Thanks for the patch. Looks promising. Some comments. > > > > 1. I overlooked this when I suggested the new broker format in ZK. This > > means that we will need to upgrade all consumer clients before we can turn > > on the flag of using the ne

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review69281 --- Thanks for the patch. Looks promising. Some comments. 1. I overlook

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Jan. 14, 2015, 2:16 a.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 7, 2015, 3:01 a.m., Jun Rao wrote: > > Thanks for the patch. It's a lot work! A few general comments below, in > > addition to the more detailed comments. > > > > 1. Formatting: add a space after comma in function signature and function > > calls. So instead of > > def foo(a: A,

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 7, 2015, 3:01 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/cluster/Broker.scala, lines 36-52 > > > > > > It would be good to document the format for both v1 and v2. > > > > Also, would it be cl

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 7, 2015, 3:01 a.m., Jun Rao wrote: > > Thanks for the patch. It's a lot work! A few general comments below, in > > addition to the more detailed comments. > > > > 1. Formatting: add a space after comma in function signature and function > > calls. So instead of > > def foo(a: A,

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 7, 2015, 5:02 p.m., Jay Kreps wrote: > > core/src/main/scala/kafka/cluster/Broker.scala, line 36 > > > > > > Hey guys, I have a big concern with adding JSON to the protocol mixed > > in with the binary format

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 7, 2015, 5:02 p.m., Jay Kreps wrote: > > core/src/main/scala/kafka/cluster/Broker.scala, line 36 > > > > > > Hey guys, I have a big concern with adding JSON to the protocol mixed > > in with the binary format

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 7, 2015, 5:02 p.m., Jay Kreps wrote: > > core/src/main/scala/kafka/cluster/Broker.scala, line 36 > > > > > > Hey guys, I have a big concern with adding JSON to the protocol mixed > > in with the binary format

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review67040 --- core/src/main/scala/kafka/cluster/Broker.scala

Re: Review Request 28769: Patch for KAFKA-1809

> On Jan. 7, 2015, 3:01 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/cluster/EndPoint.scala, line 36 > > > > > > Do we need the dot after -? Yes. Because of the brackets its an actual dot (doesn't match "any") a

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review66944 --- Thanks for the patch. It's a lot work! A few general comments below,

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Jan. 6, 2015, 7:46 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Jan. 6, 2015, 5:40 a.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Jan. 6, 2015, 4:25 a.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Jan. 5, 2015, 10:24 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Dec. 31, 2014, 2:48 a.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Dec. 30, 2014, 7:25 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Dec. 29, 2014, 8:11 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Dec. 27, 2014, 8:23 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Dec. 27, 2014, 8:03 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

> On Dec. 6, 2014, 7:40 a.m., Joe Stein wrote: > > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala, line 95 > > > > > > PLAINTEXT here should be read and matched to it's int from the > > SecurityProtocol st

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/ --- (Updated Dec. 25, 2014, 7:04 p.m.) Review request for kafka. Bugs: KAFKA-1809

Re: Review Request 28769: Patch for KAFKA-1809

> On Dec. 6, 2014, 7:40 a.m., Joe Stein wrote: > > core/src/main/scala/kafka/cluster/Broker.scala, line 40 > > > > > > More JSON, k! We need to change parsers I think or change using > > JSON this is in a few other

Re: Review Request 28769: Patch for KAFKA-1809

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review64163 --- clients/src/main/java/org/apache/kafka/clients/producer/ProducerCon