kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Edward Ribeiro" <edward.ribe...@gmail.com>
Subject Re: Review Request 33620: Patch for KAFKA-1690
Date Mon, 03 Aug 2015 18:55:08 GMT

-----------------------------------------------------------
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 (line 77)
<https://reviews.apache.org/r/33620/#comment148371>

    nit: move this line closer to the first line where it is used, that is, to line 79.



clients/src/main/java/org/apache/kafka/clients/ClientUtils.java (lines 80 - 86)
<https://reviews.apache.org/r/33620/#comment148379>

    nit: I would switch this if-else-if to a switch case as below:
    
    '''
       switch (securityProtocol) {
           case SecurityProtocol.SSL: channelBuilder = new SSLChannelBuilder(SSLFactory.Mode.CLIENT);
break;
           case SecurityProtocol.PLAINTEXT: channelBuilder = new PlainTextChannelBuilder();
break;
           default: throw new ConfigException("Invalid SecurityProtocol " + CommonClientConfigs.SECURITY_PROTOCOL_CONFIG);
            }
    '''



clients/src/main/java/org/apache/kafka/clients/NetworkClient.java (line 474)
<https://reviews.apache.org/r/33620/#comment148372>

    As you are here, you could remove this extra blank line here, no?



clients/src/main/java/org/apache/kafka/clients/NetworkClient.java (line 476)
<https://reviews.apache.org/r/33620/#comment148374>

    ''isSendable'' don't look like a good method name, imo. A better one would be ''canSendRequest''
or ''canSendMoreRequest''.



clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.java (line 19)
<https://reviews.apache.org/r/33620/#comment148376>

    Okay, this is a helper class that holds a lot of constants. In this case, it's a best
practice to add a private constructor as below:
    
    '''
    private SSLConfigs() {}
    '''
    
    This private constructor will make the class both non-instantiable and make it non inheritable.



clients/src/main/java/org/apache/kafka/common/network/ChannelBuilder.java (line 28)
<https://reviews.apache.org/r/33620/#comment148378>

    The ''public'' accessor here is unnecessary as interface methods are public by default.



clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java (line 38)
<https://reviews.apache.org/r/33620/#comment148377>

    It's a good practice to make both ''key'' and ''socketChannel'' as final fields.


- Edward Ribeiro


On July 25, 2015, 7:11 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> 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: KAFKA-1690
>     https://issues.apache.org/jira/browse/KAFKA-1690
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1690. new java producer needs ssl support as a client.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. SSLFactory tests.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Added PrincipalBuilder.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Addressing reviews.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Addressing reviews.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Addressing reviews.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Fixed minor issues with
the patch.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Fixed minor issues with
the patch.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client.
> 
> 
> Merge remote-tracking branch 'refs/remotes/origin/trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Broker side ssl changes.
> 
> 
> KAFKA-1684. SSL for socketServer.
> 
> 
> KAFKA-1690. Added SSLProducerSendTest and fixes to get right port for SSL.
> 
> 
> Merge branch 'trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Post merge fixes.
> 
> 
> KAFKA-1690. Added SSLProducerSendTest.
> 
> 
> KAFKA-1690. Minor fixes based on patch review comments.
> 
> 
> Merge commit
> 
> 
> KAFKA-1690. Added SSL Consumer Test.
> 
> 
> KAFKA-1690. SSL Support.
> 
> 
> KAFKA-1690. Addressing reviews.
> 
> 
> Merge branch 'trunk' into KAFKA-1690-V1
> 
> 
> Merge branch 'trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Addressing reviews. Removed interestOps from SSLTransportLayer.
> 
> 
> KAFKA-1690. Addressing reviews.
> 
> 
> KAFKA-1690. added staged receives to selector.
> 
> 
> KAFKA-1690. Addressing reviews.
> 
> 
> Merge branch 'trunk' into KAFKA-1690-V1
> 
> 
> Diffs
> -----
> 
>   build.gradle 0abec26fb2d7be62c8a673f9ec838e926e64b2d1 
>   checkstyle/import-control.xml 19e0659ef9385433d9f94dee43cd70a52b18c9e5 
>   clients/src/main/java/org/apache/kafka/clients/ClientUtils.java 0d68bf1e1e90fe9d5d4397ddf817b9a9af8d9f7a

