---
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
---
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
On April 3, 2015, 12:41 a.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java,
line 26
https://reviews.apache.org/r/28769/diff/22/?file=914062#file914062line26
Can we go with TRACE(Short.MAX_VALUE, TRACE) and start plaintext at
0?
---
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
---
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
On March 29, 2015, 7:33 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/protocol/ApiVersion.java,
line 28
https://reviews.apache.org/r/28769/diff/21/?file=908349#file908349line28
Since this is for intra-broker communication, should we move this class
to
---
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:
---
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:
---
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.
On April 3, 2015, 12:41 a.m., Joel Koshy wrote:
system_test/replication_testsuite/testcase_0001/testcase_0001_properties.json,
line 32
https://reviews.apache.org/r/28769/diff/22/?file=914129#file914129line32
were these changes intentional?
Very intentional. System_tests include
On April 3, 2015, 12:41 a.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java,
line 44
https://reviews.apache.org/r/28769/diff/22/?file=914062#file914062line44
All caps
Apparently name and id cannot be all caps.
Our coding style
On March 29, 2015, 7:33 p.m., Jun Rao wrote:
core/src/main/scala/kafka/server/KafkaConfig.scala, lines 345-347
https://reviews.apache.org/r/28769/diff/21/?file=908381#file908381line345
It seems that a 3-digit value like 0.8.2 is also valid?
Gwen Shapira wrote:
yes, and also
On March 29, 2015, 7:33 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/protocol/ApiVersion.java,
line 28
https://reviews.apache.org/r/28769/diff/21/?file=908349#file908349line28
Since this is for intra-broker communication, should we move this class
to
---
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.
---
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:
---
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:
---
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:
On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote:
core/src/main/scala/kafka/server/KafkaConfig.scala, lines 70-72
https://reviews.apache.org/r/28769/diff/17/?file=846168#file846168line70
Same comment as above, does a null string translate into a null in
endPoint.host?
yes. null string
On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote:
core/src/main/scala/kafka/consumer/ConsumerConfig.scala, lines 184-186
https://reviews.apache.org/r/28769/diff/17/?file=846154#file846154line184
I thought we won't support security in the old client?
We don't, but I need to test my code
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
https://reviews.apache.org/r/28769/diff/17/?file=846135#file846135line20
It seems that we rely on the lexicographical ordering of the enum name
for
---
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
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28769/#review72226
---
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 needs rebase.
---
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
---
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
---
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
---
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
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 bo...@apache.org wrote:
+1
I also feel, having security.* would be easy going forward.
Thanks
Bosco
On 1/29/15, 6:08 AM, Jeff Holoman jeff.holo...@gmail.com wrote:
---
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
---
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
+1
I also feel, having security.* would be easy going forward.
Thanks
Bosco
On 1/29/15, 6:08 AM, Jeff Holoman jeff.holo...@gmail.com wrote:
On Jan. 23, 2015, 1:57 a.m., Jun Rao wrote:
core/src/main/scala/kafka/server/KafkaConfig.scala, lines 182-183
On Jan. 23, 2015, 1:57 a.m., Jun Rao wrote:
core/src/main/scala/kafka/server/KafkaConfig.scala, lines 182-183
https://reviews.apache.org/r/28769/diff/12/?file=820431#file820431line182
Since this is also used for communication btw the controller and the
brokers, perhaps it's
---
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:
---
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
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 new protocol
---
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
---
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:
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,b: B)
On Jan. 7, 2015, 3:01 a.m., Jun Rao wrote:
core/src/main/scala/kafka/cluster/Broker.scala, lines 36-52
https://reviews.apache.org/r/28769/diff/11/?file=807930#file807930line36
It would be good to document the format for both v1 and v2.
Also, would it be cleaner if in
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,b: B)
On Jan. 7, 2015, 5:02 p.m., Jay Kreps wrote:
core/src/main/scala/kafka/cluster/Broker.scala, line 36
https://reviews.apache.org/r/28769/diff/1/?file=783935#file783935line36
Hey guys, I have a big concern with adding JSON to the protocol mixed
in with the binary format. Can we
On Jan. 7, 2015, 3:01 a.m., Jun Rao wrote:
core/src/main/scala/kafka/cluster/EndPoint.scala, line 36
https://reviews.apache.org/r/28769/diff/11/?file=807932#file807932line36
Do we need the dot after -?
Yes. Because of the brackets its an actual dot (doesn't match any) and its
---
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
On Jan. 7, 2015, 5:02 p.m., Jay Kreps wrote:
core/src/main/scala/kafka/cluster/Broker.scala, line 36
https://reviews.apache.org/r/28769/diff/1/?file=783935#file783935line36
Hey guys, I have a big concern with adding JSON to the protocol mixed
in with the binary format. Can we
On Jan. 7, 2015, 5:02 p.m., Jay Kreps wrote:
core/src/main/scala/kafka/cluster/Broker.scala, line 36
https://reviews.apache.org/r/28769/diff/1/?file=783935#file783935line36
Hey guys, I have a big concern with adding JSON to the protocol mixed
in with the binary format. Can we
---
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
---
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
---
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:
---
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
---
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
---
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:
---
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:
---
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:
---
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:
---
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:
On Dec. 6, 2014, 7:40 a.m., Joe Stein wrote:
core/src/main/scala/kafka/cluster/Broker.scala, line 40
https://reviews.apache.org/r/28769/diff/1/?file=783935#file783935line40
More JSON, k! We need to change parsers I think or change using
JSON this is in a few other tickets
---
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:
On Dec. 6, 2014, 7:40 a.m., Joe Stein wrote:
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala, line 95
https://reviews.apache.org/r/28769/diff/1/?file=783984#file783984line95
PLAINTEXT here should be read and matched to it's int from the
SecurityProtocol statics also.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28769/
---
Review request for kafka.
Bugs: KAFKA-1809
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28769/#review64163
---
60 matches
Mail list logo
