Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
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 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
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 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 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
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 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
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 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
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 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
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 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Cleanup in SharePartition acknowledgement functionality [kafka]
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 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
