You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/11/30 23:09:39 UTC

[GitHub] [kafka] hachikuji opened a new pull request #9663: MINOR: Small cleanups in `AlterIsr` handling logic

hachikuji opened a new pull request #9663:
URL: https://github.com/apache/kafka/pull/9663


   A few small cleanups in `Partition` handling of `AlterIsr`:
   
   - Factor state update and log message into `sendAlterIsrRequest`
   - Ensure illegal state error gets raised if a retry fails to be enqueued
   - Always check the proposed state against the current state in `handleAlterIsrResponse`
   - Add `toString` implementations to `IsrState` case classes
   
   ### 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

[GitHub] [kafka] hachikuji merged pull request #9663: MINOR: Small cleanups in `AlterIsr` handling logic

Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9663:
URL: https://github.com/apache/kafka/pull/9663


   


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

[GitHub] [kafka] chia7712 commented on a change in pull request #9663: MINOR: Small cleanups in `AlterIsr` handling logic

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9663:
URL: https://github.com/apache/kafka/pull/9663#discussion_r533126745



##########
File path: core/src/main/scala/kafka/cluster/Partition.scala
##########
@@ -1372,23 +1390,27 @@ class Partition(val topicPartition: TopicPartition,
    * Since our error was non-retryable we are okay staying in this state until we see new metadata from UpdateMetadata
    * or LeaderAndIsr
    */
-  private def handleAlterIsrResponse(proposedIsr: Set[Int], result: Either[Errors, LeaderAndIsr]): Unit = {
+  private def handleAlterIsrResponse(proposedIsrState: IsrState, result: Either[Errors, LeaderAndIsr]): Unit = {

Review comment:
       Could we rewrite it by currying?
   
   *Before*
   ```scala
       val callbackPartial = handleAlterIsrResponse(proposedIsrState, _ : Either[Errors, LeaderAndIsr])
   
       if (!alterIsrManager.enqueue(AlterIsrItem(topicPartition, newLeaderAndIsr, callbackPartial))) {
         throw new IllegalStateException(s"Failed to enqueue `AlterIsr` request with state " +
           s"$newLeaderAndIsr for partition $topicPartition")
       }
   ```
   
   *After*
   ```scala
   
       if (!alterIsrManager.enqueue(AlterIsrItem(topicPartition, newLeaderAndIsr, handleAlterIsrResponse(proposedIsrState)))) {
         throw new IllegalStateException(s"Failed to enqueue `AlterIsr` request with state " +
           s"$newLeaderAndIsr for partition $topicPartition")
       }
   ```




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

[GitHub] [kafka] chia7712 commented on pull request #9663: MINOR: Small cleanups in `AlterIsr` handling logic

Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9663:
URL: https://github.com/apache/kafka/pull/9663#issuecomment-736671682


   +1 to last commit


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