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

Ben Stopford edited comment on KAFKA-5036 at 4/10/17 6:42 PM:
--------------------------------------------------------------

Points 2, 5 are addressed in PR: https://github.com/apache/kafka/pull/2831
Point 3 is addressed in PR: https://github.com/apache/kafka/pull/2821 (now 
merged)
Point 4 has been addressed (fixing tests). Changed the log line to an assert 
(which [~junrao] and I discussed previously)


was (Author: benstopford):
Points 2, 5 are addressed in PR: https://github.com/apache/kafka/pull/2831
Point 3 is addressed in PR: https://github.com/apache/kafka/pull/2821 (now 
merged)
Point 4 is an issue only in the test. The LogTest sets up a Log with an empty 
epoch cache. Not addressed at this time.  

> Followups from KIP-101
> ----------------------
>
>                 Key: KAFKA-5036
>                 URL: https://issues.apache.org/jira/browse/KAFKA-5036
>             Project: Kafka
>          Issue Type: Improvement
>    Affects Versions: 0.11.0.0
>            Reporter: Jun Rao
>            Assignee: Jun Rao
>             Fix For: 0.11.0.0
>
>
> 1. It would be safer to hold onto the leader lock in Partition while serving 
> an OffsetForLeaderEpoch request.
> 2. Currently, we update the leader epoch in epochCache after log append in 
> the follower but before log append in the leader. It would be more consistent 
> to always do this after log append. This also avoids issues related to 
> failure in log append.
> 3. OffsetsForLeaderEpochRequest/OffsetsForLeaderEpochResponse:
> The code that does grouping can probably be replaced by calling 
> CollectionUtils.groupDataByTopic(). Done: 
> https://github.com/apache/kafka/commit/359a68510801a22630a7af275c9935fb2d4c8dbf
> 4. The following line in LeaderEpochFileCache is hit several times when 
> LogTest is executed:
> {code}
>        if (cachedLatestEpoch == None) error("Attempt to assign log end offset 
> to epoch before epoch has been set. This should never happen.")
> {code}
> This should be an assert (with the tests fixed up)
> 5. The constructor of LeaderEpochFileCache has the following:
> {code}
> lock synchronized { ListBuffer(checkpoint.read(): _*) }
> {code}
> But everywhere else uses a read or write lock. We should use consistent 
> locking.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Reply via email to