[
https://issues.apache.org/jira/browse/KAFKA-351?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13437038#comment-13437038
]
Neha Narkhede commented on KAFKA-351:
-------------------------------------
Thanks for the patch! Overall, this refactoring is a good change. Here are a
few review comments -
1. TestUtils. How about renaming leaderLocalOnBroker to isLeaderLocalOnBroker ?
2. LogOffsetTest:
2.1. The change in testGetOffsetsForUnknownTopic doesn't look right. Since the
topic "foo" doesn't exist, the client should get back UnknownTopicException.
The partition is not invalid, it doesn't even exist.
3. SyncProducerTest
3.1 Same here, client should get back UnknownTopicException, not
InvalidPartitionException
4. ISRExpirationTest
4.1 getLogWithLogEndOffset expected 6 calls for log.logEndOffset since the test
exercised that API 6 times during correct operation. If you change it to
anyTimes, it will hide problems with either the test or the code. Was it
changed to
get rid of some transient test failure ?
4.2 Minor formatting: For consistency, you might want to change to first letter
caps for error messages. So far, I don't think everyone quite followed this. So
some log statements have first letter caps, others don't. I personally prefer
first letter caps for all log statements.
5. Replica
5.1 Is there a reason why logEndOffsetUpdateTimeMs is not AtomicLong ? It's
access is not protected by a lock.
5.2 What is the difference between private[this] var and private var ?
5.3 It's great that you changed logEndOffset to use the new getter/setter API
convention. I think there is only one drawback to using that. I don't know a
way to search the code to list all places that use the setter. Do you ?
6. Partition
6.1 Rename addReplicaIfNotExist to addReplicaIfNotExists.
6.2 In getOrCreateLog, it is better to use case match, since in Scala case
match always evaluates to some value. Since this API needs to return the
Replica object, using case match will protect against code bugs. Instead of
if-else that checks isDefined, case-match handles Options naturally, since it
forces you to handle all the cases. Same for makeLeader, makeFollower,
checkEnoughReplicasReachAnOffset since they also return some value.
6.3 Looks like assignedReplicaMap is meant to be a map of replica_id->Replica.
It might be a good idea to change Pool to handle Options. Options are much
easier to use than handling null values. For example, getReplica can reduce to
just returning assignedReplicaMap.get(replicaId), instead of the if-else
checking for nulls.
6.5 Minor formatting comment same as 4.2
6.6. maybeIncrementLeaderHW: Since you are trying to access inSyncReplicas
here, this method should be synchronized on the leaderAndIsrUpdateLock
6.7 getOutOfSyncReplicas, updateISR: Same as 6.6
6.8 checkEnoughReplicasReachAnOffset:
6.8.1 We should probably rename this to checkEnoughReplicasReachOffset.
7. ReplicaManager
7.1 I think leaderReplicas was a poorly chosen name by me in the past. It
should be renamed to leaderPartitions since it is the set of partitions with
their leader hosted on the local broker. Also, that would mean we should rename
leaderReplicasLock to leaderPartitionsLock
7.2 Same as 6.3 for allPartitions. This will greatly simplify
getOrCreatePartition
7.3 Same as 4.2 for some of the APIs
7.4 Fix typo: shuttedd down
7.5 Fix identation and parenthesis style for checkpointHighWatermarks.
7.6 Same as 6.2 for becomeLeaderOrFollower
7.7 I wonder if it is better to rename leaderReplicaIfLocalOrException to
getLeaderReplicaIfLocal ?
> Refactor some new components introduced for replication
> --------------------------------------------------------
>
> Key: KAFKA-351
> URL: https://issues.apache.org/jira/browse/KAFKA-351
> Project: Kafka
> Issue Type: Bug
> Affects Versions: 0.8
> Reporter: Neha Narkhede
> Assignee: Jun Rao
> Labels: optimization
> Fix For: 0.8
>
> Attachments: kafka-351_v1.patch, kafka-351_v2.patch,
> kafka-351_v3.patch
>
>
> Jay had some good refactoring suggestions as part of the review for KAFKA-46.
> I'd like to file this umbrella JIRA with individual sub tasks to cover those
> suggestions
--
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
