Skip to content

Conversation

@akrambek
Copy link
Contributor

No description provided.

@jfallows jfallows changed the title Queue as different kafka produce request if producId or produceEpoch varies Queue as different kafka produce request if producerId or producerEpoch varies May 25, 2024
Comment on lines 1638 to 1640
long producerId,
short producerEpoch,
int sequence)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move parameters producerId, producerEpoch and sequence between parameters timestamp and ackMode for consistent ordering with

        struct KafkaProduceDataEx
        {
            int32 deferred = 0;
            int64 timestamp = 0;
            int64 producerId = -1;
            int16 producerEpoch = -1;
            int32 sequence = -1;
            uint32 crc32c = 0;
            KafkaAckMode ackMode = IN_SYNC_REPLICAS;
            KafkaKey key;
            KafkaHeader[] headers;
        }

Comment on lines 1672 to 1707
if (latestRecordBatch != null &&
latestRecordBatch.producerId() == producerId &&
latestRecordBatch.producerEpoch() == producerEpoch &&
latestRecordBatch.baseSequence() <= sequence)
{
oldRecordBatchLength = latestRecordBatch.length();
producerId = latestRecordBatch.producerId();
producerEpoch = latestRecordBatch.producerEpoch();
sequence = latestRecordBatch.baseSequence();
recordCount = latestRecordBatch.recordCount() + 1;
attributes = latestRecordBatch.attributes();
encodeableRecordBatchTimestamp = latestRecordBatch.firstTimestamp();
encodeableRecordBatchTimestampMax = latestRecordBatch.maxTimestamp();
encodeSlotLimit = encodeableRecordBatchSlotOffset;
encodeableRecordBatchBytes -= latestRecordBatch.sizeof();
}

sequence = producerId == RECORD_BATCH_PRODUCER_ID_NONE ? RECORD_BATCH_BASE_SEQUENCE_NONE : sequence;

final int oldEncodeSlotLimit = encodeSlotLimit;

final RecordBatchFW recordBatch = recordBatchRW.wrap(encodeSlotBuffer, encodeSlotLimit, maxLimit)
.baseOffset(0)
.length(oldRecordBatchLength)
.leaderEpoch(-1)
.magic(RECORD_BATCH_MAGIC)
.crc(0)
.attributes(attributes)
.lastOffsetDelta(recordCount - 1)
.firstTimestamp(encodeableRecordBatchTimestamp)
.maxTimestamp(encodeableRecordBatchTimestampMax)
.producerId(producerId)
.producerEpoch(producerEpoch)
.baseSequence(sequence)
.recordCount(recordCount)
.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: the if condition above is checking latestRecordBatch.producerId() == producerId then assigning producerId = latestRecordBatch.producerId() (which is a no-op if they are already same).

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest we adjust the logic to simplify.

First let's conditionally check if a new record batch is needed (if no current batch, producer differs, etc), and create an empty batch (header) if needed, with record count 0 and record batch length for 0 records.

Then, let's dynamically extend the current record batch to increase the record count by 1, the record batch length by new record bytes, plus the record bytes too. This logic should work whether the record batch was just created (empty, no records yet) or already exists with previously encoded records in the batch.

Modifying the record batch record count and length should be feasible with direct update (like crc) instead of rewriting the record batch (header) entirely.

@jfallows jfallows merged commit 6ca94d2 into aklivity:develop May 26, 2024
@akrambek akrambek deleted the bug/946-3 branch May 26, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants