----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36333/#review91651 -----------------------------------------------------------
LGTM overall, just some minor comments below. clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 815) <https://reviews.apache.org/r/36333/#comment145173> Is this comment accurate? Could a sync commit throw exception when it is no longer retriable on returned Future? clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1071) <https://reviews.apache.org/r/36333/#comment145207> nit: not capitalizing the first letter just for comment consistency? clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java (line 175) <https://reviews.apache.org/r/36333/#comment145218> I think for in-function comments we will not capitalize comments, same below. clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java (line 186) <https://reviews.apache.org/r/36333/#comment145216> decapitalize "ensure that"? clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java (line 222) <https://reviews.apache.org/r/36333/#comment145215> missing one @param. clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java (line 234) <https://reviews.apache.org/r/36333/#comment145219> Same here and below regarding comments. clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java (line 615) <https://reviews.apache.org/r/36333/#comment145226> nit: Move this class right after sendOffsetCommitRequest()? clients/src/main/java/org/apache/kafka/clients/consumer/internals/DelayedTaskQueue.java (line 41) <https://reviews.apache.org/r/36333/#comment145227> Comment: remove all instances of the task in the queue? clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java (line 144) <https://reviews.apache.org/r/36333/#comment145229> Decapitalize comments, same below. clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClientTest.java (line 46) <https://reviews.apache.org/r/36333/#comment145233> Maybe rename to testSend(...) and below just for naming consistency? clients/src/test/java/org/apache/kafka/clients/consumer/internals/CoordinatorTest.java (line 286) <https://reviews.apache.org/r/36333/#comment145234> Did you check the "expected" tag works here? I have seen in some old junit versions the test will still pass even if the exception is now thrown. clients/src/test/java/org/apache/kafka/clients/consumer/internals/DelayedTaskQueueTest.java (line 62) <https://reviews.apache.org/r/36333/#comment145235> Rename to testRemove? clients/src/test/java/org/apache/kafka/clients/consumer/internals/HeartbeatTest.java (line 63) <https://reviews.apache.org/r/36333/#comment145236> testReset? clients/src/test/java/org/apache/kafka/clients/consumer/internals/RequestFutureTest.java (line 23) <https://reviews.apache.org/r/36333/#comment145237> testCompose... ? Same below. l, - Guozhang Wang On July 14, 2015, 8:21 p.m., Jason Gustafson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36333/ > ----------------------------------------------------------- > > (Updated July 14, 2015, 8:21 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2123 > https://issues.apache.org/jira/browse/KAFKA-2123 > > > Repository: kafka > > > Description > ------- > > KAFKA-2123; resolve problems from rebase > > > KAFKA-2123; address review comments > > > KAFKA-2123; more review fixes > > > KAFKA-2123; refresh metadata on listOffset failure > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java > fd98740bff175cc9d5bc02e365d88e011ef65d22 > > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerCommitCallback.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceCallback.java > 74dfdba0ecbca04947adba5eabb1cb5190a0cd5f > > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRecords.java > eb75d2e797e3aa3992e4cf74b12f51c8f1545e02 > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > b4e8f7f0dceefaf94a3495f39f5783cce5ceb25f > clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java > 46e26a665a22625d50888efa7b53472279f36e79 > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java > c1c8172cd45f6715262f9a6f497a7b1797a834a3 > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/DelayedTask.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/DelayedTaskQueue.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java > 695eaf63db9a5fa20dc2ca68957901462a96cd96 > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Heartbeat.java > 51eae1944d5c17cf838be57adf560bafe36fbfbd > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/NoAvailableBrokersException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestFuture.java > 13fc9af7392b4ade958daf3b0c9a165ddda351a6 > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestFutureAdapter.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestFutureListener.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/SendFailedException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/StaleMetadataException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java > 683745304c671952ff566f23b5dd4cf3ab75377a > > clients/src/main/java/org/apache/kafka/common/errors/ConsumerCoordinatorNotAvailableException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/errors/DisconnectException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/errors/IllegalGenerationException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/errors/NotCoordinatorForConsumerException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/errors/OffsetLoadInProgressException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/errors/UnknownConsumerIdException.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java > 4c0ecc3badd99727b5bd9d430364e61c184e0923 > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClientTest.java > PRE-CREATION > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/CoordinatorTest.java > d085fe5c9e2a0567893508a1c71f014fae6d7510 > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/DelayedTaskQueueTest.java > PRE-CREATION > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java > 405efdc7a59438731cbc3630876bda0042a3adb3 > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/HeartbeatTest.java > ee1ede01efa070409b86f5d8874cd578e058ce51 > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/RequestFutureTest.java > PRE-CREATION > core/src/test/scala/integration/kafka/api/ConsumerTest.scala > 92ffb91b5e039dc0d4cd0e072ca46db32f280cf9 > > Diff: https://reviews.apache.org/r/36333/diff/ > > > Testing > ------- > > > Thanks, > > Jason Gustafson > >
