Conversation
Hi team, The current wording of such exception : NotEnoughReplicasException: The size of the current ISR Set(2) is insufficient to satisfy the min.isr requirement of 2 for partition Makes it look like the ISR size is 2 and SHOULD satisfy the min.isr, while 2 in the Set(2) means only broker.id=2 is in ISR Submitting this minor PR to remove any confusion and improve the message, what do you think ? Thank you
| if (inSyncSize < minIsr && requiredAcks == -1) { | ||
| throw new NotEnoughReplicasException(s"The size of the current ISR ${partitionState.isr} " + | ||
| s"is insufficient to satisfy the min.isr requirement of $minIsr for partition $topicPartition") | ||
| throw new NotEnoughReplicasException(s"The size of the current ISR : $inSyncSize " + |
There was a problem hiding this comment.
I agree the message is misleading. Thanks for fixing. Do we need to report the size at all? Could we just report the current ISR?
s"The current ISR $inSyncReplicaIds for topic partition $topicPartition does not have enough replicas to satisfy min.insync.replicas=$minIsr"Or something like that?
There was a problem hiding this comment.
Hello @hachikuji , thank you for chiming in
While reviewing that part of the code history I understand the count was added on purpose, and not as a mistake done doing refactoring etc, that is why I kept it
I'm not sure it's useful, from my POV it would be best from a troubleshooting perspective to know which broker is out of ISR, but I wanted to keep that contribution simple :)
|
This PR is being marked as stale since it has not had any activity in 90 days. If you If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact). If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
omkreddy
left a comment
omkreddy
left a comment
There was a problem hiding this comment.
@nicolasguyomar Thanks for the PR. LGTM
…og-compaction-write-record-v2 * apache-github/trunk: (34 commits) MINOR: Bump year to 2025 in NOTICE file (apache#18427) KAFKA-18411 Remove ZkProducerIdManager (apache#18413) KAFKA-18408 tweak the 'tag' field for BrokerHeartbeatRequest.json, BrokerRegistrationChangeRecord.json and RegisterBrokerRecord.json (apache#18421) KAFKA-18414 Remove KRaftRegistrationResult (apache#18401) KAFKA-17921 Support SASL_PLAINTEXT protocol with java.security.auth.login.config (apache#17671) KAFKA-18384 Remove ZkAlterPartitionManager (apache#18364) KAFKA-10790: Add deadlock detection to producer#flush (apache#17946) KAFKA-18412: Remove EmbeddedZookeeper (apache#18399) MINOR : Improve Exception log in NotEnoughReplicasException(apache#12394) MINOR: Improve PlaintextAdminIntegrationTest#testConsumerGroups (apache#18409) MINOR: Remove unused local variable (apache#18410) MINOR: Remove RaftManager.maybeDeleteMetadataLogDir and AutoTopicCreationManagerTest.scala (apache#17365) KAFKA-18368 Remove TestUtils#MockZkConnect and remove zkConnect from TestUtils#createBrokerConfig (apache#18352) MINOR: Update Consumer group timeout default to 30 sec (apache#16406) MINOR: Fix typo in CommitRequestManager (apache#18407) MINOR: cleanup JavaDocs for deprecation warnings (apache#18402) KAFKA-18303; Update ShareCoordinator to use new record format (apache#18396) MINOR: Update Consumer and Producer JavaDocs for committing offsets (apache#18336) KAFKA-16446: Improve controller event duration logging (apache#15622) KAFKA-18388 test-kraft-server-start.sh should use log4j2.yaml (apache#18370) ...
3 participants
Hi team,
The current wording of such exception :
NotEnoughReplicasException: The size of the current ISR Set(2) is insufficient to satisfy the min.isr requirement of 2 for partition XMakes it look like the ISR size is 2 and SHOULD satisfy the min.isr, while 2 in the Set(2) means only broker.id=2 is in ISR
I did not find unit test to be updated that would check this exception wording
Submitting this minor PR to remove any confusion and improve the message, what do you think ?
Thank you
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)