[jira] [Commented] (KAFKA-8400) Do not update follower replica state if the log read failed

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

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

ASF GitHub Bot commented on KAFKA-8400:
---------------------------------------

hachikuji commented on pull request #6814: KAFKA-8400; Do not update follower replica state if the log read failed
URL: https://github.com/apache/kafka/pull/6814
 
 
   This patch checks for errors handling a fetch request before updating follower state. Previously we were unsafely passing the failed `LogReadResult` with most fields set to -1 into `Replica` to update follower state. Additionally, this patch attempts to improve the test coverage for ISR shrinking and expansion logic in `Partition`.
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   
 
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> Do not update follower replica state if the log read failed
> -----------------------------------------------------------
>
>                 Key: KAFKA-8400
>                 URL: https://issues.apache.org/jira/browse/KAFKA-8400
>             Project: Kafka
>          Issue Type: Improvement
>            Reporter: Jason Gustafson
>            Assignee: Jason Gustafson
>            Priority: Major
>
> In {{ReplicaManager.fetchMessages}}, we have the following logic to read from the log and update follower state:
> {code}
>     val result = readFromLocalLog(
>         replicaId = replicaId,
>         fetchOnlyFromLeader = fetchOnlyFromLeader,
>         fetchIsolation = fetchIsolation,
>         fetchMaxBytes = fetchMaxBytes,
>         hardMaxBytesLimit = hardMaxBytesLimit,
>         readPartitionInfo = fetchInfos,
>         quota = quota)
>       if (isFromFollower) updateFollowerLogReadResults(replicaId, result)
>       else result
> {code}
> The call to {{readFromLocalLog}} could fail for many reasons, in which case we return a LogReadResult with an error set and all fields set to -1. The problem is that we do not check for the error when updating the follower state. As far as I can tell, this does not cause any correctness issues, but we're just asking for trouble. It would be better to check the error before proceeding to `Partition.updateReplicaLogReadResult`. 
> Perhaps even better would be to have {{readFromLocalLog}} return something like {{Either[LogReadResult, Errors]}} so that we are forced to handle the error.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)