-----------------------------------------------------------
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 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)
clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java
<https://reviews.apache.org/r/28769/#comment117835>
Do we need to remove final?
core/src/main/scala/kafka/client/ClientUtils.scala
<https://reviews.apache.org/r/28769/#comment117853>
Should we default protocolType to PLAINTEXT?
core/src/main/scala/kafka/cluster/Broker.scala
<https://reviews.apache.org/r/28769/#comment117854>
Instead of representing endpoints as a single string, should we represent
it as an array of strings, one for each endpoint?
core/src/main/scala/kafka/cluster/BrokerEndPoint.scala
<https://reviews.apache.org/r/28769/#comment117855>
For ease of reading, perhaps we can add a comment on what the uri format is?
core/src/main/scala/kafka/cluster/BrokerEndPoint.scala
<https://reviews.apache.org/r/28769/#comment117856>
Capitalize the first word of each sentence and end the sentence with . This
applies to a few other places too.
core/src/main/scala/kafka/consumer/ConsumerConfig.scala
<https://reviews.apache.org/r/28769/#comment117857>
I thought we won't support security in the old client?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/28769/#comment117861>
If host.name is not specified, should we generate PLAINTEXT://:6667
instead? When converting that to a BrokerEndPoint, do we get a null for host?
It will be useful to add a test for that.
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/28769/#comment117863>
Same comment as above, does a null string translate into a null in
endPoint.host?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/28769/#comment117862>
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?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/28769/#comment117859>
Would it be better to make this a sequence of string (like logDirs)?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/28769/#comment117858>
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.
- Jun Rao
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
>
>
