smjn commented on PR #20815: URL: https://github.com/apache/kafka/pull/20815#issuecomment-3481350997
> Overall looks fine. A few comments to address above. Though the above approach looks fine but I feel that the number of changes can be lowered down if we enhance function `fetchRecordState(byte acknowledgeType)` by adding `case 4 /* RENEW */ -> RecordState.ACQUIRED;` and continue using function `fetchRecordStateMapForAcknowledgementBatch`. Why are we not taking that approach rather than creating `fetchAckTypeMapForBatch` > > Regarding tests, I think we should also add some tests for the following scenarios - > > 1. LSO movement is happening while some offsets/batches are in renewal mode. > 2. combination of per-batch and per-offsets tracked in flight state for renewal acknowledgement Rather than fetching global transitions from existing method, the global hashmap was considered a slightly better approach, hence the overall mechanics were changed. The state for RENEW was deliberately avoided to signify that there is no transition involved when RENEWal happens. Will add 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
