You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "adixitconfluent (via GitHub)" <gi...@apache.org> on 2026/01/02 09:50:18 UTC
[PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
adixitconfluent opened a new pull request, #21236:
URL: https://github.com/apache/kafka/pull/21236
### About
This PR performs cleanup in `SharePartition.java` acknowledgement flow. We do not need `Objects.requireNonNull(recordState);` firstly because this has been covered in this [code](https://github.com/apache/kafka/blob/trunk/core/src/main/java/kafka/server/share/SharePartition.java#L1000). Also throwing a `NullPointerException` is indeed wrong for an invalid acknowledge type provided by the client.
### Testing
Added some unit tests.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #21236:
URL: https://github.com/apache/kafka/pull/21236#discussion_r2657521385
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2387,7 +2386,6 @@ private Optional<Throwable> acknowledgeCompleteBatch(
// either released or moved to a state where member id existence is not important. The member id
// is only important when the batch is acquired.
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
- Objects.requireNonNull(recordState);
Review Comment:
RecordState recordState = Objects.requireNonNull(ACK_TYPE_TO_RECORD_STATE.get(ackType));
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
Posted by "adixitconfluent (via GitHub)" <gi...@apache.org>.
adixitconfluent commented on code in PR #21236:
URL: https://github.com/apache/kafka/pull/21236#discussion_r2657530241
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2387,7 +2386,6 @@ private Optional<Throwable> acknowledgeCompleteBatch(
// either released or moved to a state where member id existence is not important. The member id
// is only important when the batch is acquired.
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
- Objects.requireNonNull(recordState);
Review Comment:
I don't think we require `Objects.requireNonNull()`since this code would be covered when we perform `fetchAckTypeMapForBatch` and it should be covered there. Hence, I thought to remove it.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #21236:
URL: https://github.com/apache/kafka/pull/21236#discussion_r2657565779
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2387,7 +2386,6 @@ private Optional<Throwable> acknowledgeCompleteBatch(
// either released or moved to a state where member id existence is not important. The member id
// is only important when the batch is acquired.
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
- Objects.requireNonNull(recordState);
Review Comment:
For now ofcourse yes, but the check was placed so one do not forget to update the `ACK_TYPE_TO_RECORD_STATE` map while adding new values.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
Posted by "adixitconfluent (via GitHub)" <gi...@apache.org>.
adixitconfluent commented on code in PR #21236:
URL: https://github.com/apache/kafka/pull/21236#discussion_r2657589045
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2387,7 +2386,6 @@ private Optional<Throwable> acknowledgeCompleteBatch(
// either released or moved to a state where member id existence is not important. The member id
// is only important when the batch is acquired.
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
- Objects.requireNonNull(recordState);
Review Comment:
then maybe lets not have `Objects.requireNonNull` since it throws a `NullPointerException`, rather we throw an `IllegalRequestException` or `InvalidRequestException`, wdyt?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
Posted by "adixitconfluent (via GitHub)" <gi...@apache.org>.
adixitconfluent commented on code in PR #21236:
URL: https://github.com/apache/kafka/pull/21236#discussion_r2657530241
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2387,7 +2386,6 @@ private Optional<Throwable> acknowledgeCompleteBatch(
// either released or moved to a state where member id existence is not important. The member id
// is only important when the batch is acquired.
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
- Objects.requireNonNull(recordState);
Review Comment:
I don't think we require `Objects.requireNonNull()`since this code would be covered when we perform `fetchAckTypeMapForBatch` and it should be [covered there](https://github.com/apache/kafka/blob/trunk/core/src/main/java/kafka/server/share/SharePartition.java#L996). Hence, I thought to remove it.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 merged PR #21236:
URL: https://github.com/apache/kafka/pull/21236
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #21236:
URL: https://github.com/apache/kafka/pull/21236#discussion_r2657713207
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2387,7 +2386,6 @@ private Optional<Throwable> acknowledgeCompleteBatch(
// either released or moved to a state where member id existence is not important. The member id
// is only important when the batch is acquired.
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
- Objects.requireNonNull(recordState);
Review Comment:
Anything is fine but some exception whould highlight the map is missing the required value.
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2387,7 +2386,6 @@ private Optional<Throwable> acknowledgeCompleteBatch(
// either released or moved to a state where member id existence is not important. The member id
// is only important when the batch is acquired.
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
- Objects.requireNonNull(recordState);
Review Comment:
Anything is fine but some exception should highlight the map is missing the required value.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
Posted by "adixitconfluent (via GitHub)" <gi...@apache.org>.
adixitconfluent commented on code in PR #21236:
URL: https://github.com/apache/kafka/pull/21236#discussion_r2657776869
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2387,7 +2386,6 @@ private Optional<Throwable> acknowledgeCompleteBatch(
// either released or moved to a state where member id existence is not important. The member id
// is only important when the batch is acquired.
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
- Objects.requireNonNull(recordState);
Review Comment:
done
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org