>   clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java 2c421f42ed3fc5d61cf9c87a7eaa7bb23e26f63b

>   clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b

>   clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java 70377ae2fa46deb381139d28590ce6d4115e1adc

>   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba

>   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7

>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java aa264202f2724907924985a5ecbe74afc4c6c04b

>   clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java bae528d31516679bed88ee61b408f209f185a8cc

>   clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/network/Authenticator.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/network/ByteBufferSend.java df0e6d5105ca97b7e1cb4d334ffb7b443506bd0b

>   clients/src/main/java/org/apache/kafka/common/network/ChannelBuilder.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/network/DefaultAuthenticator.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/network/KafkaChannel.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/network/NetworkReceive.java 3ca0098b8ec8cfdf81158465b2d40afc47eb6f80

>   clients/src/main/java/org/apache/kafka/common/network/PlainTextChannelBuilder.java
PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java
PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/network/SSLChannelBuilder.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/network/Selectable.java 618a0fa53848ae6befea7eba39c2f3285b734494

>   clients/src/main/java/org/apache/kafka/common/network/Selector.java aaf60c98c2c0f4513a8d65ee0db67953a529d598

>   clients/src/main/java/org/apache/kafka/common/network/Send.java 8f6daadf6b67c3414911cda77765512131e56fd3

>   clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java dab1a94dd29563688b6ecf4eeb0e180b06049d3f

>   clients/src/main/java/org/apache/kafka/common/security/auth/DefaultPrincipalBuilder.java
PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/security/auth/KafkaPrincipal.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java PRE-CREATION

>   clients/src/main/java/org/apache/kafka/common/utils/Utils.java af9993cf9b3991f1e61e1201c94e19bc1bf76a68

>   clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java 13ce519f03d13db041e1f2dbcd6b59414d2775b8

>   clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java f3f8334f848be4cc043d5a573975609a3681fe7e

>   clients/src/test/java/org/apache/kafka/common/network/EchoServer.java PRE-CREATION

>   clients/src/test/java/org/apache/kafka/common/network/SSLFactoryTest.java PRE-CREATION

>   clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java PRE-CREATION

>   clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 158f9829ff64a969008f699e40c51e918287859e

>   clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java e7951d835472e5defe49be435f2c93685ba544d5

>   clients/src/test/java/org/apache/kafka/test/MockSelector.java 51eb9d142f566c94a87add68b8c4f78b56d6ec3e

>   clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java PRE-CREATION 
>   core/src/main/scala/kafka/api/FetchResponse.scala 0b6b33ab6f7a732ff1322b6f48abd4c43e0d7215

>   core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7

>   core/src/main/scala/kafka/server/KafkaConfig.scala dbe170f87331f43e2dc30165080d2cb7dfe5fdbf

>   core/src/main/scala/kafka/server/KafkaServer.scala 18917bc4464b9403b16d85d20c3fd4c24893d1d3

>   core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala d8eee52fc750e23c06c1f06f03b96980d9865a32

>   core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5

>   core/src/test/scala/integration/kafka/api/SSLConsumerTest.scala PRE-CREATION 
>   core/src/test/scala/integration/kafka/api/SSLProducerSendTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 8b14bcfe7af601fe4b0fb0a7c0c544e87403062a

>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala e4bf2df48dd59a251b646b7f96d63ec4b924fc0b

>   core/src/test/scala/unit/kafka/network/SocketServerTest.scala 7dc2fad542ea553ee888543dd215eb41ea57d509

>   core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 04a02e08a54139ee1a298c5354731bae009efef3

>   core/src/test/scala/unit/kafka/utils/TestUtils.scala eb169d8b33c27d598cc24e5a2e5f78b789fa38d3

> 
> Diff: https://reviews.apache.org/r/33620/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message