[jira] [Commented] (KAFKA-339) using MultiFetch in the follower

["Joel Koshy (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/KAFKA-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13397097#comment-13397097 ] 

Joel Koshy commented on KAFKA-339:
----------------------------------

Thanks for the patch. Some comments:

AbstractFetcher:
- currentOffset should never be empty, so we can get rid of the if.
- hasPartition, partitionCount need to synchronize fetchMap
- "shutting down" should probably be removed from the string on line 106
- newOffset can be computed from the messageSet - so the processPartitionData implementation does not need to return the log end offset (and likewise when we use this in the high-level consumer). It's probably safer to prevent processPartitionData from overriding the new offset, and I don't see any benefit in allowing it to do so.

AbstractFetcherManager:
- addFetcher:
  - can rename to maybeAddFetcher
  - also, maybe we should move the info log on line 38 to the None case and print the fetcher id; and add another log for the other case saying there's already a fetcher.
- What is the purpose of having a fetcher ID vs simply topic-partition?
- Should synchronize fetcherRunnableMap in shutdown with mapLock

ReplicaManager:
- Maybe some corner case that I'm missing, but makeFollower already passes in the new leaderBrokerId so why do we need to re-read from ZooKeeper (line 173)?

ReplicaFetchTest:
- Ideally producer.close() should be before the waitUntilTrue
- The condition function uses & instead of &&.
- Also, instead of the hard-coded 60L I think it would be clearer and sufficient to do something like:
  val expectedOffset = brokers.head.getLogManager...logEndOffset
  assertEquals(brokers.size, brokers.count( broker => broker.getLogManager...logEndOffset == expectedOffset ))

                
> using MultiFetch in the follower
> --------------------------------
>
>                 Key: KAFKA-339
>                 URL: https://issues.apache.org/jira/browse/KAFKA-339
>             Project: Kafka
>          Issue Type: Sub-task
>          Components: core
>    Affects Versions: 0.8
>            Reporter: Jun Rao
>            Assignee: Jun Rao
>             Fix For: 0.8
>
>         Attachments: kafka-339_v1.patch
>
>   Original Estimate: 252h
>  Remaining Estimate: 252h
>
> A broker could be following multiple topic/partitions from the broker. Instead of using 1 fetcher thread per topic/partition, it would be more efficient to use 1 fetcher thread that issues multi-fetch requests.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira