> 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. > > 4. The following unit test fails consistently. > > kafka.network.SocketServerTest > testSocketsCloseOnShutdown FAILED > > org.scalatest.junit.JUnitTestFailedError: expected exception when > > writing to closed plain socket > > at > > org.scalatest.junit.AssertionsForJUnit$class.newAssertionFailedException(AssertionsForJUnit.scala:101) > > at > > kafka.network.SocketServerTest.newAssertionFailedException(SocketServerTest.scala:37) > > at org.scalatest.Assertions$class.fail(Assertions.scala:711) > > at kafka.network.SocketServerTest.fail(SocketServerTest.scala:37) > > at > > kafka.network.SocketServerTest.testSocketsCloseOnShutdown(SocketServerTest.scala:162)
1. Since the tools go major rewrites now (SimpleConsumer may go away, ConsumerOffsetChecker went away...) and since some tools doesn't really need to support security protocols at all (I think? I definitely can't see why then need to support both SSL and Kerberos), I'd rather modify the tools as a separate patch once we have security implemented. The larger part of the change will be implementing SSL/Kerberos in the tools anyway... 3. I know :( chasing commits between reviews is the biggest pain here... 4. Will check it out. > On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/client/ClientUtils.scala, line 111 > > <https://reviews.apache.org/r/28769/diff/17/?file=846148#file846148line111> > > > > Should we default protocolType to PLAINTEXT? I prefer not to default. Makes it more challenging to catch cases where the wrong protocol is used accidentaly. Do you see a case for defaulting here? > On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 148-149 > > <https://reviews.apache.org/r/28769/diff/17/?file=846168#file846168line148> > > > > Do we use PLAINTEXT://0.0.0.0:9092 to bind to all interface or > > PLAINTEXT://:9092? If it's the former, does it work for ipv6 and do we need > > to change getListeners() accordingly? 0.0.0.0 binds to all interfaces. Missing hostname binds to default interface. Its pretty standard (i.e. matches the NIO bind behavior, and the one for lower level sockets too) and works the same way as it did before this patch. > On Feb. 11, 2015, 9:45 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, line 186 > > <https://reviews.apache.org/r/28769/diff/17/?file=846168#file846168line186> > > > > This should default to 0.8.2.0. The user will upgrade the broker jar > > with the default setting first, followed by another rolling bounce after > > changing the config to 0.8.3.0. Actually hardcoding the version means having to remember to change this with every release. I'll try to figure something more reasonable for this. - Gwen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28769/#review71916 ----------------------------------------------------------- On Feb. 3, 2015, 6:52 p.m., Gwen Shapira wrote: > > ----------------------------------------------------------- > 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 > https://issues.apache.org/jira/browse/KAFKA-1809 > > > Repository: kafka > > > Description > ------- > > changed topicmetadata to include brokerendpoints and fixed few unit tests > > > fixing systest and support for binding to default address > > > fixed system tests > > > fix default address binding and ipv6 support > > > fix some issues regarding endpoint parsing. Also, larger segments for systest > make the validation much faster > > > added link to security wiki in doc > > > fixing unit test after rename of ProtocolType to SecurityProtocol > > > Following Joe's advice, added security protocol enum on client side, and > modified protocol to use ID instead of string. > > > validate producer config against enum > > > add a second protocol for testing and modify SocketServerTests to check on > multi-ports > > > Reverted the metadata request changes and removed the explicit security > protocol argument. Instead the socketserver will determine the protocol based > on the port and add this to the request > > > bump version for UpdateMetadataRequest and added support for rolling upgrades > with new config > > > following tests - fixed LeaderAndISR protocol and ZK registration for > backward compatibility > > > cleaned up some changes that were actually not necessary. hopefully making > this patch slightly easier to review > > > undoing some changes that don't belong here > > > bring back config lost in cleanup > > > fixes neccessary for an all non-plaintext cluster to work > > > minor modifications following comments by Jun > > > added missing license > > > formatting > > > clean up imports > > > cleaned up V2 to not include host+port field. Using use.new.protocol flag to > decide which version to serialize > > > change endpoints collection in Broker to Map[protocol, endpoint], mostly to > be clear that we intend to have one endpoint per protocol > > > validate that listeners and advertised listeners have unique ports and > protocols > > > support legacy configs > > > some fixes following rebase > > > Reverted to backward compatible zk registration, changed use.new.protocol to > support multiple versions and few minor fixes > > > fixing some issues after rebase > > > modified inter.broker.protocol config to start with security per feedback > > > rebasing on trunk again > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > 9a43d668376295f47cea0b8eb26b15a6c73bb39c > clients/src/main/java/org/apache/kafka/common/protocol/ApiVersion.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java > 0186783e1abd0438d6720d035ee1903b51691e09 > clients/src/main/java/org/apache/kafka/common/utils/Utils.java > 8a305b0fb46563e9e683c327d9e18de532f291de > clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java > 4c2ea34815b63174732d58b699e1a0a9e6ec3b6f > config/server.properties 1614260b71a658b405bb24157c8f12b1f1031aa5 > core/src/main/scala/kafka/admin/AdminUtils.scala > 28b12c7b89a56c113b665fbde1b95f873f8624a3 > core/src/main/scala/kafka/admin/TopicCommand.scala > 285c0333ff43543d3e46444c1cd9374bb883bb59 > core/src/main/scala/kafka/api/ConsumerMetadataResponse.scala > 24aaf954dc42e2084454fa5fc9e8f388ea95c756 > core/src/main/scala/kafka/api/LeaderAndIsrRequest.scala > 4ff7e8f8cc695551dd5d2fe65c74f6b6c571e340 > core/src/main/scala/kafka/api/TopicMetadata.scala > 0190076df0adf906ecd332284f222ff974b315fc > core/src/main/scala/kafka/api/TopicMetadataResponse.scala > 92ac4e687be22e4800199c0666bfac5e0059e5bb > core/src/main/scala/kafka/api/UpdateMetadataRequest.scala > 530982e36b17934b8cc5fb668075a5342e142c59 > core/src/main/scala/kafka/client/ClientUtils.scala > ebba87f0566684c796c26cb76c64b4640a5ccfde > core/src/main/scala/kafka/cluster/Broker.scala > 0060add008bb3bc4b0092f2173c469fce0120be6 > core/src/main/scala/kafka/cluster/BrokerEndPoint.scala PRE-CREATION > core/src/main/scala/kafka/cluster/EndPoint.scala PRE-CREATION > core/src/main/scala/kafka/cluster/SecurityProtocol.scala PRE-CREATION > core/src/main/scala/kafka/common/BrokerEndPointNotAvailableException.scala > PRE-CREATION > core/src/main/scala/kafka/consumer/ConsumerConfig.scala > 9ebbee6c16dc83767297c729d2d74ebbd063a993 > core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala > b9e2bea7b442a19bcebd1b350d39541a8c9dd068 > core/src/main/scala/kafka/consumer/ConsumerFetcherThread.scala > ee6139c901082358382c5ef892281386bf6fc91b > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala > 5487259751ebe19f137948249aa1fd2637d2deb4 > core/src/main/scala/kafka/controller/ControllerChannelManager.scala > 14b22ab38cc143c8b8d8617d253c5dfd5f5c7b7e > core/src/main/scala/kafka/controller/KafkaController.scala > 66df6d2fbdbdd556da6bea0df84f93e0472c8fbf > core/src/main/scala/kafka/javaapi/ConsumerMetadataResponse.scala > d281bb31a66fd749ecddfbe38479b6903f436831 > core/src/main/scala/kafka/javaapi/TopicMetadata.scala > f384e04678df10a5b46a439f475c63371bf8e32b > core/src/main/scala/kafka/network/RequestChannel.scala > 7b1db3dbbb2c0676f166890f566c14aa248467ab > core/src/main/scala/kafka/network/SocketServer.scala > 39b1651b680b2995cedfde95d74c086d9c6219ef > core/src/main/scala/kafka/producer/ProducerPool.scala > 43df70bb461dd3e385e6b20396adef3c4016a3fc > core/src/main/scala/kafka/server/AbstractFetcherManager.scala > 20c00cb8cc2351950edbc8cb1752905a0c26e79f > core/src/main/scala/kafka/server/AbstractFetcherThread.scala > 8c281d4668f92eff95a4a5df3c03c4b5b20e7095 > core/src/main/scala/kafka/server/KafkaApis.scala > f2b027bf944e735fd52cc282690ec1b8395f9290 > core/src/main/scala/kafka/server/KafkaConfig.scala > 6d74983472249eac808d361344c58cc2858ec971 > core/src/main/scala/kafka/server/KafkaHealthcheck.scala > 4acdd70fe9c1ee78d6510741006c2ece65450671 > core/src/main/scala/kafka/server/KafkaServer.scala > 89200da30a04943f0b9befe84ab17e62b747c8c4 > core/src/main/scala/kafka/server/MetadataCache.scala > bf81a1ab88c14be8697b441eedbeb28fa0112643 > core/src/main/scala/kafka/server/ReplicaFetcherManager.scala > 351dbbad3bdb709937943b336a5b0a9e0162a5e2 > core/src/main/scala/kafka/server/ReplicaFetcherThread.scala > 6879e730282185bda3d6bc3659cb15af0672cecf > core/src/main/scala/kafka/server/ReplicaManager.scala > fb948b9ab28c516e81dab14dcbe211dcd99842b6 > core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala > d1e7c434e77859d746b8dc68dd5d5a3740425e79 > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala > ba6ddd7a909df79a0f7d45e8b4a2af94ea0fceb6 > core/src/main/scala/kafka/tools/SimpleConsumerShell.scala > b4f903b6c7c3bb725cac7c05eb1f885906413c4d > core/src/main/scala/kafka/tools/UpdateOffsetsInZK.scala > 111c9a8b94ce45d95551482e9fd3f8c1cccbf548 > core/src/main/scala/kafka/utils/Utils.scala > 738c1af9ef5de16fdf5130daab69757a14c48b5c > core/src/main/scala/kafka/utils/ZkUtils.scala > c14bd455b6642f5e6eb254670bef9f57ae41d6cb > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > 90c0b7a19c7af8e5416e4bdba62b9824f1abd5ab > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala > b15237b76def3b234924280fa3fdb25dbb0cc0dc > core/src/test/scala/other/kafka/TestOffsetManager.scala > 41f334d48897b3027ed54c58bbf4811487d3b191 > core/src/test/scala/unit/kafka/KafkaConfigTest.scala > 4d36b8b1173f60d43463c13c9d8c1275a84c8c28 > core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala > 1bf2667f47853585bc33ffb3e28256ec5f24ae84 > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > a1f72f8c2042ff2a43af503b2e06f84706dad9db > core/src/test/scala/unit/kafka/cluster/BrokerTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala > c0355cc0135c6af2e346b4715659353a31723b86 > core/src/test/scala/unit/kafka/integration/FetcherTest.scala > 25845abbcad2e79f56f729e59239b738d3ddbc9d > core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala > ef4c9aeaa2711567915f036fb82bedcf5d106e2b > core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala > 35dc071b1056e775326981573c9618d8046e601d > core/src/test/scala/unit/kafka/log/LogTest.scala > c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e > core/src/test/scala/unit/kafka/network/SocketServerTest.scala > 78b431f9c88cca1bc5e430ffd41083d0e92b7e75 > core/src/test/scala/unit/kafka/producer/AsyncProducerTest.scala > 1db6ac329f7b54e600802c8a623f80d159d4e69b > core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala > d60d8e0f49443f4dc8bc2cad6e2f951eda28f5cb > core/src/test/scala/unit/kafka/server/AdvertiseBrokerTest.scala > f0c4a56b61b4f081cf4bee799c6e9c523ff45e19 > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 82dce80d553957d8b5776a9e140c346d4e07f766 > core/src/test/scala/unit/kafka/server/LeaderElectionTest.scala > c2ba07c5fdbaf0e65ca033b2e4d88f45a8a15b2e > core/src/test/scala/unit/kafka/server/LogOffsetTest.scala > c06ee756bf0fe07e5d3c92823a476c960b37afd6 > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 54755e8dd3f23ced313067566cd4ea867f8a496e > system_test/replication_testsuite/testcase_1/testcase_1_properties.json > 0c6d7a316cc6b51ac0755ca03558507db0706c31 > system_test/utils/kafka_system_test_utils.py > 41d511cbc310fa87e0f2cd2f772e479e8e3ae4e2 > > Diff: https://reviews.apache.org/r/28769/diff/ > > > Testing > ------- > > > Thanks, > > Gwen Shapira > >
