---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated Aug. 18, 2015, 6:24 p.m.)
Review request for kafka.
Bugs:
On Aug. 18, 2015, 4:09 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 438
https://reviews.apache.org/r/33620/diff/17/?file=1042291#file1042291line438
Hmm, do we want to break here? It seems that we want to continue in the
On Aug. 18, 2015, 4:09 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 450
https://reviews.apache.org/r/33620/diff/17/?file=1042291#file1042291line450
The termination condition doesn't seem quite right. If dst has no
On Aug. 19, 2015, 1:56 a.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/utils/Utils.java, line 520
https://reviews.apache.org/r/33620/diff/18/?file=1043256#file1043256line520
second flip after `BUFFER_OVERFLOW` in `handshakeWrap` - you had
mentioned offline
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review95765
---
I haven't finished going through the latest patch, but discussed
On Aug. 19, 2015, 1:56 a.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/utils/Utils.java, line 520
https://reviews.apache.org/r/33620/diff/18/?file=1043256#file1043256line520
second flip after `BUFFER_OVERFLOW` in `handshakeWrap` - you had
mentioned offline
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review95798
---
Ship it!
Looks good to me. Could you rebase?
- Jun Rao
On Aug.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated Aug. 19, 2015, 12:24 a.m.)
Review request for kafka.
Bugs:
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 368-381
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line368
If handshake status is BUFFER_OVERFLOW, we will return to the caller
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review95671
---
Ship it!
Thanks for the latest patch. +1. I only have a few minor
On July 27, 2015, 1:32 p.m., Ismael Juma wrote:
clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.java, line
29
https://reviews.apache.org/r/33620/diff/13/?file=1021968#file1021968line29
SSL is deprecated
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review95582
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated Aug. 17, 2015, 3:13 p.m.)
Review request for kafka.
Bugs:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review95619
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated Aug. 17, 2015, 7:21 p.m.)
Review request for kafka.
Bugs:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated Aug. 17, 2015, 4:28 p.m.)
Review request for kafka.
Bugs:
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 417
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line417
It's still not very clear to me how renegotiation can be supported in
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 306-320
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line306
It seems that the logic here can be simpler. In handshake(), we call
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 368-381
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line368
If handshake status is BUFFER_OVERFLOW, we will return to the caller
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated Aug. 17, 2015, 3:41 a.m.)
Review request for kafka.
Bugs:
On July 27, 2015, 1:32 p.m., Ismael Juma wrote:
core/src/main/scala/kafka/server/KafkaConfig.scala, line 867
https://reviews.apache.org/r/33620/diff/13/?file=1022000#file1022000line867
Why are we using a Java Map here? Is it used in Java code? If not, then
why not use Scala's
On July 31, 2015, 4:28 p.m., Ismael Juma wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 319
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line319
This message seems a bit unclear to me. Are we saying that
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 306-320
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line306
It seems that the logic here can be simpler. In handshake(), we call
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 417
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line417
It's still not very clear to me how renegotiation can be supported in
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/Selector.java, line
535
https://reviews.apache.org/r/33620/diff/13/?file=1021981#file1021981line535
Not sure why we need to check hasSend. It's possible for a channel to
have both
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
core/src/test/scala/unit/kafka/network/SocketServerTest.scala, line 207
https://reviews.apache.org/r/33620/diff/13/?file=1022008#file1022008line207
Is this test needed given the tests we have on EchoServer?
Also, do we have a
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 430-433
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line430
Is this needed? If we need to expand appReadBuffer, netReadBuffer's
On July 29, 2015, 1:15 a.m., Dong Lin wrote:
clients/src/test/java/org/apache/kafka/common/network/SSLFactoryTest.java,
line 52
https://reviews.apache.org/r/33620/diff/13/?file=1021992#file1021992line52
I am not sure about this. It is probably a stupid question. Do you
indeed
On July 27, 2015, 1:32 p.m., Ismael Juma wrote:
clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.java, line
29
https://reviews.apache.org/r/33620/diff/13/?file=1021968#file1021968line29
SSL is deprecated
On Aug. 3, 2015, 4:50 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 262-269
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line262
Could we transition from NEED_WRAP to NOT_HANDSHAKING directly? Or
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93939
---
clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93862
---
Thanks for the patch. A few more comments below.
build.gradle
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93734
---
On July 31, 2015, 4:32 p.m., Sriharsha Chintalapani wrote:
core/src/test/scala/integration/kafka/api/SSLConsumerTest.scala, line 218
https://reviews.apache.org/r/33620/diff/13/?file=1022004#file1022004line218
If we want to enforce this coding convention.Lets open up a new JIRA
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93735
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93736
---
core/src/test/scala/integration/kafka/api/SSLConsumerTest.scala
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93729
---
On July 31, 2015, 4:29 p.m., Sriharsha Chintalapani wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 162
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line162
hasRemaining doesn't work here. Hence the reason to go
On July 27, 2015, 1:32 p.m., Ismael Juma wrote:
clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.java, line
29
https://reviews.apache.org/r/33620/diff/13/?file=1021968#file1021968line29
SSL is deprecated
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93639
---
On July 30, 2015, 10:37 p.m., Dong Lin wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 416
https://reviews.apache.org/r/33620/diff/13/?file=1021979#file1021979line416
Do you mean unwrapResult.getHandshakeStatus() ==
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93684
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93332
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93110
---
I did an initial pass over the code (excluding tests) and left some
On July 27, 2015, 1:32 p.m., Ismael Juma wrote:
core/src/main/scala/kafka/api/FetchResponse.scala, line 82
https://reviews.apache.org/r/33620/diff/13/?file=1021998#file1021998line82
Casts are to be avoided in Scala, pattern matching is a better way to
do this:
On July 27, 2015, 1:32 p.m., Ismael Juma wrote:
core/src/main/scala/kafka/api/FetchResponse.scala, line 82
https://reviews.apache.org/r/33620/diff/13/?file=1021998#file1021998line82
Casts are to be avoided in Scala, pattern matching is a better way to
do this:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review93177
---
On July 22, 2015, 3 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 411
https://reviews.apache.org/r/33620/diff/12/?file=1017027#file1017027line411
Is renegotiation actually supported? It seems that renegotiation can
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated July 25, 2015, 7:11 p.m.)
Review request for kafka.
Bugs:
On July 22, 2015, 3 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 231-234
https://reviews.apache.org/r/33620/diff/12/?file=1017027#file1017027line231
Still some questions on this.
1. When handshakeStatus is
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review92599
---
On July 22, 2015, 3 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 231-234
https://reviews.apache.org/r/33620/diff/12/?file=1017027#file1017027line231
Still some questions on this.
1. When handshakeStatus is
On July 22, 2015, 3 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 361-374
https://reviews.apache.org/r/33620/diff/12/?file=1017027#file1017027line361
Hmm, if status is ok and handshakeStatus is NEED_UNWRAP, could we get
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review92405
---
On July 22, 2015, 3 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 411
https://reviews.apache.org/r/33620/diff/12/?file=1017027#file1017027line411
Is renegotiation actually supported? It seems that renegotiation can
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review92711
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review92314
---
Thanks for the patch. Looks good overall. A few comments below.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated July 20, 2015, 1:10 p.m.)
Review request for kafka.
Bugs:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated July 20, 2015, 7 p.m.)
Review request for kafka.
Bugs: KAFKA-1690
On June 30, 2015, 3:03 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 67
https://reviews.apache.org/r/33620/diff/10/?file=990783#file990783line67
Shouldn't we only start the ssl handshake after the raw socket
connection
On June 30, 2015, 3:03 p.m., Jun Rao wrote:
core/src/main/scala/kafka/network/SocketServer.scala, line 393
https://reviews.apache.org/r/33620/diff/10/?file=990801#file990801line393
This parameter doesn't seem to be used. We can remove it.
I am using for metrics
On June 30, 2015,
On June 30, 2015, 5:20 p.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/network/Channel.java, lines
148-150
https://reviews.apache.org/r/33620/diff/10/?file=990775#file990775line148
This should be caught by checkstyle - can you double-check? If we want
On June 24, 2015, 4:39 p.m., Jiangjie Qin wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 75-78
https://reviews.apache.org/r/33620/diff/10/?file=990783#file990783line75
Can they be replaced by clear() method?
These are set only once I
On May 22, 2015, 12:33 a.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 294
https://reviews.apache.org/r/33620/diff/8/?file=966806#file966806line294
Not sure why we are looping back here. If the HandshakeStatus is
On June 30, 2015, 5:27 p.m., Michael Herstine wrote:
clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java,
line 31
https://reviews.apache.org/r/33620/diff/10/?file=990787#file990787line31
This may be my lack of Kafak-specific knowledge, but I'm getting a bit
On June 30, 2015, 3:03 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 179
https://reviews.apache.org/r/33620/diff/10/?file=990783#file990783line179
Is this needed? It seems that the assumption is that if there is sth to
On May 15, 2015, 10:54 p.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 321
https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line321
Actually, can you describe how this would be done (say, for dealing
On June 30, 2015, 3:03 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 602
https://reviews.apache.org/r/33620/diff/10/?file=990783#file990783line602
Not sure if we should do the flip here. The caller may not be able to
On June 30, 2015, 5:27 p.m., Michael Herstine wrote:
clients/src/main/java/org/apache/kafka/common/network/Channel.java, lines
109-114
https://reviews.apache.org/r/33620/diff/10/?file=990775#file990775line109
So you want class `Channel` to have at most one send pending at any
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review89677
---
Thanks for the patch. A few comments below.
Also, we need to
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review89922
---
I have a few other comments which I can add later or discuss
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review89910
---
clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review89063
---
Thanks for the patch, Sriharsha. A few comments below.
On May 22, 2015, 12:33 a.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java,
line 231
https://reviews.apache.org/r/33620/diff/8/?file=966794#file966794line231
Do we need config.values? Could we just pass in config.originals() as
we do
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated June 23, 2015, 8:18 p.m.)
Review request for kafka.
Bugs:
On May 22, 2015, 12:33 a.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java,
line 231
https://reviews.apache.org/r/33620/diff/8/?file=966794#file966794line231
Do we need config.values? Could we just pass in config.originals() as
we do
On May 22, 2015, 12:33 a.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 294
https://reviews.apache.org/r/33620/diff/8/?file=966806#file966806line294
Not sure why we are looping back here. If the HandshakeStatus is
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated June 4, 2015, 1:52 a.m.)
Review request for kafka.
Bugs: KAFKA-1690
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review86250
---
On May 22, 2015, 12:33 a.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 240-242
https://reviews.apache.org/r/33620/diff/8/?file=966806#file966806line240
Would it be better to just throw the exception in the NOT_HANDSHAKING
- Michael
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review83993
---
On May 21, 2015, 5:37 p.m., Sriharsha Chintalapani
On May 15, 2015, 10:54 p.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 153
https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line153
I think Michael meant the following which I think is valid right?
On May 22, 2015, 12:14 a.m., Michael Herstine wrote:
clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java,
line 44
https://reviews.apache.org/r/33620/diff/8/?file=966813#file966813line44
I'm trying to imagine implementing `buildPrincipal` in such a
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated May 21, 2015, 5:37 p.m.)
Review request for kafka.
Bugs: KAFKA-1690
On May 15, 2015, 8:26 p.m., Michael Herstine wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java,
lines 137-139
https://reviews.apache.org/r/33620/diff/5/?file=957064#file957064line137
This is interesting; I don't see a corresponding
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated May 20, 2015, 9:54 p.m.)
Review request for kafka.
Bugs: KAFKA-1690
On May 15, 2015, 8:26 p.m., Michael Herstine wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 418
https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line418
So you're transferring bytes from `appReadBuffer` to `dst`, right?
On May 15, 2015, 8:26 p.m., Michael Herstine wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
lines 112-121
https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line112
Sorry, but I'm a little confused here. Why would the caller be
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review84144
---
On May 14, 2015, 10:21 a.m., Rajini Sivaram wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 190
https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line190
I think when delegated tasks are run asynchronously, selection
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review84012
---
On May 15, 2015, 8:26 p.m., Michael Herstine wrote:
clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java,
line 34
https://reviews.apache.org/r/33620/diff/5/?file=957068#file957068line34
This is probably just my ignorance of the Kafka codebase, but what does
On May 15, 2015, 8:26 p.m., Michael Herstine wrote:
clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java,
line 29
https://reviews.apache.org/r/33620/diff/5/?file=957072#file957072line29
Will it be pluggable; i.e. can individual sites provide their own
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review83993
---
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review84006
---
On May 15, 2015, 10:54 p.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java,
line 153
https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line153
I think Michael meant the following which I think is valid right?
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review83998
---
Few more minor comments.
General comment on javadocs: although
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/
---
(Updated May 15, 2015, 2:18 p.m.)
Review request for kafka.
Bugs: KAFKA-1690
On May 13, 2015, 3:40 p.m., Jun Rao wrote:
clients/src/main/java/org/apache/kafka/common/network/Channel.java, lines
70-71
https://reviews.apache.org/r/33620/diff/5/?file=957061#file957061line70
This is a general question. For ssl and sasl, does authentication only
happen at
On May 16, 2015, 12:07 a.m., Joel Koshy wrote:
clients/src/main/java/org/apache/kafka/common/network/SSLChannelBuilder.java,
line 35
https://reviews.apache.org/r/33620/diff/6/?file=961241#file961241line35
Can we just use a fixed thread pool?
A more general
1 - 100 of 124 matches
Mail list logo
