Skip to content

Conversation

@adixitconfluent
Copy link
Contributor

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. Also throwing a NullPointerException is indeed wrong for an invalid acknowledge type provided by the client.

Testing

Added some unit tests.

@github-actions github-actions bot added triage PRs from the community core Kafka Broker KIP-932 Queues for Kafka small Small PRs labels Jan 2, 2026
@AndrewJSchofield AndrewJSchofield added ci-approved and removed triage PRs from the community labels Jan 2, 2026
Copy link
Contributor

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, minor comment.

Comment on lines 2388 to -2390
RecordState recordState = ACK_TYPE_TO_RECORD_STATE.get(ackType);
Objects.requireNonNull(recordState);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecordState recordState = Objects.requireNonNull(ACK_TYPE_TO_RECORD_STATE.get(ackType));

Copy link
Contributor Author

@adixitconfluent adixitconfluent Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then maybe lets not have Objects.requireNonNull since it throws a NullPointerException, rather we throw an IllegalRequestException or InvalidRequestException, wdyt?

Copy link
Contributor

@apoorvmittal10 apoorvmittal10 Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything is fine but some exception should highlight the map is missing the required value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Looks good to me.

Copy link
Contributor

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change, LGTM!

@apoorvmittal10 apoorvmittal10 merged commit aef1fac into apache:trunk Jan 3, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-approved core Kafka Broker KIP-932 Queues for Kafka small Small PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants