[ 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)