From ef5bf7e3f4e1298586258cd85b8369aee6594dd7 Mon Sep 17 00:00:00 2001 From: Sumant Tambe Date: Wed, 20 Dec 2017 11:22:52 -0800 Subject: [PATCH 1/4] KAFKA-5886: Introduce delivery.timeout.ms producer config --- .../kafka/clients/producer/KafkaProducer.java | 52 ++- .../clients/producer/ProducerConfig.java | 9 +- .../producer/internals/ProducerBatch.java | 101 +++-- .../producer/internals/RecordAccumulator.java | 368 +++++++++++------- .../clients/producer/internals/Sender.java | 6 +- .../apache/kafka/common/config/ConfigDef.java | 12 +- .../org/apache/kafka/clients/MockClient.java | 6 +- .../producer/internals/ProducerBatchTest.java | 32 +- .../internals/RecordAccumulatorTest.java | 307 ++++++++++++--- .../producer/internals/SenderTest.java | 230 +++++++++-- .../internals/TransactionManagerTest.java | 6 +- .../apache/kafka/connect/runtime/Worker.java | 1 + .../scala/unit/kafka/utils/TestUtils.scala | 5 + 13 files changed, 820 insertions(+), 315 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java index 3991467b676c6..5735381b3d4ef 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java @@ -16,6 +16,17 @@ */ package org.apache.kafka.clients.producer; +import java.net.InetSocketAddress; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Properties; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.apache.kafka.clients.ApiVersions; import org.apache.kafka.clients.ClientUtils; import org.apache.kafka.clients.KafkaClient; @@ -24,6 +35,7 @@ import org.apache.kafka.clients.consumer.KafkaConsumer; import org.apache.kafka.clients.consumer.OffsetAndMetadata; import org.apache.kafka.clients.consumer.OffsetCommitCallback; +import org.apache.kafka.clients.producer.internals.BufferPool; import org.apache.kafka.clients.producer.internals.ProducerInterceptors; import org.apache.kafka.clients.producer.internals.ProducerMetrics; import org.apache.kafka.clients.producer.internals.RecordAccumulator; @@ -69,18 +81,6 @@ import org.apache.kafka.common.utils.Time; import org.slf4j.Logger; -import java.net.InetSocketAddress; -import java.util.Collections; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Properties; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.Future; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; - import static org.apache.kafka.common.serialization.ExtendedSerializer.Wrapper.ensureExtended; /** @@ -235,6 +235,7 @@ public class KafkaProducer implements Producer { private static final AtomicInteger PRODUCER_CLIENT_ID_SEQUENCE = new AtomicInteger(1); private static final String JMX_PREFIX = "kafka.producer"; public static final String NETWORK_THREAD_PREFIX = "kafka-producer-network-thread"; + public static final String PRODUCER_METRIC_GROUP_NAME = "producer-metrics"; private final String clientId; // Visible for testing @@ -392,18 +393,22 @@ public KafkaProducer(Properties properties, Serializer keySerializer, Seriali int retries = configureRetries(config, transactionManager != null, log); int maxInflightRequests = configureInflightRequests(config, transactionManager != null); short acks = configureAcks(config, transactionManager != null, log); + long deliveryTimeoutMs = configureDeliveryTimeout(config); this.apiVersions = new ApiVersions(); this.accumulator = new RecordAccumulator(logContext, config.getInt(ProducerConfig.BATCH_SIZE_CONFIG), - this.totalMemorySize, this.compressionType, config.getLong(ProducerConfig.LINGER_MS_CONFIG), retryBackoffMs, + this.requestTimeoutMs, + deliveryTimeoutMs, metrics, + PRODUCER_METRIC_GROUP_NAME, time, apiVersions, - transactionManager); + transactionManager, + new BufferPool(this.totalMemorySize, config.getInt(ProducerConfig.BATCH_SIZE_CONFIG), metrics, time, PRODUCER_METRIC_GROUP_NAME)); List addresses = ClientUtils.parseAndValidateAddresses(config.getList(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG)); if (metadata != null) { this.metadata = metadata; @@ -459,6 +464,25 @@ public KafkaProducer(Properties properties, Serializer keySerializer, Seriali } } + private static long configureDeliveryTimeout(ProducerConfig config) { + long deliveryTimeoutMs = config.getLong(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG); + long lingerMs = config.getLong(ProducerConfig.LINGER_MS_CONFIG); + int requestTimeoutMs = config.getInt(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG); + + // when the sum of lingerMs and requestTimeoutMs overflows a long we make sure deliveryTimeoutMs is at least + // equal to lingerMs. + boolean overflow = lingerMs + requestTimeoutMs < 0L; + boolean invalid = overflow ? deliveryTimeoutMs < lingerMs : deliveryTimeoutMs < lingerMs + requestTimeoutMs; + + if (invalid) { + throw new ConfigException("Must set " + ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG + " higher than " + + ProducerConfig.LINGER_MS_CONFIG + " + " + + ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG); + } + + return deliveryTimeoutMs; + } + private static TransactionManager configureTransactionState(ProducerConfig config, LogContext logContext, Logger log) { TransactionManager transactionManager = null; diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java b/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java index 8e7b662e6df14..d12af75e82f0c 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java @@ -99,6 +99,12 @@ public class ProducerConfig extends AbstractConfig { + "specified time waiting for more records to show up. This setting defaults to 0 (i.e. no delay). Setting " + LINGER_MS_CONFIG + "=5, " + "for example, would have the effect of reducing the number of requests sent but would add up to 5ms of latency to records sent in the absence of load."; + /** delivery.timeout.ms */ + public static final String DELIVERY_TIMEOUT_MS_CONFIG = "delivery.timeout.ms"; + private static final String DELIVERY_TIMEOUT_MS_DOC = "An upper bound on the time to report success or failure after Producer.send() returns. " + + "Producer may report failure to send a message earlier than this config if all the retries are exhausted or " + + "a record is added to a batch nearing expiration."; + /** client.id */ public static final String CLIENT_ID_CONFIG = CommonClientConfigs.CLIENT_ID_CONFIG; @@ -224,7 +230,7 @@ public class ProducerConfig extends AbstractConfig { static { CONFIG = new ConfigDef().define(BOOTSTRAP_SERVERS_CONFIG, Type.LIST, Collections.emptyList(), new ConfigDef.NonNullValidator(), Importance.HIGH, CommonClientConfigs.BOOTSTRAP_SERVERS_DOC) .define(BUFFER_MEMORY_CONFIG, Type.LONG, 32 * 1024 * 1024L, atLeast(0L), Importance.HIGH, BUFFER_MEMORY_DOC) - .define(RETRIES_CONFIG, Type.INT, 0, between(0, Integer.MAX_VALUE), Importance.HIGH, RETRIES_DOC) + .define(RETRIES_CONFIG, Type.INT, Integer.MAX_VALUE, between(0, Integer.MAX_VALUE), Importance.HIGH, RETRIES_DOC) .define(ACKS_CONFIG, Type.STRING, "1", @@ -234,6 +240,7 @@ public class ProducerConfig extends AbstractConfig { .define(COMPRESSION_TYPE_CONFIG, Type.STRING, "none", Importance.HIGH, COMPRESSION_TYPE_DOC) .define(BATCH_SIZE_CONFIG, Type.INT, 16384, atLeast(0), Importance.MEDIUM, BATCH_SIZE_DOC) .define(LINGER_MS_CONFIG, Type.LONG, 0, atLeast(0L), Importance.MEDIUM, LINGER_MS_DOC) + .define(DELIVERY_TIMEOUT_MS_CONFIG, Type.LONG, 120 * 1000, atLeast(0L), Importance.MEDIUM, DELIVERY_TIMEOUT_MS_DOC) .define(CLIENT_ID_CONFIG, Type.STRING, "", Importance.MEDIUM, CommonClientConfigs.CLIENT_ID_DOC) .define(SEND_BUFFER_CONFIG, Type.INT, 128 * 1024, atLeast(-1), Importance.MEDIUM, CommonClientConfigs.SEND_BUFFER_DOC) .define(RECEIVE_BUFFER_CONFIG, Type.INT, 32 * 1024, atLeast(-1), Importance.MEDIUM, CommonClientConfigs.RECEIVE_BUFFER_DOC) diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java b/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java index ea0f0f7dff953..17beb6dfa0eac 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java @@ -58,7 +58,7 @@ public final class ProducerBatch { private enum FinalState { ABORTED, FAILED, SUCCEEDED } - final long createdMs; + public final long createdMs; final TopicPartition topicPartition; final ProduceRequestResult produceFuture; @@ -77,13 +77,13 @@ private enum FinalState { ABORTED, FAILED, SUCCEEDED } private boolean retry; private boolean reopened = false; - public ProducerBatch(TopicPartition tp, MemoryRecordsBuilder recordsBuilder, long now) { - this(tp, recordsBuilder, now, false); + public ProducerBatch(TopicPartition tp, MemoryRecordsBuilder recordsBuilder, long creationTime) { + this(tp, recordsBuilder, creationTime, false); } - public ProducerBatch(TopicPartition tp, MemoryRecordsBuilder recordsBuilder, long now, boolean isSplitBatch) { - this.createdMs = now; - this.lastAttemptMs = now; + public ProducerBatch(TopicPartition tp, MemoryRecordsBuilder recordsBuilder, long creationTime, boolean isSplitBatch) { + this.createdMs = creationTime; + this.lastAttemptMs = creationTime; this.recordsBuilder = recordsBuilder; this.topicPartition = tp; this.lastAppendTime = createdMs; @@ -158,7 +158,17 @@ public void abort(RuntimeException exception) { } /** - * Complete the request. If the batch was previously aborted, this is a no-op. + * Finalize the state of a batch. Final state, once set, is immutable. This function may be called + * once or twice on a batch. It may be called twice if + * 1. An inflight batch expires before a response from the broker is received. The batch's final + * state is set to FAILED. But it could succeed on the broker and second time around batch.done() may + * try to set SUCCEEDED final state. + * 2. If a transaction abortion happens or if the producer is closed forcefully, the final state is + * ABORTED but again it could succeed if broker responds with a success. + * + * Attempted transitions from [FAILED | ABORTED] --> SUCCEEDED are logged. + * Attempted transitions from one failure state to the same or a different failed state are ignored. + * Attempted transitions from SUCCEEDED to the same or a failed state throw an exception. * * @param baseOffset The base offset of the messages assigned by the server * @param logAppendTime The log append time or -1 if CreateTime is being used @@ -166,26 +176,32 @@ public void abort(RuntimeException exception) { * @return true if the batch was completed successfully and false if the batch was previously aborted */ public boolean done(long baseOffset, long logAppendTime, RuntimeException exception) { - final FinalState finalState; - if (exception == null) { + final FinalState tryFinalState = (exception == null) ? FinalState.SUCCEEDED : FinalState.FAILED; + + if (tryFinalState == FinalState.SUCCEEDED) { log.trace("Successfully produced messages to {} with base offset {}.", topicPartition, baseOffset); - finalState = FinalState.SUCCEEDED; } else { log.trace("Failed to produce messages to {}.", topicPartition, exception); - finalState = FinalState.FAILED; } - if (!this.finalState.compareAndSet(null, finalState)) { - if (this.finalState.get() == FinalState.ABORTED) { - log.debug("ProduceResponse returned for {} after batch had already been aborted.", topicPartition); - return false; - } else { - throw new IllegalStateException("Batch has already been completed in final state " + this.finalState.get()); + if (this.finalState.compareAndSet(null, tryFinalState)) { + completeFutureAndFireCallbacks(baseOffset, logAppendTime, exception); + return true; + } + + if (this.finalState.get() != FinalState.SUCCEEDED) { + if (tryFinalState == FinalState.SUCCEEDED) { + // Log if a previously unsuccessful batch succeeded later on. + log.debug("ProduceResponse returned {} for {} after batch had already been {}.", tryFinalState, + topicPartition, this.finalState.get()); } + // FAILED --> ABORTED and ABORTED --> FAILED transitions are ignored. + } else { + // A SUCCESSFUL batch must not attempt another state change. + throw new IllegalStateException("A " + this.finalState.get() + " batch must not attempt another state change to " + tryFinalState); } - completeFutureAndFireCallbacks(baseOffset, logAppendTime, exception); - return true; + return false; } private void completeFutureAndFireCallbacks(long baseOffset, long logAppendTime, RuntimeException exception) { @@ -300,29 +316,37 @@ public String toString() { } /** - * A batch whose metadata is not available should be expired if one of the following is true: - *
    - *
  1. the batch is not in retry AND request timeout has elapsed after it is ready (full or linger.ms has reached). - *
  2. the batch is in retry AND request timeout has elapsed after the backoff period ended. - *
+ * Expire the batch if current time has passed the delivery timeout of the batch. + * The delivery timeout is measured from batch creation time instead of batch close time + * because in some cases batch close may be arbitrarily delayed. * This methods closes this batch and sets {@code expiryErrorMessage} if the batch has timed out. */ - boolean maybeExpire(int requestTimeoutMs, long retryBackoffMs, long now, long lingerMs, boolean isFull) { - if (!this.inRetry() && isFull && requestTimeoutMs < (now - this.lastAppendTime)) - expiryErrorMessage = (now - this.lastAppendTime) + " ms has passed since last append"; - else if (!this.inRetry() && requestTimeoutMs < (createdTimeMs(now) - lingerMs)) - expiryErrorMessage = (createdTimeMs(now) - lingerMs) + " ms has passed since batch creation plus linger time"; - else if (this.inRetry() && requestTimeoutMs < (waitedTimeMs(now) - retryBackoffMs)) - expiryErrorMessage = (waitedTimeMs(now) - retryBackoffMs) + " ms has passed since last attempt plus backoff time"; + //boolean maybeExpire(long deliveryTimeoutMs, long now) { + // if (deliveryTimeoutMs <= (now - this.createdMs)) { + // expire(now); + // return true; + // } + // return false; + //} - boolean expired = expiryErrorMessage != null; - if (expired) - abortRecordAppends(); - return expired; + boolean hasReachedDeliveryTimeout(long deliveryTimeoutMs, long now) { + return deliveryTimeoutMs <= now - this.createdMs; } /** - * If {@link #maybeExpire(int, long, long, long, boolean)} returned true, the sender will fail the batch with + * Expire the batch. This methods closes this batch and sets {@code expiryErrorMessage}. + */ + void expire(long now) { + expiryErrorMessage = (now - this.createdMs) + " ms has passed since batch creation"; + abortRecordAppends(); + } + + public FinalState finalState() { + return this.finalState.get(); + } + + /** + * If {@link #hasReachedDeliveryTimeout(long, long)} returned true, the sender will fail the batch with * the exception returned by this method. * @return An exception indicating the batch expired. */ @@ -468,4 +492,9 @@ public boolean sequenceHasBeenReset() { return reopened; } + boolean soonToExpire(long deliveryTimeoutMs, long now, int requestTimeoutMs) { + // We don't add anything in deliveryTimeoutMs because it may overflow + // when deliveryTimeoutMs==Long.MAX_VALUE + return deliveryTimeoutMs <= now + requestTimeoutMs - createdMs; + } } diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java b/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java index 31c6d754c9d92..1f459a9b23184 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java @@ -16,6 +16,21 @@ */ package org.apache.kafka.clients.producer.internals; +import java.nio.ByteBuffer; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Iterator; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.kafka.clients.ApiVersions; import org.apache.kafka.clients.producer.Callback; import org.apache.kafka.common.Cluster; @@ -34,10 +49,10 @@ import org.apache.kafka.common.record.AbstractRecords; import org.apache.kafka.common.record.CompressionRatioEstimator; import org.apache.kafka.common.record.CompressionType; -import org.apache.kafka.common.record.Record; -import org.apache.kafka.common.record.RecordBatch; import org.apache.kafka.common.record.MemoryRecords; import org.apache.kafka.common.record.MemoryRecordsBuilder; +import org.apache.kafka.common.record.Record; +import org.apache.kafka.common.record.RecordBatch; import org.apache.kafka.common.record.TimestampType; import org.apache.kafka.common.utils.CopyOnWriteMap; import org.apache.kafka.common.utils.LogContext; @@ -45,20 +60,6 @@ import org.apache.kafka.common.utils.Utils; import org.slf4j.Logger; -import java.nio.ByteBuffer; -import java.util.ArrayDeque; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Deque; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Iterator; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.atomic.AtomicInteger; - /** * This class acts as a queue that accumulates records into {@link MemoryRecords} * instances to be sent to the server. @@ -76,6 +77,8 @@ public final class RecordAccumulator { private final CompressionType compression; private final long lingerMs; private final long retryBackoffMs; + private final int requestTimeoutMs; + private final long deliveryTimeoutMs; private final BufferPool free; private final Time time; private final ApiVersions apiVersions; @@ -86,12 +89,15 @@ public final class RecordAccumulator { private int drainIndex; private final TransactionManager transactionManager; + // A per-partition queue of batches ordered by creation time for quick access of the oldest batch + private final ConcurrentMap> soonToExpireInFlightBatches; + private long nextBatchExpiryTimeMs; // the earliest time (absolute) a batch will expire. + /** * Create a new record accumulator * * @param logContext The log context used for logging * @param batchSize The size to use when allocating {@link MemoryRecords} instances - * @param totalSize The maximum memory the record accumulator can use. * @param compression The compression codec for the records * @param lingerMs An artificial delay time to add before declaring a records instance that isn't full ready for * sending. This allows time for more records to arrive. Setting a non-zero lingerMs will trade off some @@ -106,14 +112,17 @@ public final class RecordAccumulator { */ public RecordAccumulator(LogContext logContext, int batchSize, - long totalSize, CompressionType compression, long lingerMs, long retryBackoffMs, + int requestTimeoutMs, + long deliveryTimeoutMs, Metrics metrics, + String metricGrpName, Time time, ApiVersions apiVersions, - TransactionManager transactionManager) { + TransactionManager transactionManager, + BufferPool bufferPool) { this.log = logContext.logger(RecordAccumulator.class); this.drainIndex = 0; this.closed = false; @@ -123,14 +132,17 @@ public RecordAccumulator(LogContext logContext, this.compression = compression; this.lingerMs = lingerMs; this.retryBackoffMs = retryBackoffMs; + this.requestTimeoutMs = requestTimeoutMs; + this.deliveryTimeoutMs = deliveryTimeoutMs; + this.nextBatchExpiryTimeMs = Long.MAX_VALUE; this.batches = new CopyOnWriteMap<>(); - String metricGrpName = "producer-metrics"; - this.free = new BufferPool(totalSize, batchSize, metrics, time, metricGrpName); + this.free = bufferPool; this.incomplete = new IncompleteBatches(); this.muted = new HashMap<>(); this.time = time; this.apiVersions = apiVersions; this.transactionManager = transactionManager; + this.soonToExpireInFlightBatches = new ConcurrentHashMap<>(); registerMetrics(metrics, metricGrpName); } @@ -240,7 +252,7 @@ public RecordAppendResult append(TopicPartition tp, private MemoryRecordsBuilder recordsBuilder(ByteBuffer buffer, byte maxUsableMagic) { if (transactionManager != null && maxUsableMagic < RecordBatch.MAGIC_VALUE_V2) { throw new UnsupportedVersionException("Attempting to use idempotence with a broker which does not " + - "support the required message format (v2). The broker must be version 0.11 or later."); + "support the required message format (v2). The broker must be version 0.11 or later."); } return MemoryRecords.builder(buffer, maxUsableMagic, compression, TimestampType.CREATE_TIME, 0L); } @@ -254,7 +266,7 @@ private MemoryRecordsBuilder recordsBuilder(ByteBuffer buffer, byte maxUsableMag * if it is expired, or when the producer is closed. */ private RecordAppendResult tryAppend(long timestamp, byte[] key, byte[] value, Header[] headers, - Callback callback, Deque deque) { + Callback callback, Deque deque) { ProducerBatch last = deque.peekLast(); if (last != null) { FutureRecordMetadata future = last.tryAppend(timestamp, key, value, headers, callback, time.milliseconds()); @@ -273,37 +285,39 @@ private boolean isMuted(TopicPartition tp, long now) { return result; } + private void maybeUpdateNextBatchExpiryTime(ProducerBatch batch) { + // We determine the earliest time any batch may expire. We use + // this value later on to determine the max time we can sleep in poll. + // We assume that the batch at the front of the deque will always be the next to expire. + // This may not be true if max.in.flight.requests.per.connection > 1 and retries happen. + // Watch for overflow in createdMs + deliveryTimeoutMs when deliveryTimeoutMs is Long.MAX_VALUE + nextBatchExpiryTimeMs = (batch.createdMs + deliveryTimeoutMs < 0) ? nextBatchExpiryTimeMs + : Math.min(nextBatchExpiryTimeMs, batch.createdMs + deliveryTimeoutMs); + } + /** * Get a list of batches which have been sitting in the accumulator too long and need to be expired. */ - public List expiredBatches(int requestTimeout, long now) { + public List expiredBatches(long now) { List expiredBatches = new ArrayList<>(); for (Map.Entry> entry : this.batches.entrySet()) { - Deque dq = entry.getValue(); - TopicPartition tp = entry.getKey(); - // We only check if the batch should be expired if the partition does not have a batch in flight. - // This is to prevent later batches from being expired while an earlier batch is still in progress. - // Note that `muted` is only ever populated if `max.in.flight.request.per.connection=1` so this protection - // is only active in this case. Otherwise the expiration order is not guaranteed. - if (!isMuted(tp, now)) { - synchronized (dq) { - // iterate over the batches and expire them if they have been in the accumulator for more than requestTimeOut - ProducerBatch lastBatch = dq.peekLast(); - Iterator batchIterator = dq.iterator(); - while (batchIterator.hasNext()) { - ProducerBatch batch = batchIterator.next(); - boolean isFull = batch != lastBatch || batch.isFull(); - // Check if the batch has expired. Expired batches are closed by maybeExpire, but callbacks - // are invoked after completing the iterations, since sends invoked from callbacks - // may append more batches to the deque being iterated. The batch is deallocated after - // callbacks are invoked. - if (batch.maybeExpire(requestTimeout, retryBackoffMs, now, this.lingerMs, isFull)) { + // Expire inflight batches in the order of draining. Expire if the final state of the + // batch is not known by (batch's creation time + deliveryTimeoutMs). + List mayExpireBatches = soonToExpireInFlightBatches.get(entry.getKey()); + if (mayExpireBatches != null) { + Iterator iter = mayExpireBatches.iterator(); + while (iter.hasNext()) { + ProducerBatch batch = iter.next(); + + if (batch.hasReachedDeliveryTimeout(deliveryTimeoutMs, now)) { + iter.remove(); + if (batch.finalState() == null) { + batch.expire(now); expiredBatches.add(batch); - batchIterator.remove(); - } else { - // Stop at the first batch that has not expired. - break; } + } else { + maybeUpdateNextBatchExpiryTime(batch); + break; } } } @@ -312,10 +326,12 @@ public List expiredBatches(int requestTimeout, long now) { } /** - * Re-enqueue the given record batch in the accumulator to retry + * Re-enqueue the given record batch in the accumulator to retry. + * Remove the batch from the list of inflight batches. */ public void reenqueue(ProducerBatch batch, long now) { batch.reenqueued(now); + maybeRemoveFromSoonToExpire(batch); Deque deque = getOrCreateDeque(batch.topicPartition); synchronized (deque) { if (transactionManager != null) @@ -325,6 +341,13 @@ public void reenqueue(ProducerBatch batch, long now) { } } + private void maybeRemoveFromSoonToExpire(ProducerBatch batch) { + List soonToExpireBatches = soonToExpireInFlightBatches.get(batch.topicPartition); + if (soonToExpireBatches != null) { + soonToExpireBatches.remove(batch); + } + } + /** * Split the big batch that has been rejected and reenqueue the split batches in to the accumulator. * @return the number of split batches. @@ -334,7 +357,7 @@ public int splitAndReenqueue(ProducerBatch bigBatch) { // is bigger. There are several different ways to do the reset. We chose the most conservative one to ensure // the split doesn't happen too often. CompressionRatioEstimator.setEstimation(bigBatch.topicPartition.topic(), compression, - Math.max(1.0f, (float) bigBatch.compressionRatio())); + Math.max(1.0f, (float) bigBatch.compressionRatio())); Deque dq = bigBatch.split(this.batchSize); int numSplitBatches = dq.size(); Deque partitionDequeue = getOrCreateDeque(bigBatch.topicPartition); @@ -356,8 +379,8 @@ public int splitAndReenqueue(ProducerBatch bigBatch) { } // We will have to do extra work to ensure the queue is in order when requests are being retried and there are - // multiple requests in flight to that partition. If the first inflight request fails to append, then all the subsequent - // in flight requests will also fail because the sequence numbers will not be accepted. + // multiple requests in flight to that partition. If the first in flight request fails to append, then all the + // subsequent in flight requests will also fail because the sequence numbers will not be accepted. // // Further, once batches are being retried, we are reduced to a single in flight request for that partition. So when // the subsequent batches come back in sequence order, they will have to be placed further back in the queue. @@ -368,12 +391,12 @@ public int splitAndReenqueue(ProducerBatch bigBatch) { private void insertInSequenceOrder(Deque deque, ProducerBatch batch) { // When we are requeing and have enabled idempotence, the reenqueued batch must always have a sequence. if (batch.baseSequence() == RecordBatch.NO_SEQUENCE) - throw new IllegalStateException("Trying to reenqueue a batch which doesn't have a sequence even " + - "though idempotence is enabled."); + throw new IllegalStateException("Trying to re-enqueue a batch which doesn't have a sequence even " + + "though idempotency is enabled."); if (transactionManager.nextBatchBySequence(batch.topicPartition) == null) - throw new IllegalStateException("We are reenqueueing a batch which is not tracked as part of the in flight " + - "requests. batch.topicPartition: " + batch.topicPartition + "; batch.baseSequence: " + batch.baseSequence()); + throw new IllegalStateException("We are re-enqueueing a batch which is not tracked as part of the in flight " + + "requests. batch.topicPartition: " + batch.topicPartition + "; batch.baseSequence: " + batch.baseSequence()); ProducerBatch firstBatchInQueue = deque.peekFirst(); if (firstBatchInQueue != null && firstBatchInQueue.hasSequence() && firstBatchInQueue.baseSequence() < batch.baseSequence()) { @@ -390,7 +413,7 @@ private void insertInSequenceOrder(Deque deque, ProducerBatch bat orderedBatches.add(deque.pollFirst()); log.debug("Reordered incoming batch with sequence {} for partition {}. It was placed in the queue at " + - "position {}", batch.baseSequence(), batch.topicPartition, orderedBatches.size()); + "position {}", batch.baseSequence(), batch.topicPartition, orderedBatches.size()); // Either we have reached a point where there are batches without a sequence (ie. never been drained // and are hence in order by default), or the batch at the front of the queue has a sequence greater // than the incoming batch. This is the right place to add the incoming batch. @@ -484,6 +507,120 @@ public boolean hasUndrained() { return false; } + + private boolean shouldStop(ProducerBatch first, TopicPartition tp) { + ProducerIdAndEpoch producerIdAndEpoch = null; + if (transactionManager != null) { + if (!transactionManager.isSendToPartitionAllowed(tp)) + return true; + + producerIdAndEpoch = transactionManager.producerIdAndEpoch(); + if (!producerIdAndEpoch.isValid()) + // we cannot send the batch until we have refreshed the producer id + return true; + + if (!first.hasSequence() && transactionManager.hasUnresolvedSequence(first.topicPartition)) + // Don't drain any new batches while the state of previous sequence numbers + // is unknown. The previous batches would be unknown if they were aborted + // on the client after being sent to the broker at least once. + return true; + + int firstInFlightSequence = transactionManager.firstInFlightSequence(first.topicPartition); + if (firstInFlightSequence != RecordBatch.NO_SEQUENCE && first.hasSequence() + && first.baseSequence() != firstInFlightSequence) + // If the queued batch already has an assigned sequence, then it is being retried. + // In this case, we wait until the next immediate batch is ready and drain that. + // We only move on when the next in line batch is complete (either successfully or due to + // a fatal broker error). This effectively reduces our in flight request count to 1. + return true; + } + return false; + } + + private List drainBatchesForOneNode(Cluster cluster, Node node, int maxSize, long now) { + int size = 0; + List parts = cluster.partitionsForNode(node.id()); + List ready = new ArrayList<>(); + /* to make starvation less likely this loop doesn't start at 0 */ + int start = drainIndex = drainIndex % parts.size(); + do { + PartitionInfo part = parts.get(drainIndex); + TopicPartition tp = new TopicPartition(part.topic(), part.partition()); + this.drainIndex = (this.drainIndex + 1) % parts.size(); + + // Only proceed if the partition has no in-flight batches. + if (isMuted(tp, now)) + continue; + + Deque deque = getDeque(tp); + if (deque == null) + continue; + + synchronized (deque) { + // invariant: !isMuted(tp,now) && deque != null + ProducerBatch first = deque.peekFirst(); + if (first == null) + continue; + + // first != null + boolean backoff = first.attempts() > 0 && first.waitedTimeMs(now) < retryBackoffMs; + // Only drain the batch if it is not during backoff period. + if (backoff) + continue; + + if (size + first.estimatedSizeInBytes() > maxSize && !ready.isEmpty()) { + // there is a rare case that a single batch size is larger than the request size due to + // compression; in this case we will still eventually send this batch in a single request + break; + } else { + if (shouldStop(first, tp)) + break; + + boolean isTransactional = transactionManager != null ? transactionManager.isTransactional() : false; + ProducerIdAndEpoch producerIdAndEpoch = + transactionManager != null ? transactionManager.producerIdAndEpoch() : null; + ProducerBatch batch = deque.pollFirst(); + if (producerIdAndEpoch != null && !batch.hasSequence()) { + // If the batch already has an assigned sequence, then we should not change the producer id and + // sequence number, since this may introduce duplicates. In particular, the previous attempt + // may actually have been accepted, and if we change the producer id and sequence here, this + // attempt will also be accepted, causing a duplicate. + // + // Additionally, we update the next sequence number bound for the partition, and also have + // the transaction manager track the batch so as to ensure that sequence ordering is maintained + // even if we receive out of order responses. + batch.setProducerState(producerIdAndEpoch, transactionManager.sequenceNumber(batch.topicPartition), isTransactional); + transactionManager.incrementSequenceNumber(batch.topicPartition, batch.recordCount); + log.debug("Assigned producerId {} and producerEpoch {} to batch with base sequence " + + "{} being sent to partition {}", producerIdAndEpoch.producerId, + producerIdAndEpoch.epoch, batch.baseSequence(), tp); + + transactionManager.addInFlightBatch(batch); + } + + batch.close(); + size += batch.records().sizeInBytes(); + ready.add(batch); + // Put this batch in the list of soon-to-expire-inflight-batches because we might have to expire + // it if we don't know its final state by deliveryTimeoutMs. If the expiry is farther than + // requestTimeoutMs, we don't have to keep track of this batch because it will either succeed or + // fail (due to request timeout) much sooner. + if (batch.soonToExpire(deliveryTimeoutMs, now, requestTimeoutMs)) { + List inflightBatchList = soonToExpireInFlightBatches.get(batch.topicPartition); + if (inflightBatchList == null) { + inflightBatchList = new LinkedList<>(); + soonToExpireInFlightBatches.put(tp, inflightBatchList); + } + inflightBatchList.add(batch); + } + batch.drained(now); + } + } + } while (start != drainIndex); + return ready; + } + + /** * Drain all the data for the given nodes and collate them into a list of batches that will fit within the specified * size on a per-node basis. This method attempts to avoid choosing the same topic-node over and over. @@ -494,106 +631,43 @@ public boolean hasUndrained() { * @param now The current unix time in milliseconds * @return A list of {@link ProducerBatch} for each node specified with total size less than the requested maxSize. */ - public Map> drain(Cluster cluster, - Set nodes, - int maxSize, - long now) { + public Map> drain(Cluster cluster, Set nodes, int maxSize, long now) { if (nodes.isEmpty()) return Collections.emptyMap(); Map> batches = new HashMap<>(); for (Node node : nodes) { - int size = 0; - List parts = cluster.partitionsForNode(node.id()); - List ready = new ArrayList<>(); - /* to make starvation less likely this loop doesn't start at 0 */ - int start = drainIndex = drainIndex % parts.size(); - do { - PartitionInfo part = parts.get(drainIndex); - TopicPartition tp = new TopicPartition(part.topic(), part.partition()); - // Only proceed if the partition has no in-flight batches. - if (!isMuted(tp, now)) { - Deque deque = getDeque(tp); - if (deque != null) { - synchronized (deque) { - ProducerBatch first = deque.peekFirst(); - if (first != null) { - boolean backoff = first.attempts() > 0 && first.waitedTimeMs(now) < retryBackoffMs; - // Only drain the batch if it is not during backoff period. - if (!backoff) { - if (size + first.estimatedSizeInBytes() > maxSize && !ready.isEmpty()) { - // there is a rare case that a single batch size is larger than the request size due - // to compression; in this case we will still eventually send this batch in a single - // request - break; - } else { - ProducerIdAndEpoch producerIdAndEpoch = null; - boolean isTransactional = false; - if (transactionManager != null) { - if (!transactionManager.isSendToPartitionAllowed(tp)) - break; - - producerIdAndEpoch = transactionManager.producerIdAndEpoch(); - if (!producerIdAndEpoch.isValid()) - // we cannot send the batch until we have refreshed the producer id - break; - - isTransactional = transactionManager.isTransactional(); - - if (!first.hasSequence() && transactionManager.hasUnresolvedSequence(first.topicPartition)) - // Don't drain any new batches while the state of previous sequence numbers - // is unknown. The previous batches would be unknown if they were aborted - // on the client after being sent to the broker at least once. - break; - - int firstInFlightSequence = transactionManager.firstInFlightSequence(first.topicPartition); - if (firstInFlightSequence != RecordBatch.NO_SEQUENCE && first.hasSequence() - && first.baseSequence() != firstInFlightSequence) - // If the queued batch already has an assigned sequence, then it is being - // retried. In this case, we wait until the next immediate batch is ready - // and drain that. We only move on when the next in line batch is complete (either successfully - // or due to a fatal broker error). This effectively reduces our - // in flight request count to 1. - break; - } - - ProducerBatch batch = deque.pollFirst(); - if (producerIdAndEpoch != null && !batch.hasSequence()) { - // If the batch already has an assigned sequence, then we should not change the producer id and - // sequence number, since this may introduce duplicates. In particular, - // the previous attempt may actually have been accepted, and if we change - // the producer id and sequence here, this attempt will also be accepted, - // causing a duplicate. - // - // Additionally, we update the next sequence number bound for the partition, - // and also have the transaction manager track the batch so as to ensure - // that sequence ordering is maintained even if we receive out of order - // responses. - batch.setProducerState(producerIdAndEpoch, transactionManager.sequenceNumber(batch.topicPartition), isTransactional); - transactionManager.incrementSequenceNumber(batch.topicPartition, batch.recordCount); - log.debug("Assigned producerId {} and producerEpoch {} to batch with base sequence " + - "{} being sent to partition {}", producerIdAndEpoch.producerId, - producerIdAndEpoch.epoch, batch.baseSequence(), tp); - - transactionManager.addInFlightBatch(batch); - } - batch.close(); - size += batch.records().sizeInBytes(); - ready.add(batch); - batch.drained(now); - } - } - } - } - } - } - this.drainIndex = (this.drainIndex + 1) % parts.size(); - } while (start != drainIndex); + List ready = drainBatchesForOneNode(cluster, node, maxSize, now); batches.put(node.id(), ready); } + + maybeUpdateNextExpiryTime(now); return batches; } + /** + * The earliest absolute time a batch will expire (in milliseconds) + */ + public Long nextExpiryTimeMs() { + return this.nextBatchExpiryTimeMs; + } + + /** + * If there are batches to soon to expire, but now is past the last known earliest expiry time, + * we reset the expiry time to now, because it's the smallest possible valid absolute time in future. + * This could happen when there were no ready nodes for a while and clock went past the last known earliest + * expiry time. If there're no batches to soon expire, we simply reset it to the largest possible value. + * This ensures that next delivery timeout keeps moving ahead. + */ + void maybeUpdateNextExpiryTime(long now) { + if (now >= nextBatchExpiryTimeMs) + nextBatchExpiryTimeMs = soonToExpireInFlightBatches.isEmpty() ? Long.MAX_VALUE : now; + } + + ConcurrentMap> soonToExpireInFlightBatches() { + return soonToExpireInFlightBatches; + } + private Deque getDeque(TopicPartition tp) { return batches.get(tp); } @@ -785,4 +859,4 @@ public ReadyCheckResult(Set readyNodes, long nextReadyCheckDelayMs, Set expiredBatches = this.accumulator.expiredBatches(this.requestTimeoutMs, now); + List expiredBatches = this.accumulator.expiredBatches(now); // Reset the producer id if an expired batch has previously been sent to the broker. Also update the metrics // for expired batches. see the documentation of @TransactionState.resetProducerId to understand why // we need to reset the producer id here. @@ -301,6 +301,9 @@ private long sendProducerData(long now) { // that isn't yet sendable (e.g. lingering, backing off). Note that this specifically does not include nodes // with sendable data that aren't ready to send since they would cause busy looping. long pollTimeout = Math.min(result.nextReadyCheckDelayMs, notReadyTimeout); + pollTimeout = Math.min(pollTimeout, this.accumulator.nextExpiryTimeMs() - now); + pollTimeout = Math.max(pollTimeout, 0); + if (!result.readyNodes.isEmpty()) { log.trace("Nodes with data ready to send: {}", result.readyNodes); // if some partitions are already ready to be sent, the select time would be 0; @@ -644,6 +647,7 @@ private void failBatch(ProducerBatch batch, long baseOffset, long logAppendTime, */ private boolean canRetry(ProducerBatch batch, ProduceResponse.PartitionResponse response) { return batch.attempts() < this.retries && + batch.finalState() == null && ((response.error.exception() instanceof RetriableException) || (transactionManager != null && transactionManager.canRetry(response, batch))); } diff --git a/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java b/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java index c9efb82e061c5..7ad4c31fb5057 100644 --- a/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java +++ b/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java @@ -57,7 +57,7 @@ * Map<String, String> props = new HashMap<>(); * props.put("config_with_default", "some value"); * props.put("config_with_dependents", "some other value"); - * + * * Map<String, Object> configs = defs.parse(props); * // will return "some value" * String someConfig = (String) configs.get("config_with_default"); @@ -595,10 +595,10 @@ private void validate(String name, Map parsed, Map recommendedValues = key.recommender.validValues(name, parsed); @@ -845,9 +845,9 @@ public static class Range implements Validator { private final Number min; private final Number max; - private Range(Number min, Number max) { + private Range(Number min, Number maxInclusive) { this.min = min; - this.max = max; + this.max = maxInclusive; } /** @@ -860,7 +860,7 @@ public static Range atLeast(Number min) { } /** - * A numeric range that checks both the upper and lower bound + * A numeric range that checks both the upper (inclusive) and lower bound */ public static Range between(Number min, Number max) { return new Range(min, max); diff --git a/clients/src/test/java/org/apache/kafka/clients/MockClient.java b/clients/src/test/java/org/apache/kafka/clients/MockClient.java index 6b41a9e877934..0a88f6963c062 100644 --- a/clients/src/test/java/org/apache/kafka/clients/MockClient.java +++ b/clients/src/test/java/org/apache/kafka/clients/MockClient.java @@ -280,7 +280,6 @@ public List poll(long timeoutMs, long now) { checkTimeoutOfPendingRequests(now); List copy = new ArrayList<>(this.responses); - if (metadata != null && metadata.updateRequested()) { MetadataUpdate metadataUpdate = metadataUpdates.poll(); if (cluster != null) @@ -351,7 +350,10 @@ public void respondToRequest(ClientRequest clientRequest, AbstractResponse respo public void respond(AbstractResponse response, boolean disconnected) { - ClientRequest request = requests.remove(); + ClientRequest request = null; + if (requests.size() > 0) { + request = requests.remove(); + } short version = request.requestBuilder().latestAllowedVersion(); responses.add(new ClientResponse(request.makeHeader(version), request.callback(), request.destination(), request.createdTimeMs(), time.milliseconds(), disconnected, null, null, response)); diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/ProducerBatchTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/ProducerBatchTest.java index 2f89d7949e702..6a85449a65985 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/ProducerBatchTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/ProducerBatchTest.java @@ -226,40 +226,30 @@ public void testSplitPreservesMagicAndCompressionType() { } /** - * A {@link ProducerBatch} configured using a very large linger value and a timestamp preceding its create - * time is interpreted correctly as not expired when the linger time is larger than the difference - * between now and create time by {@link ProducerBatch#maybeExpire(int, long, long, long, boolean)}. + * A {@link ProducerBatch} configured using a timestamp preceding its create time is interpreted correctly + * as not expired by {@link ProducerBatch#hasReachedDeliveryTimeout(long, long)}. */ @Test - public void testLargeLingerOldNowExpire() { + public void testBatchExpiration() { + long deliveryTimeoutMs = 10240; ProducerBatch batch = new ProducerBatch(new TopicPartition("topic", 1), memoryRecordsBuilder, now); // Set `now` to 2ms before the create time. - assertFalse(batch.maybeExpire(10240, 100L, now - 2L, Long.MAX_VALUE, false)); + assertFalse(batch.hasReachedDeliveryTimeout(deliveryTimeoutMs, now - 2)); + // Set `now` to deliveryTimeoutMs. + assertTrue(batch.hasReachedDeliveryTimeout(deliveryTimeoutMs, now + deliveryTimeoutMs)); } /** - * A {@link ProducerBatch} configured using a very large retryBackoff value with retry = true and a timestamp - * preceding its create time is interpreted correctly as not expired when the retryBackoff time is larger than the - * difference between now and create time by {@link ProducerBatch#maybeExpire(int, long, long, long, boolean)}. + * A {@link ProducerBatch} configured using a timestamp preceding its create time is interpreted correctly + * * as not expired by {@link ProducerBatch#hasReachedDeliveryTimeout(long, long)}. */ @Test - public void testLargeRetryBackoffOldNowExpire() { + public void testBatchExpirationAfterReenqueue() { ProducerBatch batch = new ProducerBatch(new TopicPartition("topic", 1), memoryRecordsBuilder, now); // Set batch.retry = true batch.reenqueued(now); // Set `now` to 2ms before the create time. - assertFalse(batch.maybeExpire(10240, Long.MAX_VALUE, now - 2L, 10240L, false)); - } - - /** - * A {@link ProducerBatch#maybeExpire(int, long, long, long, boolean)} call with a now value before the create - * time of the ProducerBatch is correctly recognized as not expired when invoked with parameter isFull = true. - */ - @Test - public void testLargeFullOldNowExpire() { - ProducerBatch batch = new ProducerBatch(new TopicPartition("topic", 1), memoryRecordsBuilder, now); - // Set `now` to 2ms before the create time. - assertFalse(batch.maybeExpire(10240, 10240L, now - 2L, 10240L, true)); + assertFalse(batch.hasReachedDeliveryTimeout(10240, now - 2L)); } @Test diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java index 5f4841032d2a3..9272f49242eba 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java @@ -84,9 +84,9 @@ public class RecordAccumulatorTest { private byte[] key = "key".getBytes(); private byte[] value = "value".getBytes(); private int msgSize = DefaultRecord.sizeInBytes(0, 0, key.length, value.length, - Record.EMPTY_HEADERS); + Record.EMPTY_HEADERS); private Cluster cluster = new Cluster(null, Arrays.asList(node1, node2), Arrays.asList(part1, part2, part3), - Collections.emptySet(), Collections.emptySet()); + Collections.emptySet(), Collections.emptySet()); private Metrics metrics = new Metrics(time); private final long maxBlockTimeMs = 1000; private final LogContext logContext = new LogContext(); @@ -104,7 +104,7 @@ public void testFull() throws Exception { int batchSize = 1025; RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10L * batchSize, CompressionType.NONE, 10L); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10L * batchSize, CompressionType.NONE, 10L); int appends = expectedNumAppends(batchSize); for (int i = 0; i < appends; i++) { // append to the first batch @@ -153,7 +153,7 @@ private void testAppendLarge(CompressionType compressionType) throws Exception { int batchSize = 512; byte[] value = new byte[2 * batchSize]; RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, compressionType, 0L); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, compressionType, 0L); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("Our partition's leader should be ready", Collections.singleton(node1), accum.ready(cluster, time.milliseconds()).readyNodes); @@ -189,10 +189,10 @@ private void testAppendLargeOldMessageFormat(CompressionType compressionType) th ApiVersions apiVersions = new ApiVersions(); apiVersions.update(node1.idString(), NodeApiVersions.create(Collections.singleton( - new ApiVersionsResponse.ApiVersion(ApiKeys.PRODUCE.id, (short) 0, (short) 2)))); + new ApiVersionsResponse.ApiVersion(ApiKeys.PRODUCE.id, (short) 0, (short) 2)))); RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, compressionType, 0L); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, compressionType, 0L); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("Our partition's leader should be ready", Collections.singleton(node1), accum.ready(cluster, time.milliseconds()).readyNodes); @@ -216,7 +216,7 @@ private void testAppendLargeOldMessageFormat(CompressionType compressionType) th public void testLinger() throws Exception { long lingerMs = 10L; RecordAccumulator accum = createTestRecordAccumulator( - 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, lingerMs); + 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, lingerMs); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("No partitions should be ready", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); time.sleep(10); @@ -235,7 +235,7 @@ public void testLinger() throws Exception { @Test public void testPartialDrain() throws Exception { RecordAccumulator accum = createTestRecordAccumulator( - 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, 10L); + 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, 10L); int appends = 1024 / msgSize + 1; List partitions = asList(tp1, tp2); for (TopicPartition tp : partitions) { @@ -255,7 +255,7 @@ public void testStressfulSituation() throws Exception { final int msgs = 10000; final int numParts = 2; final RecordAccumulator accum = createTestRecordAccumulator( - 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, 0L); + 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, 0L); List threads = new ArrayList<>(); for (int i = 0; i < numThreads; i++) { threads.add(new Thread() { @@ -300,7 +300,7 @@ public void testNextReadyCheckDelay() throws Exception { int batchSize = 1025; RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); // Just short of going over the limit so we trigger linger time int appends = expectedNumAppends(batchSize); @@ -332,10 +332,17 @@ public void testNextReadyCheckDelay() throws Exception { @Test public void testRetryBackoff() throws Exception { - long lingerMs = Long.MAX_VALUE / 4; - long retryBackoffMs = Long.MAX_VALUE / 2; - final RecordAccumulator accum = new RecordAccumulator(logContext, 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, - CompressionType.NONE, lingerMs, retryBackoffMs, metrics, time, new ApiVersions(), null); + long lingerMs = Integer.MAX_VALUE / 16; + long retryBackoffMs = Integer.MAX_VALUE / 8; + int requestTimeoutMs = Integer.MAX_VALUE / 4; + long deliveryTimeoutMs = Integer.MAX_VALUE; + long totalSize = 10 * 1024; + int batchSize = 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD; + String metricGrpName = "producer-metrics"; + + final RecordAccumulator accum = new RecordAccumulator(logContext, batchSize, + CompressionType.NONE, lingerMs, retryBackoffMs, requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, time, new ApiVersions(), null, + new BufferPool(totalSize, batchSize, metrics, time, metricGrpName)); long now = time.milliseconds(); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); @@ -373,7 +380,7 @@ public void testRetryBackoff() throws Exception { public void testFlush() throws Exception { long lingerMs = Long.MAX_VALUE; final RecordAccumulator accum = createTestRecordAccumulator( - 4 * 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); + 4 * 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); for (int i = 0; i < 100; i++) { accum.append(new TopicPartition(topic, i % 3), 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); @@ -413,7 +420,7 @@ public void run() { @Test public void testAwaitFlushComplete() throws Exception { RecordAccumulator accum = createTestRecordAccumulator( - 4 * 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, Long.MAX_VALUE); + 4 * 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, Long.MAX_VALUE); accum.append(new TopicPartition(topic, 0), 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); accum.beginFlush(); @@ -434,7 +441,7 @@ public void testAbortIncompleteBatches() throws Exception { final AtomicInteger numExceptionReceivedInCallback = new AtomicInteger(0); final RecordAccumulator accum = createTestRecordAccumulator( - 128 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); + 128 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); class TestCallback implements Callback { @Override public void onCompletion(RecordMetadata metadata, Exception exception) { @@ -473,7 +480,7 @@ public void testAbortUnsentBatches() throws Exception { final AtomicInteger numExceptionReceivedInCallback = new AtomicInteger(0); final RecordAccumulator accum = createTestRecordAccumulator( - 128 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); + 128 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); final KafkaException cause = new KafkaException(); class TestCallback implements Callback { @@ -488,7 +495,7 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { RecordAccumulator.ReadyCheckResult result = accum.ready(cluster, time.milliseconds()); assertFalse(result.readyNodes.isEmpty()); Map> drained = accum.drain(cluster, result.readyNodes, Integer.MAX_VALUE, - time.milliseconds()); + time.milliseconds()); assertTrue(accum.hasUndrained()); assertTrue(accum.hasIncomplete()); @@ -509,50 +516,133 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { assertTrue(accum.hasIncomplete()); } + private void doExpireBatchSingle(long deliveryTimeoutMs) throws InterruptedException { + long lingerMs = 3000L; + List muteStates = Arrays.asList(false, true); + Set readyNodes = null; + List expiredBatches = new ArrayList<>(); + + // test case assumes that the records do not fill the batch completely + int batchSize = 1025; + + RecordAccumulator accum = createTestRecordAccumulator(deliveryTimeoutMs, + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); + + // Make the batches ready due to linger. These batches are not in retry + for (Boolean mute: muteStates) { + accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); + assertEquals("No partition should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); + + time.sleep(lingerMs); + readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; + assertEquals("Our partition's leader should be ready", Collections.singleton(node1), readyNodes); + List batches = accum.expiredBatches(time.milliseconds()); + expiredBatches.addAll(batches); + + assertEquals("The batch should not expire when just linger has passed", 0, batches.size()); + + if (mute) + accum.mutePartition(tp1); + else + accum.unmutePartition(tp1, 0L); + + System.out.println(tp1 + " is muted ? " + mute); + System.out.println("deliveryTimeoutMs: " + deliveryTimeoutMs + ", lingerMs: " + lingerMs); + // Advance the clock to expire the batch. + time.sleep(deliveryTimeoutMs - lingerMs); + + System.out.println("time.milliseconds = " + time.milliseconds()); + batches = accum.expiredBatches(time.milliseconds()); + // < 0 ? Long.MAX_VALUE : time.milliseconds()); + expiredBatches.addAll(batches); + + if (expiredBatches.size() != 1) { + System.out.println("mute ? " + mute); + } + } + assertEquals("The batch may expire when the partition is muted", 1, expiredBatches.size()); + + assertEquals("No partitions should be ready.", 0, + accum.ready(cluster, time.milliseconds()).readyNodes.size()); + } + + //@Test + //public void testExpiredBatchSingle() throws InterruptedException { + // doExpireBatchSingle(3200L); + //} + + //@Test + //public void testExpiredBatchSingleMaxValue() throws InterruptedException { + // doExpireBatchSingle(Long.MAX_VALUE); + //} + @Test public void testExpiredBatches() throws InterruptedException { long retryBackoffMs = 100L; - long lingerMs = 3000L; + long lingerMs = 30L; int requestTimeout = 60; + long deliveryTimeoutMs = 3200L; // test case assumes that the records do not fill the batch completely - int batchSize = 1025; - + int batchPayloadSize = 1025; + int batchSize = batchPayloadSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD; RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); - int appends = expectedNumAppends(batchSize); + deliveryTimeoutMs, batchSize, 10 * batchPayloadSize, CompressionType.NONE, lingerMs); + int appends = expectedNumAppends(batchPayloadSize); + + System.out.println("Expected # of append is " + appends + " for batch size " + batchSize); // Test batches not in retry for (int i = 0; i < appends; i++) { accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); } + // Make the batches ready due to batch full accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, 0); Set readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; assertEquals("Our partition's leader should be ready", Collections.singleton(node1), readyNodes); // Advance the clock to expire the batch. - time.sleep(requestTimeout + 1); + time.sleep(deliveryTimeoutMs + 1); accum.mutePartition(tp1); - List expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + + Map> drainResult = accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + + List expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when the partition is muted", 0, expiredBatches.size()); accum.unmutePartition(tp1, 0L); - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + + //time.sleep(deliveryTimeMs - requestTimeout - lingerMs); + + + drainResult = accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + + System.out.println("=================================================="); + for (Map.Entry> entry: drainResult.entrySet()) { + System.out.println(entry.getKey() + " :-->: " + entry.getValue()); + } + + + expiredBatches = accum.expiredBatches(time.milliseconds()); + assertEquals("The batch should be expired", 1, expiredBatches.size()); - assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); + assertEquals("One partitions should be ready.", 1, accum.ready(cluster, time.milliseconds()).readyNodes.size()); // Advance the clock to make the next batch ready due to linger.ms time.sleep(lingerMs); assertEquals("Our partition's leader should be ready", Collections.singleton(node1), readyNodes); time.sleep(requestTimeout + 1); + accum.mutePartition(tp1); - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + drainResult = accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when metadata is still available and partition is muted", 0, expiredBatches.size()); accum.unmutePartition(tp1, 0L); - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + drainResult = accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should be expired when the partition is not muted", 1, expiredBatches.size()); assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); @@ -568,39 +658,45 @@ public void testExpiredBatches() throws InterruptedException { accum.reenqueue(drained.get(node1.id()).get(0), time.milliseconds()); // test expiration. - time.sleep(requestTimeout + retryBackoffMs); - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + time.sleep(deliveryTimeoutMs); + expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired.", 0, expiredBatches.size()); time.sleep(1L); accum.mutePartition(tp1); - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when the partition is muted", 0, expiredBatches.size()); accum.unmutePartition(tp1, 0L); - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should be expired when the partition is not muted.", 1, expiredBatches.size()); + // Test that when being throttled muted batches are expired before the throttle time is over. accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, 0); time.sleep(lingerMs); readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; assertEquals("Our partition's leader should be ready", Collections.singleton(node1), readyNodes); // Advance the clock to expire the batch. - time.sleep(requestTimeout + 1); + time.sleep(deliveryTimeoutMs - lingerMs); accum.mutePartition(tp1); - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when the partition is muted", 0, expiredBatches.size()); long throttleTimeMs = 100L; accum.unmutePartition(tp1, time.milliseconds() + throttleTimeMs); // The batch shouldn't be expired yet. - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when the partition is muted", 0, expiredBatches.size()); // Once the throttle time is over, the batch can be expired. time.sleep(throttleTimeMs); - expiredBatches = accum.expiredBatches(requestTimeout, time.milliseconds()); + accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should be expired", 1, expiredBatches.size()); assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); } @@ -612,7 +708,7 @@ public void testMutedPartitions() throws InterruptedException { int batchSize = 1025; RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, 10); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, 10); int appends = expectedNumAppends(batchSize); for (int i = 0; i < appends; i++) { accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); @@ -646,10 +742,18 @@ public void testIdempotenceWithOldMagic() throws InterruptedException { // Simulate talking to an older broker, ie. one which supports a lower magic. ApiVersions apiVersions = new ApiVersions(); int batchSize = 1025; + int requestTimeoutMs = 1600; + long deliveryTimeoutMs = 3200L; + long lingerMs = 10L; + long retryBackoffMs = 100L; + long totalSize = 10 * batchSize; + String metricGrpName = "producer-metrics"; + apiVersions.update("foobar", NodeApiVersions.create(Arrays.asList(new ApiVersionsResponse.ApiVersion(ApiKeys.PRODUCE.id, (short) 0, (short) 2)))); - RecordAccumulator accum = new RecordAccumulator(logContext, batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, - CompressionType.NONE, 10, 100L, metrics, time, apiVersions, new TransactionManager()); + RecordAccumulator accum = new RecordAccumulator(logContext, batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, + CompressionType.NONE, lingerMs, retryBackoffMs, requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, new TransactionManager(), + new BufferPool(totalSize, batchSize, metrics, time, metricGrpName)); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, 0); } @@ -727,9 +831,9 @@ public void testSplitBatchOffAccumulator() throws InterruptedException { assertFalse(drained.get(node1.id()).isEmpty()); } assertTrue("All the batches should have been drained.", - accum.ready(cluster, time.milliseconds()).readyNodes.isEmpty()); + accum.ready(cluster, time.milliseconds()).readyNodes.isEmpty()); assertEquals("The split batches should be allocated off the accumulator", - bufferCapacity, accum.bufferPoolAvailableMemory()); + bufferCapacity, accum.bufferPoolAvailableMemory()); } @Test @@ -749,7 +853,7 @@ public void testSplitFrequency() throws InterruptedException { for (int i = 0; i < numMessages; i++) { int dice = random.nextInt(100); byte[] value = (dice < goodCompRatioPercentage) ? - bytesWithGoodCompression(random) : bytesWithPoorCompression(random, 100); + bytesWithGoodCompression(random) : bytesWithPoorCompression(random, 100); accum.append(tp1, 0L, null, value, Record.EMPTY_HEADERS, null, 0); BatchDrainedResult result = completeOrSplitBatches(accum, batchSize); numSplit += result.numSplit; @@ -760,8 +864,82 @@ public void testSplitFrequency() throws InterruptedException { numSplit += result.numSplit; numBatches += result.numBatches; assertTrue(String.format("Total num batches = %d, split batches = %d, more than 10%% of the batch splits. " - + "Random seed is " + seed, - numBatches, numSplit), (double) numSplit / numBatches < 0.1f); + + "Random seed is " + seed, + numBatches, numSplit), (double) numSplit / numBatches < 0.1f); + } + } + + @Test + public void testSoonToExpireBatchesArePickedUpForExpiry() throws InterruptedException { + long lingerMs = 500L; + int batchSize = 1025; + + RecordAccumulator accum = createTestRecordAccumulator( + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); + + accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); + Set readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; + Map> drained = accum.drain(cluster, readyNodes, Integer.MAX_VALUE, time.milliseconds()); + assertTrue(drained.isEmpty()); + assertTrue(accum.soonToExpireInFlightBatches().isEmpty()); + + // advanced clock and send one batch out but it should not be included in soon to expire inflight + // batches because batch's expiry is quite far. + time.sleep(lingerMs + 1); + readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; + drained = accum.drain(cluster, readyNodes, Integer.MAX_VALUE, time.milliseconds()); + assertEquals("A batch did not drain after linger", 1, drained.size()); + assertTrue(accum.soonToExpireInFlightBatches().isEmpty()); + + // Queue another batch and advance clock such that batch expiry time is earlier than request timeout. + accum.append(tp2, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); + time.sleep(lingerMs * 4); + + // Now drain and check that accumulator picked up the drained batch because its expiry is soon. + readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; + drained = accum.drain(cluster, readyNodes, Integer.MAX_VALUE, time.milliseconds()); + assertEquals("A batch did not drain after linger", 1, drained.size()); + assertEquals("A soon to expire batch was ignored", 1, accum.soonToExpireInFlightBatches().size()); + } + + @Test + public void testExpiredBatchesRetry() throws InterruptedException { + long lingerMs = 3000L; + long rtt = 1000; + long deliveryTimeoutMs = 3200L; + Set readyNodes = null; + List expiredBatches = null; + List muteStates = Arrays.asList(false, true); + + // test case assumes that the records do not fill the batch completely + int batchSize = 1025; + + RecordAccumulator accum = createTestRecordAccumulator( + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); + + // Test batches in retry. + for (Boolean mute: muteStates) { + accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, 0); + time.sleep(lingerMs); + readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; + assertEquals("Our partition's leader should be ready", Collections.singleton(node1), readyNodes); + Map> drained = accum.drain(cluster, readyNodes, Integer.MAX_VALUE, time.milliseconds()); + assertEquals("There should be only one batch.", 1, drained.get(node1.id()).size()); + time.sleep(rtt); + accum.reenqueue(drained.get(node1.id()).get(0), time.milliseconds()); + + if (mute) + accum.mutePartition(tp1); + else + accum.unmutePartition(tp1, 0L); + + // test expiration + time.sleep(deliveryTimeoutMs - rtt); + accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); + expiredBatches = accum.expiredBatches(time.milliseconds()); + + System.out.println("mute = " + mute); + assertEquals("The batch in retry may expire.", mute ? 0 : 1, expiredBatches.size()); } } @@ -852,7 +1030,7 @@ private int expectedNumAppends(int batchSize) { int offsetDelta = 0; while (true) { int recordSize = DefaultRecord.sizeInBytes(offsetDelta, 0, key.length, value.length, - Record.EMPTY_HEADERS); + Record.EMPTY_HEADERS); if (size + recordSize > batchSize) return offsetDelta; offsetDelta += 1; @@ -860,20 +1038,33 @@ private int expectedNumAppends(int batchSize) { } } + + private RecordAccumulator createTestRecordAccumulator(int batchSize, long totalSize, CompressionType type, long lingerMs) { + long deliveryTimeoutMs = 3200L; + return createTestRecordAccumulator(deliveryTimeoutMs, batchSize, totalSize, type, lingerMs); + } + /** * Return a test RecordAccumulator instance */ - private RecordAccumulator createTestRecordAccumulator(int batchSize, long totalSize, CompressionType type, long lingerMs) { + private RecordAccumulator createTestRecordAccumulator(long deliveryTimeoutMs, int batchSize, long totalSize, CompressionType type, long lingerMs) { + long retryBackoffMs = 100L; + int requestTimeoutMs = 1600; + String metricGrpName = "producer-metrics"; + return new RecordAccumulator( - logContext, - batchSize, - totalSize, - type, - lingerMs, - 100L, - metrics, - time, - new ApiVersions(), - null); + logContext, + batchSize, + type, + lingerMs, + retryBackoffMs, + requestTimeoutMs, + deliveryTimeoutMs, + metrics, + metricGrpName, + time, + new ApiVersions(), + null, + new BufferPool(totalSize, batchSize, metrics, time, metricGrpName)); } } diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java index d87c8f9e894c4..1ff28b813a301 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java @@ -16,6 +16,21 @@ */ package org.apache.kafka.clients.producer.internals; +import java.nio.ByteBuffer; +import java.util.Collections; +import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; +import java.util.IdentityHashMap; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.apache.kafka.clients.ApiVersions; import org.apache.kafka.clients.ClientRequest; import org.apache.kafka.clients.Metadata; @@ -62,6 +77,7 @@ import org.apache.kafka.common.requests.ResponseHeader; import org.apache.kafka.common.utils.LogContext; import org.apache.kafka.common.utils.MockTime; +import org.apache.kafka.common.utils.Time; import org.apache.kafka.test.DelayedReceive; import org.apache.kafka.test.MockSelector; import org.apache.kafka.test.TestUtils; @@ -69,25 +85,10 @@ import org.junit.Before; import org.junit.Test; -import java.nio.ByteBuffer; -import java.util.Collections; -import java.util.Deque; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Iterator; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.Future; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; - import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public class SenderTest { @@ -429,9 +430,12 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { // Advance the clock to expire the first batch. time.sleep(10000); + + Node clusterNode = this.cluster.nodes().get(0); + accumulator.drain(cluster, Collections.singleton(clusterNode), Integer.MAX_VALUE, time.milliseconds()); + // Disconnect the target node for the pending produce request. This will ensure that sender will try to // expire the batch. - Node clusterNode = this.cluster.nodes().get(0); client.disconnect(clusterNode.idString()); client.blackout(clusterNode, 100); @@ -974,6 +978,9 @@ public void testExpiryOfFirstBatchShouldNotCauseUnresolvedSequencesIfFutureBatch // Send first ProduceRequest Future request1 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request + // We separate the two appends by 1 second so that the two batches + // don't expire at the same time. + time.sleep(1000L); Future request2 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request @@ -984,7 +991,9 @@ public void testExpiryOfFirstBatchShouldNotCauseUnresolvedSequencesIfFutureBatch sender.run(time.milliseconds()); // receive first response Node node = this.cluster.nodes().get(0); - time.sleep(10000L); + // We add 600 millis to expire the first batch but not the second. + // Note deliveryTimeoutMs is 1500. + time.sleep(600L); client.disconnect(node.idString()); client.blackout(node, 10); @@ -1032,6 +1041,9 @@ public void testExpiryOfFirstBatchShouldCauseResetIfFutureBatchesFail() throws E // Send first ProduceRequest Future request1 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request + // We separate the two appends by 1 second so that the two batches + // don't expire at the same time. + time.sleep(1000L); Future request2 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request @@ -1042,7 +1054,9 @@ public void testExpiryOfFirstBatchShouldCauseResetIfFutureBatchesFail() throws E sender.run(time.milliseconds()); // receive first response Node node = this.cluster.nodes().get(0); - time.sleep(10000L); + // We add 600 millis to expire the first batch but not the second. + // Note deliveryTimeoutMs is 1500. + time.sleep(600L); client.disconnect(node.idString()); client.blackout(node, 10); @@ -1088,11 +1102,10 @@ public void testExpiryOfAllSentBatchesShouldCauseUnresolvedSequences() throws Ex sender.run(time.milliseconds()); // send request sendIdempotentProducerResponse(0, tp0, Errors.NOT_LEADER_FOR_PARTITION, -1); sender.run(time.milliseconds()); // receive response - assertEquals(1L, transactionManager.sequenceNumber(tp0).longValue()); Node node = this.cluster.nodes().get(0); - time.sleep(10000L); + time.sleep(15000L); client.disconnect(node.idString()); client.blackout(node, 10); @@ -1794,11 +1807,16 @@ private void testSplitBatchAndSend(TransactionManager txnManager, TopicPartition tp) throws Exception { int maxRetries = 1; String topic = tp.topic(); + int requestTimeoutMs = 1500; + long deliveryTimeoutMs = 3000L; + long totalSize = 1024 * 1024; + String metricGrpName = "producer-metrics"; // Set a good compression ratio. CompressionRatioEstimator.setEstimation(topic, CompressionType.GZIP, 0.2f); try (Metrics m = new Metrics()) { - accumulator = new RecordAccumulator(logContext, batchSize, 1024 * 1024, CompressionType.GZIP, 0L, 0L, m, time, - new ApiVersions(), txnManager); + accumulator = new RecordAccumulator(logContext, batchSize, CompressionType.GZIP, + 0L, 0L, requestTimeoutMs, deliveryTimeoutMs, m, metricGrpName, time, new ApiVersions(), txnManager, + new BufferPool(totalSize, batchSize, metrics, time, "producer-internal-metrics")); SenderMetricsRegistry senderMetrics = new SenderMetricsRegistry(m); Sender sender = new Sender(logContext, client, metadata, this.accumulator, true, MAX_REQUEST_SIZE, ACKS_ALL, maxRetries, senderMetrics, time, REQUEST_TIMEOUT, 1000L, txnManager, new ApiVersions()); @@ -1871,6 +1889,149 @@ private void testSplitBatchAndSend(TransactionManager txnManager, } } + @Test + public void testNoDoubleDeallocation() throws Exception { + long deliverTimeoutMs = 1500L; + long totalSize = 1024 * 1024; + String metricGrpName = "producer-custom-metrics"; + MatchingBufferPool pool = new MatchingBufferPool(totalSize, batchSize, metrics, time, metricGrpName); + setupWithTransactionState(null, false, pool); + + // Send first ProduceRequest + Future request1 = + accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; + sender.run(time.milliseconds()); // send request + assertEquals(1, client.inFlightRequestCount()); + time.sleep(deliverTimeoutMs); + sender.run(time.milliseconds()); // expire the batch + assertTrue(request1.isDone()); + // make sure the batch got deallocated right after expiry + assertTrue(pool.allMatch()); + + assertEquals(0, client.inFlightRequestCount()); + //client.respond(produceResponse(tp0, -1, Errors.NOT_LEADER_FOR_PARTITION, -1)); // return a retriable error + + //sender.run(time.milliseconds()); // receive first response and do not reenqueue. + //assertEquals(0, client.inFlightRequestCount()); + //sender.run(time.milliseconds()); // run again and must not send anything. + //assertEquals(0, client.inFlightRequestCount()); + //assertTrue(pool.allMatch()); + } + + @Test + public void testInflightBatchesExpireOnDeliveryTimeout() throws InterruptedException { + long deliveryTimeoutMs = 1500L; + + setupWithTransactionState(null, true, null); + + // Send first ProduceRequest + Future request = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; + sender.run(time.milliseconds()); // send request + assertEquals(1, client.inFlightRequestCount()); + + Map responseMap = new HashMap<>(); + responseMap.put(tp0, new ProduceResponse.PartitionResponse(Errors.NONE, 0L, 0L, 0L)); + client.respond(new ProduceResponse(responseMap)); + + time.sleep(deliveryTimeoutMs); + sender.run(time.milliseconds()); // receive first response + try { + request.get(); + fail("The expired batch should throw a TimeoutException"); + } catch (ExecutionException e) { + assertTrue(e.getCause() instanceof TimeoutException); + } + } + + @Test + public void testWhenFirstBatchExpireNoSendSecondBatchIfGuaranteeOrder() throws InterruptedException { + long deliveryTimeoutMs = 1500L; + + setupWithTransactionState(null, true, null); + + // Send first ProduceRequest + accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT); + sender.run(time.milliseconds()); // send request + assertEquals(1, client.inFlightRequestCount()); + + time.sleep(deliveryTimeoutMs / 2); + + // Send second ProduceRequest + accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT); + sender.run(time.milliseconds()); // must not send request because the partition is muted + assertEquals(1, client.inFlightRequestCount()); + + time.sleep(deliveryTimeoutMs / 2); // expire the first batch only + sender.run(time.milliseconds()); + assertEquals(1, client.inFlightRequestCount()); + + client.respond(produceResponse(tp0, 0L, Errors.NONE, 0, 0L)); + sender.run(time.milliseconds()); // receive response (offset=0) + assertEquals(0, client.inFlightRequestCount()); + + sender.run(time.milliseconds()); // Drain the second request only this time + assertEquals(1, client.inFlightRequestCount()); + } + + @Test + public void testExpiredBatchDoesNotRetry() throws Exception { + long deliverTimeoutMs = 1500L; + setupWithTransactionState(null, false, null); + + // Send first ProduceRequest + Future request1 = + accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, + MAX_BLOCK_TIMEOUT).future; + sender.run(time.milliseconds()); // send request + assertEquals(1, client.inFlightRequestCount()); + time.sleep(deliverTimeoutMs); + + Map responseMap = new HashMap<>(); + responseMap.put(tp0, new ProduceResponse.PartitionResponse(Errors.NONE, 0L, 0L, 0L)); + client.respond(produceResponse(tp0, -1, Errors.NOT_LEADER_FOR_PARTITION, -1)); // return a retriable error + + sender.run(time.milliseconds()); // expire the batch + assertTrue(request1.isDone()); + + System.out.println("client.inFlightRequestCount() = " + client.inFlightRequestCount()); + assertEquals(0, client.inFlightRequestCount()); + + + //sender.run(time.milliseconds()); // receive first response and do not reenqueue. + //assertEquals(0, client.inFlightRequestCount()); + //sender.run(time.milliseconds()); // run again and must not send anything. + //assertEquals(0, client.inFlightRequestCount()); + } + + private class MatchingBufferPool extends BufferPool { + IdentityHashMap allocatedBuffers; + + MatchingBufferPool(long totalSize, int batchSize, Metrics metrics, Time time, String metricGrpName) { + super(totalSize, batchSize, metrics, time, metricGrpName); + allocatedBuffers = new IdentityHashMap<>(); + } + + @Override + public ByteBuffer allocate(int size, long maxTimeToBlockMs) throws InterruptedException { + ByteBuffer buffer = super.allocate(size, maxTimeToBlockMs); + allocatedBuffers.put(buffer, Boolean.TRUE); + return buffer; + } + + @Override + public void deallocate(ByteBuffer buffer, int size) { + if (!allocatedBuffers.containsKey(buffer)) { + throw new IllegalStateException("Deallocating a buffer that is not allocated"); + } + allocatedBuffers.remove(buffer); + super.deallocate(buffer, size); + } + + public boolean allMatch() { + return allocatedBuffers.isEmpty(); + } + } + private MockClient.RequestMatcher produceRequestMatcher(final TopicPartition tp, final ProducerIdAndEpoch producerIdAndEpoch, final int sequence, @@ -1931,16 +2092,29 @@ private ProduceResponse produceResponse(TopicPartition tp, long offset, Errors e } private void setupWithTransactionState(TransactionManager transactionManager) { + setupWithTransactionState(transactionManager, false, null); + } + + private void setupWithTransactionState(TransactionManager transactionManager, boolean guaranteeOrder, BufferPool customPool) { + long totalSize = 1024 * 1024; + String metricGrpName = "producer-metrics"; Map metricTags = new LinkedHashMap<>(); metricTags.put("client-id", CLIENT_ID); MetricConfig metricConfig = new MetricConfig().tags(metricTags); this.metrics = new Metrics(metricConfig, time); - this.accumulator = new RecordAccumulator(logContext, batchSize, 1024 * 1024, CompressionType.NONE, 0L, 0L, metrics, time, - apiVersions, transactionManager); - this.senderMetricsRegistry = new SenderMetricsRegistry(this.metrics); + BufferPool pool = (customPool == null) ? new BufferPool(totalSize, batchSize, metrics, time, metricGrpName) : customPool; + setupWithTransactionState(transactionManager, guaranteeOrder, metricTags, pool); + } - this.sender = new Sender(logContext, this.client, this.metadata, this.accumulator, false, MAX_REQUEST_SIZE, ACKS_ALL, - Integer.MAX_VALUE, this.senderMetricsRegistry, this.time, REQUEST_TIMEOUT, 50, transactionManager, apiVersions); + private void setupWithTransactionState(TransactionManager transactionManager, boolean guaranteeOrder, Map metricTags, BufferPool pool) { + long deliveryTimeoutMs = 1500L; + int requestTimeoutMs = 1500; + String metricGrpName = "producer-metrics"; + this.accumulator = new RecordAccumulator(logContext, batchSize, CompressionType.NONE, 0L, 0L, + requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, transactionManager, pool); + this.senderMetricsRegistry = new SenderMetricsRegistry(this.metrics); + this.sender = new Sender(logContext, this.client, this.metadata, this.accumulator, guaranteeOrder, MAX_REQUEST_SIZE, ACKS_ALL, + Integer.MAX_VALUE, this.senderMetricsRegistry, this.time, REQUEST_TIMEOUT, 50, transactionManager, apiVersions); this.metadata.update(this.cluster, Collections.emptySet(), time.milliseconds()); } diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java index 558ec72109695..108099e92f42c 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java @@ -118,6 +118,10 @@ public void setup() { Map metricTags = new LinkedHashMap<>(); metricTags.put("client-id", CLIENT_ID); int batchSize = 16 * 1024; + int requestTimeoutMs = 1500; + long deliveryTimeoutMs = 3000L; + long totalSize = 1024 * 1024; + String metricGrpName = "producer-metrics"; MetricConfig metricConfig = new MetricConfig().tags(metricTags); this.brokerNode = new Node(0, "localhost", 2211); this.transactionManager = new TransactionManager(logContext, transactionalId, transactionTimeoutMs, @@ -125,7 +129,7 @@ public void setup() { Metrics metrics = new Metrics(metricConfig, time); SenderMetricsRegistry senderMetrics = new SenderMetricsRegistry(metrics); - this.accumulator = new RecordAccumulator(logContext, batchSize, 1024 * 1024, CompressionType.NONE, 0L, 0L, metrics, time, apiVersions, transactionManager); + this.accumulator = new RecordAccumulator(logContext, batchSize, CompressionType.NONE, 0L, 0L, requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, transactionManager, new BufferPool(totalSize, batchSize, metrics, time, metricGrpName)); this.sender = new Sender(logContext, this.client, this.metadata, this.accumulator, true, MAX_REQUEST_SIZE, ACKS_ALL, MAX_RETRIES, senderMetrics, this.time, REQUEST_TIMEOUT, 50, transactionManager, apiVersions); this.metadata.update(this.cluster, Collections.emptySet(), time.milliseconds()); diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java index 7291d4f6e88f6..1f62103213c0e 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java @@ -139,6 +139,7 @@ public Worker( producerProps.put(ProducerConfig.MAX_BLOCK_MS_CONFIG, Long.toString(Long.MAX_VALUE)); producerProps.put(ProducerConfig.ACKS_CONFIG, "all"); producerProps.put(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION, "1"); + producerProps.put(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG, Integer.toString(Integer.MAX_VALUE)); // User-specified overrides producerProps.putAll(config.originalsWithPrefix("producer.")); } diff --git a/core/src/test/scala/unit/kafka/utils/TestUtils.scala b/core/src/test/scala/unit/kafka/utils/TestUtils.scala index cf60c780d3987..00e434f74afa8 100755 --- a/core/src/test/scala/unit/kafka/utils/TestUtils.scala +++ b/core/src/test/scala/unit/kafka/utils/TestUtils.scala @@ -564,6 +564,11 @@ object TestUtils extends Logging { producerProps.put(ProducerConfig.BUFFER_MEMORY_CONFIG, bufferSize.toString) producerProps.put(ProducerConfig.RETRIES_CONFIG, retries.toString) producerProps.put(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG, requestTimeoutMs.toString) + producerProps.put(ProducerConfig.LINGER_MS_CONFIG, lingerMs.toString) + + // In case of overflow set maximum possible value for deliveryTimeoutMs + val deliveryTimeoutMs = if (lingerMs + requestTimeoutMs < 0) Long.MaxValue else lingerMs + requestTimeoutMs + producerProps.put(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG, deliveryTimeoutMs.toString) /* Only use these if not already set */ val defaultProps = Map( From 89a35b8ae07a1c20814cb4afc30fcdb7b8e12ced Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 21 Jun 2018 09:56:57 -0700 Subject: [PATCH 2/4] KAFKA-5886: update kafka pull request 3849, and fix SenderTest and RecordAccumulatorTest --- .../kafka/clients/producer/KafkaProducer.java | 36 ++-- .../clients/producer/ProducerConfig.java | 19 +- .../producer/internals/ProducerBatch.java | 70 ++----- .../producer/internals/RecordAccumulator.java | 118 ++++------- .../clients/producer/internals/Sender.java | 186 +++++++++++++----- .../kafka/common/config/AbstractConfig.java | 2 +- .../apache/kafka/common/config/ConfigDef.java | 11 +- .../org/apache/kafka/clients/MockClient.java | 3 +- .../clients/consumer/KafkaConsumerTest.java | 4 +- .../internals/RecordAccumulatorTest.java | 165 ++++++---------- .../producer/internals/SenderTest.java | 121 +++++++----- .../internals/TransactionManagerTest.java | 2 +- .../kafka/api/BaseProducerSendTest.scala | 12 +- .../kafka/api/PlaintextConsumerTest.scala | 4 +- .../kafka/api/PlaintextProducerSendTest.scala | 6 +- .../api/ProducerFailureHandlingTest.scala | 6 +- .../DynamicBrokerReconfigurationTest.scala | 4 +- .../unit/kafka/server/FetchRequestTest.scala | 2 +- .../scala/unit/kafka/utils/TestUtils.scala | 6 +- docs/upgrade.html | 4 + 20 files changed, 374 insertions(+), 407 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java index 5735381b3d4ef..b40b09a588a9c 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java @@ -393,15 +393,14 @@ public KafkaProducer(Properties properties, Serializer keySerializer, Seriali int retries = configureRetries(config, transactionManager != null, log); int maxInflightRequests = configureInflightRequests(config, transactionManager != null); short acks = configureAcks(config, transactionManager != null, log); - long deliveryTimeoutMs = configureDeliveryTimeout(config); + int deliveryTimeoutMs = configureDeliveryTimeout(config, log); this.apiVersions = new ApiVersions(); this.accumulator = new RecordAccumulator(logContext, config.getInt(ProducerConfig.BATCH_SIZE_CONFIG), this.compressionType, - config.getLong(ProducerConfig.LINGER_MS_CONFIG), + config.getInt(ProducerConfig.LINGER_MS_CONFIG), retryBackoffMs, - this.requestTimeoutMs, deliveryTimeoutMs, metrics, PRODUCER_METRIC_GROUP_NAME, @@ -464,29 +463,30 @@ public KafkaProducer(Properties properties, Serializer keySerializer, Seriali } } - private static long configureDeliveryTimeout(ProducerConfig config) { - long deliveryTimeoutMs = config.getLong(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG); - long lingerMs = config.getLong(ProducerConfig.LINGER_MS_CONFIG); + private static int configureDeliveryTimeout(ProducerConfig config, Logger log) { + int deliveryTimeoutMs = config.getInt(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG); + int lingerMs = config.getInt(ProducerConfig.LINGER_MS_CONFIG); int requestTimeoutMs = config.getInt(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG); - // when the sum of lingerMs and requestTimeoutMs overflows a long we make sure deliveryTimeoutMs is at least - // equal to lingerMs. - boolean overflow = lingerMs + requestTimeoutMs < 0L; - boolean invalid = overflow ? deliveryTimeoutMs < lingerMs : deliveryTimeoutMs < lingerMs + requestTimeoutMs; - - if (invalid) { - throw new ConfigException("Must set " + ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG + " higher than " + - ProducerConfig.LINGER_MS_CONFIG + " + " + - ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG); + if (deliveryTimeoutMs < Integer.MAX_VALUE && deliveryTimeoutMs < lingerMs + requestTimeoutMs) { + if (config.originals().containsKey(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG)) { + // throw an exception if the user explicitly set an inconsistent value + throw new ConfigException(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG + + " should be equal to or larger than " + ProducerConfig.LINGER_MS_CONFIG + + " + " + ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG); + } else { + // override deliveryTimeoutMs default value to lingerMs + requestTimeoutMs for backward compatibility + deliveryTimeoutMs = lingerMs + requestTimeoutMs; + log.warn("{} should be equal to or larger than {} + {}. Setting it to {}.", + ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG, ProducerConfig.LINGER_MS_CONFIG, + ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG, deliveryTimeoutMs); + } } - return deliveryTimeoutMs; } private static TransactionManager configureTransactionState(ProducerConfig config, LogContext logContext, Logger log) { - TransactionManager transactionManager = null; - boolean userConfiguredIdempotence = false; if (config.originals().containsKey(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG)) userConfiguredIdempotence = true; diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java b/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java index d12af75e82f0c..ab55353a66746 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java @@ -99,11 +99,18 @@ public class ProducerConfig extends AbstractConfig { + "specified time waiting for more records to show up. This setting defaults to 0 (i.e. no delay). Setting " + LINGER_MS_CONFIG + "=5, " + "for example, would have the effect of reducing the number of requests sent but would add up to 5ms of latency to records sent in the absence of load."; + /** request.timeout.ms */ + public static final String REQUEST_TIMEOUT_MS_CONFIG = CommonClientConfigs.REQUEST_TIMEOUT_MS_CONFIG; + private static final String REQUEST_TIMEOUT_MS_DOC = CommonClientConfigs.REQUEST_TIMEOUT_MS_DOC + + " This should be larger than replica.lag.time.max.ms (a broker configuration)" + + " to reduce the possibility of message duplication due to unnecessary producer retries."; + /** delivery.timeout.ms */ public static final String DELIVERY_TIMEOUT_MS_CONFIG = "delivery.timeout.ms"; private static final String DELIVERY_TIMEOUT_MS_DOC = "An upper bound on the time to report success or failure after Producer.send() returns. " + "Producer may report failure to send a message earlier than this config if all the retries are exhausted or " - + "a record is added to a batch nearing expiration."; + + "a record is added to a batch nearing expiration. " + DELIVERY_TIMEOUT_MS_CONFIG + "should be equal to or " + + "greater than " + REQUEST_TIMEOUT_MS_CONFIG + " + " + LINGER_MS_CONFIG; /** client.id */ public static final String CLIENT_ID_CONFIG = CommonClientConfigs.CLIENT_ID_CONFIG; @@ -194,12 +201,6 @@ public class ProducerConfig extends AbstractConfig { public static final String PARTITIONER_CLASS_CONFIG = "partitioner.class"; private static final String PARTITIONER_CLASS_DOC = "Partitioner class that implements the org.apache.kafka.clients.producer.Partitioner interface."; - /** request.timeout.ms */ - public static final String REQUEST_TIMEOUT_MS_CONFIG = CommonClientConfigs.REQUEST_TIMEOUT_MS_CONFIG; - private static final String REQUEST_TIMEOUT_MS_DOC = CommonClientConfigs.REQUEST_TIMEOUT_MS_DOC - + " This should be larger than replica.lag.time.max.ms (a broker configuration)" - + " to reduce the possibility of message duplication due to unnecessary producer retries."; - /** interceptor.classes */ public static final String INTERCEPTOR_CLASSES_CONFIG = "interceptor.classes"; public static final String INTERCEPTOR_CLASSES_DOC = "A list of classes to use as interceptors. " @@ -239,8 +240,8 @@ public class ProducerConfig extends AbstractConfig { ACKS_DOC) .define(COMPRESSION_TYPE_CONFIG, Type.STRING, "none", Importance.HIGH, COMPRESSION_TYPE_DOC) .define(BATCH_SIZE_CONFIG, Type.INT, 16384, atLeast(0), Importance.MEDIUM, BATCH_SIZE_DOC) - .define(LINGER_MS_CONFIG, Type.LONG, 0, atLeast(0L), Importance.MEDIUM, LINGER_MS_DOC) - .define(DELIVERY_TIMEOUT_MS_CONFIG, Type.LONG, 120 * 1000, atLeast(0L), Importance.MEDIUM, DELIVERY_TIMEOUT_MS_DOC) + .define(LINGER_MS_CONFIG, Type.INT, 0, atLeast(0), Importance.MEDIUM, LINGER_MS_DOC) + .define(DELIVERY_TIMEOUT_MS_CONFIG, Type.INT, 120 * 1000, atLeast(0), Importance.MEDIUM, DELIVERY_TIMEOUT_MS_DOC) .define(CLIENT_ID_CONFIG, Type.STRING, "", Importance.MEDIUM, CommonClientConfigs.CLIENT_ID_DOC) .define(SEND_BUFFER_CONFIG, Type.INT, 128 * 1024, atLeast(-1), Importance.MEDIUM, CommonClientConfigs.SEND_BUFFER_DOC) .define(RECEIVE_BUFFER_CONFIG, Type.INT, 32 * 1024, atLeast(-1), Importance.MEDIUM, CommonClientConfigs.RECEIVE_BUFFER_DOC) diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java b/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java index 17beb6dfa0eac..6e08185611d12 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerBatch.java @@ -20,7 +20,6 @@ import org.apache.kafka.clients.producer.RecordMetadata; import org.apache.kafka.common.TopicPartition; import org.apache.kafka.common.errors.RecordBatchTooLargeException; -import org.apache.kafka.common.errors.TimeoutException; import org.apache.kafka.common.header.Header; import org.apache.kafka.common.record.AbstractRecords; import org.apache.kafka.common.record.CompressionRatioEstimator; @@ -58,7 +57,7 @@ public final class ProducerBatch { private enum FinalState { ABORTED, FAILED, SUCCEEDED } - public final long createdMs; + final long createdMs; final TopicPartition topicPartition; final ProduceRequestResult produceFuture; @@ -77,13 +76,13 @@ private enum FinalState { ABORTED, FAILED, SUCCEEDED } private boolean retry; private boolean reopened = false; - public ProducerBatch(TopicPartition tp, MemoryRecordsBuilder recordsBuilder, long creationTime) { - this(tp, recordsBuilder, creationTime, false); + public ProducerBatch(TopicPartition tp, MemoryRecordsBuilder recordsBuilder, long createdMs) { + this(tp, recordsBuilder, createdMs, false); } - public ProducerBatch(TopicPartition tp, MemoryRecordsBuilder recordsBuilder, long creationTime, boolean isSplitBatch) { - this.createdMs = creationTime; - this.lastAttemptMs = creationTime; + public ProducerBatch(TopicPartition tp, MemoryRecordsBuilder recordsBuilder, long createdMs, boolean isSplitBatch) { + this.createdMs = createdMs; + this.lastAttemptMs = createdMs; this.recordsBuilder = recordsBuilder; this.topicPartition = tp; this.lastAppendTime = createdMs; @@ -181,7 +180,7 @@ public boolean done(long baseOffset, long logAppendTime, RuntimeException except if (tryFinalState == FinalState.SUCCEEDED) { log.trace("Successfully produced messages to {} with base offset {}.", topicPartition, baseOffset); } else { - log.trace("Failed to produce messages to {}.", topicPartition, exception); + log.trace("Failed to produce messages to {} with base offset {}.", topicPartition, baseOffset, exception); } if (this.finalState.compareAndSet(null, tryFinalState)) { @@ -192,15 +191,17 @@ public boolean done(long baseOffset, long logAppendTime, RuntimeException except if (this.finalState.get() != FinalState.SUCCEEDED) { if (tryFinalState == FinalState.SUCCEEDED) { // Log if a previously unsuccessful batch succeeded later on. - log.debug("ProduceResponse returned {} for {} after batch had already been {}.", tryFinalState, - topicPartition, this.finalState.get()); + log.debug("ProduceResponse returned {} for {} after batch with base offset {} had already been {}.", + tryFinalState, topicPartition, baseOffset, this.finalState.get()); + } else { + // FAILED --> FAILED and ABORTED --> FAILED transitions are ignored. + log.debug("Ignored state transition {} -> {} for {} batch with base offset {}", + this.finalState.get(), tryFinalState, topicPartition, baseOffset); } - // FAILED --> ABORTED and ABORTED --> FAILED transitions are ignored. } else { // A SUCCESSFUL batch must not attempt another state change. throw new IllegalStateException("A " + this.finalState.get() + " batch must not attempt another state change to " + tryFinalState); } - return false; } @@ -315,47 +316,14 @@ public String toString() { return "ProducerBatch(topicPartition=" + topicPartition + ", recordCount=" + recordCount + ")"; } - /** - * Expire the batch if current time has passed the delivery timeout of the batch. - * The delivery timeout is measured from batch creation time instead of batch close time - * because in some cases batch close may be arbitrarily delayed. - * This methods closes this batch and sets {@code expiryErrorMessage} if the batch has timed out. - */ - //boolean maybeExpire(long deliveryTimeoutMs, long now) { - // if (deliveryTimeoutMs <= (now - this.createdMs)) { - // expire(now); - // return true; - // } - // return false; - //} - boolean hasReachedDeliveryTimeout(long deliveryTimeoutMs, long now) { - return deliveryTimeoutMs <= now - this.createdMs; - } - - /** - * Expire the batch. This methods closes this batch and sets {@code expiryErrorMessage}. - */ - void expire(long now) { - expiryErrorMessage = (now - this.createdMs) + " ms has passed since batch creation"; - abortRecordAppends(); + return deliveryTimeoutMs <= now - this.createdMs; } public FinalState finalState() { return this.finalState.get(); } - /** - * If {@link #hasReachedDeliveryTimeout(long, long)} returned true, the sender will fail the batch with - * the exception returned by this method. - * @return An exception indicating the batch expired. - */ - TimeoutException timeoutException() { - if (expiryErrorMessage == null) - throw new IllegalStateException("Batch has not expired"); - return new TimeoutException("Expiring " + recordCount + " record(s) for " + topicPartition + ": " + expiryErrorMessage); - } - int attempts() { return attempts.get(); } @@ -371,10 +339,6 @@ long queueTimeMs() { return drainedMs - createdMs; } - long createdTimeMs(long nowMs) { - return Math.max(0, nowMs - createdMs); - } - long waitedTimeMs(long nowMs) { return Math.max(0, nowMs - lastAttemptMs); } @@ -491,10 +455,4 @@ public boolean isTransactional() { public boolean sequenceHasBeenReset() { return reopened; } - - boolean soonToExpire(long deliveryTimeoutMs, long now, int requestTimeoutMs) { - // We don't add anything in deliveryTimeoutMs because it may overflow - // when deliveryTimeoutMs==Long.MAX_VALUE - return deliveryTimeoutMs <= now + requestTimeoutMs - createdMs; - } } diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java b/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java index 1f459a9b23184..44078e1a2be07 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java @@ -23,12 +23,9 @@ import java.util.Deque; import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; import org.apache.kafka.clients.ApiVersions; @@ -77,7 +74,6 @@ public final class RecordAccumulator { private final CompressionType compression; private final long lingerMs; private final long retryBackoffMs; - private final int requestTimeoutMs; private final long deliveryTimeoutMs; private final BufferPool free; private final Time time; @@ -88,10 +84,7 @@ public final class RecordAccumulator { private final Map muted; private int drainIndex; private final TransactionManager transactionManager; - - // A per-partition queue of batches ordered by creation time for quick access of the oldest batch - private final ConcurrentMap> soonToExpireInFlightBatches; - private long nextBatchExpiryTimeMs; // the earliest time (absolute) a batch will expire. + private long nextBatchExpiryTimeMs = Long.MAX_VALUE; // the earliest time (absolute) a batch will expire. /** * Create a new record accumulator @@ -115,7 +108,6 @@ public RecordAccumulator(LogContext logContext, CompressionType compression, long lingerMs, long retryBackoffMs, - int requestTimeoutMs, long deliveryTimeoutMs, Metrics metrics, String metricGrpName, @@ -132,9 +124,7 @@ public RecordAccumulator(LogContext logContext, this.compression = compression; this.lingerMs = lingerMs; this.retryBackoffMs = retryBackoffMs; - this.requestTimeoutMs = requestTimeoutMs; this.deliveryTimeoutMs = deliveryTimeoutMs; - this.nextBatchExpiryTimeMs = Long.MAX_VALUE; this.batches = new CopyOnWriteMap<>(); this.free = bufferPool; this.incomplete = new IncompleteBatches(); @@ -142,7 +132,6 @@ public RecordAccumulator(LogContext logContext, this.time = time; this.apiVersions = apiVersions; this.transactionManager = transactionManager; - this.soonToExpireInFlightBatches = new ConcurrentHashMap<>(); registerMetrics(metrics, metricGrpName); } @@ -239,7 +228,6 @@ public RecordAppendResult append(TopicPartition tp, // Don't deallocate this buffer in the finally block as it's being used in the record batch buffer = null; - return new RecordAppendResult(future, dq.size() > 1 || batch.isFull(), true); } } finally { @@ -266,7 +254,7 @@ private MemoryRecordsBuilder recordsBuilder(ByteBuffer buffer, byte maxUsableMag * if it is expired, or when the producer is closed. */ private RecordAppendResult tryAppend(long timestamp, byte[] key, byte[] value, Header[] headers, - Callback callback, Deque deque) { + Callback callback, Deque deque) { ProducerBatch last = deque.peekLast(); if (last != null) { FutureRecordMetadata future = last.tryAppend(timestamp, key, value, headers, callback, time.milliseconds()); @@ -285,14 +273,19 @@ private boolean isMuted(TopicPartition tp, long now) { return result; } - private void maybeUpdateNextBatchExpiryTime(ProducerBatch batch) { - // We determine the earliest time any batch may expire. We use - // this value later on to determine the max time we can sleep in poll. - // We assume that the batch at the front of the deque will always be the next to expire. - // This may not be true if max.in.flight.requests.per.connection > 1 and retries happen. - // Watch for overflow in createdMs + deliveryTimeoutMs when deliveryTimeoutMs is Long.MAX_VALUE - nextBatchExpiryTimeMs = (batch.createdMs + deliveryTimeoutMs < 0) ? nextBatchExpiryTimeMs - : Math.min(nextBatchExpiryTimeMs, batch.createdMs + deliveryTimeoutMs); + public void maybeUpdateNextBatchExpiryTime(ProducerBatch batch) { + if (batch.createdMs + deliveryTimeoutMs > 0) { + // the non-negative check is to guard us against potential overflow due to setting + // a large value for deliveryTimeoutMs + nextBatchExpiryTimeMs = Math.min(nextBatchExpiryTimeMs, batch.createdMs + deliveryTimeoutMs); + } else { + log.warn("Skipping next batch expiry time update due to addition overflow: " + + "batch.createMs={}, deliveryTimeoutMs={}", batch.createdMs, deliveryTimeoutMs); + } + } + + public Set getTopicPartitions() { + return this.batches.keySet(); } /** @@ -301,20 +294,15 @@ private void maybeUpdateNextBatchExpiryTime(ProducerBatch batch) { public List expiredBatches(long now) { List expiredBatches = new ArrayList<>(); for (Map.Entry> entry : this.batches.entrySet()) { - // Expire inflight batches in the order of draining. Expire if the final state of the - // batch is not known by (batch's creation time + deliveryTimeoutMs). - List mayExpireBatches = soonToExpireInFlightBatches.get(entry.getKey()); - if (mayExpireBatches != null) { - Iterator iter = mayExpireBatches.iterator(); - while (iter.hasNext()) { - ProducerBatch batch = iter.next(); - + // expire the batches in the order of sending + Deque deque = entry.getValue(); + synchronized (deque) { + while (!deque.isEmpty()) { + ProducerBatch batch = deque.getFirst(); if (batch.hasReachedDeliveryTimeout(deliveryTimeoutMs, now)) { - iter.remove(); - if (batch.finalState() == null) { - batch.expire(now); - expiredBatches.add(batch); - } + deque.poll(); + batch.abortRecordAppends(); + expiredBatches.add(batch); } else { maybeUpdateNextBatchExpiryTime(batch); break; @@ -325,13 +313,16 @@ public List expiredBatches(long now) { return expiredBatches; } + public long getDeliveryTimeoutMs() { + return deliveryTimeoutMs; + } + /** - * Re-enqueue the given record batch in the accumulator to retry. - * Remove the batch from the list of inflight batches. + * Re-enqueue the given record batch in the accumulator. In Sender.completeBatch method, we check + * whether the batch has reached deliveryTimeoutMs or not. Hence we do not do the delivery timeout check here. */ public void reenqueue(ProducerBatch batch, long now) { batch.reenqueued(now); - maybeRemoveFromSoonToExpire(batch); Deque deque = getOrCreateDeque(batch.topicPartition); synchronized (deque) { if (transactionManager != null) @@ -341,13 +332,6 @@ public void reenqueue(ProducerBatch batch, long now) { } } - private void maybeRemoveFromSoonToExpire(ProducerBatch batch) { - List soonToExpireBatches = soonToExpireInFlightBatches.get(batch.topicPartition); - if (soonToExpireBatches != null) { - soonToExpireBatches.remove(batch); - } - } - /** * Split the big batch that has been rejected and reenqueue the split batches in to the accumulator. * @return the number of split batches. @@ -357,7 +341,7 @@ public int splitAndReenqueue(ProducerBatch bigBatch) { // is bigger. There are several different ways to do the reset. We chose the most conservative one to ensure // the split doesn't happen too often. CompressionRatioEstimator.setEstimation(bigBatch.topicPartition.topic(), compression, - Math.max(1.0f, (float) bigBatch.compressionRatio())); + Math.max(1.0f, (float) bigBatch.compressionRatio())); Deque dq = bigBatch.split(this.batchSize); int numSplitBatches = dq.size(); Deque partitionDequeue = getOrCreateDeque(bigBatch.topicPartition); @@ -489,7 +473,6 @@ public ReadyCheckResult ready(Cluster cluster, long nowMs) { } } } - return new ReadyCheckResult(readyNodes, nextReadyCheckDelayMs, unknownLeaderTopics); } @@ -507,8 +490,7 @@ public boolean hasUndrained() { return false; } - - private boolean shouldStop(ProducerBatch first, TopicPartition tp) { + private boolean shouldStopDrainBatchesForPartition(ProducerBatch first, TopicPartition tp) { ProducerIdAndEpoch producerIdAndEpoch = null; if (transactionManager != null) { if (!transactionManager.isSendToPartitionAllowed(tp)) @@ -573,7 +555,7 @@ private List drainBatchesForOneNode(Cluster cluster, Node node, i // compression; in this case we will still eventually send this batch in a single request break; } else { - if (shouldStop(first, tp)) + if (shouldStopDrainBatchesForPartition(first, tp)) break; boolean isTransactional = transactionManager != null ? transactionManager.isTransactional() : false; @@ -597,22 +579,10 @@ private List drainBatchesForOneNode(Cluster cluster, Node node, i transactionManager.addInFlightBatch(batch); } - batch.close(); size += batch.records().sizeInBytes(); ready.add(batch); - // Put this batch in the list of soon-to-expire-inflight-batches because we might have to expire - // it if we don't know its final state by deliveryTimeoutMs. If the expiry is farther than - // requestTimeoutMs, we don't have to keep track of this batch because it will either succeed or - // fail (due to request timeout) much sooner. - if (batch.soonToExpire(deliveryTimeoutMs, now, requestTimeoutMs)) { - List inflightBatchList = soonToExpireInFlightBatches.get(batch.topicPartition); - if (inflightBatchList == null) { - inflightBatchList = new LinkedList<>(); - soonToExpireInFlightBatches.put(tp, inflightBatchList); - } - inflightBatchList.add(batch); - } + batch.drained(now); } } @@ -620,7 +590,6 @@ private List drainBatchesForOneNode(Cluster cluster, Node node, i return ready; } - /** * Drain all the data for the given nodes and collate them into a list of batches that will fit within the specified * size on a per-node basis. This method attempts to avoid choosing the same topic-node over and over. @@ -640,8 +609,6 @@ public Map> drain(Cluster cluster, Set nodes, List ready = drainBatchesForOneNode(cluster, node, maxSize, now); batches.put(node.id(), ready); } - - maybeUpdateNextExpiryTime(now); return batches; } @@ -652,22 +619,6 @@ public Long nextExpiryTimeMs() { return this.nextBatchExpiryTimeMs; } - /** - * If there are batches to soon to expire, but now is past the last known earliest expiry time, - * we reset the expiry time to now, because it's the smallest possible valid absolute time in future. - * This could happen when there were no ready nodes for a while and clock went past the last known earliest - * expiry time. If there're no batches to soon expire, we simply reset it to the largest possible value. - * This ensures that next delivery timeout keeps moving ahead. - */ - void maybeUpdateNextExpiryTime(long now) { - if (now >= nextBatchExpiryTimeMs) - nextBatchExpiryTimeMs = soonToExpireInFlightBatches.isEmpty() ? Long.MAX_VALUE : now; - } - - ConcurrentMap> soonToExpireInFlightBatches() { - return soonToExpireInFlightBatches; - } - private Deque getDeque(TopicPartition tp) { return batches.get(tp); } @@ -858,5 +809,4 @@ public ReadyCheckResult(Set readyNodes, long nextReadyCheckDelayMs, Set> inFlightBatches; + public Sender(LogContext logContext, KafkaClient client, Metadata metadata, @@ -149,6 +154,73 @@ public Sender(LogContext logContext, this.retryBackoffMs = retryBackoffMs; this.apiVersions = apiVersions; this.transactionManager = transactionManager; + this.inFlightBatches = new HashMap<>(); + } + + public List inFlightBatches(TopicPartition tp) { + return inFlightBatches.containsKey(tp) ? inFlightBatches.get(tp) : new ArrayList<>(); + } + + public void maybeRemoveFromInflightBatches(ProducerBatch batch) { + List batches = inFlightBatches.get(batch.topicPartition); + if (batches != null) { + batches.remove(batch); + if (batches.isEmpty()) { + inFlightBatches.remove(batch.topicPartition); + } + } + } + + /** + * Get the in-flight batches that has reached delivery timeout. + */ + private List getExpiredInflightBatches(long now) { + List expiredBatches = new ArrayList<>(); + for (Map.Entry> entry : inFlightBatches.entrySet()) { + TopicPartition topicPartition = entry.getKey(); + List partitionInFlightBatches = entry.getValue(); + if (partitionInFlightBatches != null) { + Iterator iter = partitionInFlightBatches.iterator(); + while (iter.hasNext()) { + ProducerBatch batch = iter.next(); + if (batch.hasReachedDeliveryTimeout(accumulator.getDeliveryTimeoutMs(), now)) { + iter.remove(); + // expireBatches is called in Sender.sendProducerData, before client.poll. + // The batch.finalState() == null invariant should always hold. An IllegalStateException + // exception will be thrown if the invariant is violated. + if (batch.finalState() == null) { + expiredBatches.add(batch); + } else { + throw new IllegalStateException(batch.topicPartition + " batch created at " + + batch.createdMs + " gets unexpected final state " + batch.finalState()); + } + } else { + accumulator.maybeUpdateNextBatchExpiryTime(batch); + break; + } + } + if (partitionInFlightBatches.isEmpty()) + inFlightBatches.remove(topicPartition); + } + } + return expiredBatches; + } + + private void addToInflightBatches(List batches) { + for (ProducerBatch batch : batches) { + List inflightBatchList = inFlightBatches.get(batch.topicPartition); + if (inflightBatchList == null) { + inflightBatchList = new ArrayList<>(); + inFlightBatches.put(batch.topicPartition, inflightBatchList); + } + inflightBatchList.add(batch); + } + } + + public void addToInflightBatches(Map> batches) { + for (List batchList : batches.values()) { + addToInflightBatches(batchList); + } } /** @@ -204,12 +276,12 @@ void run(long now) { if (transactionManager.shouldResetProducerStateAfterResolvingSequences()) // Check if the previous run expired batches which requires a reset of the producer state. transactionManager.resetProducerId(); - if (!transactionManager.isTransactional()) { // this is an idempotent producer, so make sure we have a producer id maybeWaitForProducerId(); } else if (transactionManager.hasUnresolvedSequences() && !transactionManager.hasFatalError()) { - transactionManager.transitionToFatalError(new KafkaException("The client hasn't received acknowledgment for " + + transactionManager.transitionToFatalError( + new KafkaException("The client hasn't received acknowledgment for " + "some previously sent messages and can no longer retry them. It isn't safe to continue.")); } else if (transactionManager.hasInFlightTransactionalRequest() || maybeSendTransactionalRequest(now)) { // as long as there are outstanding transactional requests, we simply wait for them to return @@ -241,7 +313,6 @@ void run(long now) { private long sendProducerData(long now) { Cluster cluster = metadata.fetch(); - // get the list of partitions with data ready to send RecordAccumulator.ReadyCheckResult result = this.accumulator.ready(cluster, now); @@ -253,8 +324,8 @@ private long sendProducerData(long now) { for (String topic : result.unknownLeaderTopics) this.metadata.add(topic); - log.debug("Requesting metadata update due to unknown leader topics from the batched records: {}", result.unknownLeaderTopics); - + log.debug("Requesting metadata update due to unknown leader topics from the batched records: {}", + result.unknownLeaderTopics); this.metadata.requestUpdate(); } @@ -270,8 +341,8 @@ private long sendProducerData(long now) { } // create produce requests - Map> batches = this.accumulator.drain(cluster, result.readyNodes, - this.maxRequestSize, now); + Map> batches = this.accumulator.drain(cluster, result.readyNodes, this.maxRequestSize, now); + addToInflightBatches(batches); if (guaranteeMessageOrder) { // Mute all the partitions drained for (List batchList : batches.values()) { @@ -280,30 +351,34 @@ private long sendProducerData(long now) { } } + List expiredInflightBatches = getExpiredInflightBatches(now); List expiredBatches = this.accumulator.expiredBatches(now); + expiredBatches.addAll(expiredInflightBatches); + // Reset the producer id if an expired batch has previously been sent to the broker. Also update the metrics // for expired batches. see the documentation of @TransactionState.resetProducerId to understand why // we need to reset the producer id here. if (!expiredBatches.isEmpty()) log.trace("Expired {} batches in accumulator", expiredBatches.size()); for (ProducerBatch expiredBatch : expiredBatches) { - failBatch(expiredBatch, -1, NO_TIMESTAMP, expiredBatch.timeoutException(), false); + String errorMessage = "Expiring " + expiredBatch.recordCount + " record(s) for " + expiredBatch.topicPartition + + ":" + (now - expiredBatch.createdMs) + " ms has passed since batch creation"; + failBatch(expiredBatch, -1, NO_TIMESTAMP, new TimeoutException(errorMessage), false); if (transactionManager != null && expiredBatch.inRetry()) { // This ensures that no new batches are drained until the current in flight batches are fully resolved. transactionManager.markSequenceUnresolved(expiredBatch.topicPartition); } } - sensors.updateProduceRequestMetrics(batches); // If we have any nodes that are ready to send + have sendable data, poll with 0 timeout so this can immediately - // loop and try sending more data. Otherwise, the timeout is determined by nodes that have partitions with data - // that isn't yet sendable (e.g. lingering, backing off). Note that this specifically does not include nodes - // with sendable data that aren't ready to send since they would cause busy looping. + // loop and try sending more data. Otherwise, the timeout will be the smaller value between next batch expiry + // time, and the delay time for checking data availability. Note that the nodes may have data that isn't yet + // sendable due to lingering, backing off, etc. This specifically does not include nodes with sendable data + // that aren't ready to send since they would cause busy looping. long pollTimeout = Math.min(result.nextReadyCheckDelayMs, notReadyTimeout); pollTimeout = Math.min(pollTimeout, this.accumulator.nextExpiryTimeMs() - now); pollTimeout = Math.max(pollTimeout, 0); - if (!result.readyNodes.isEmpty()) { log.trace("Nodes with data ready to send: {}", result.readyNodes); // if some partitions are already ready to be sent, the select time would be 0; @@ -313,7 +388,6 @@ private long sendProducerData(long now) { pollTimeout = 0; } sendProduceRequests(batches, now); - return pollTimeout; } @@ -321,7 +395,6 @@ private boolean maybeSendTransactionalRequest(long now) { if (transactionManager.isCompleting() && accumulator.hasIncomplete()) { if (transactionManager.isAborting()) accumulator.abortUndrainedBatches(new KafkaException("Failing batch since transaction was aborted")); - // There may still be requests left which are being retried. Since we do not know whether they had // been successfully appended to the broker log, we must resend them until their final status is clear. // If they had been appended and we did not receive the error, then our sequence number would no longer @@ -344,7 +417,6 @@ private boolean maybeSendTransactionalRequest(long now) { transactionManager.lookupCoordinator(nextRequestHandler); break; } - if (!NetworkClientUtils.awaitReady(client, targetNode, time, requestTimeoutMs)) { transactionManager.lookupCoordinator(nextRequestHandler); break; @@ -356,12 +428,10 @@ private boolean maybeSendTransactionalRequest(long now) { if (targetNode != null) { if (nextRequestHandler.isRetry()) time.sleep(nextRequestHandler.retryBackoffMs()); - - ClientRequest clientRequest = client.newClientRequest(targetNode.idString(), - requestBuilder, now, true, requestTimeoutMs, nextRequestHandler); + ClientRequest clientRequest = client.newClientRequest( + targetNode.idString(), requestBuilder, now, true, requestTimeoutMs, nextRequestHandler); transactionManager.setInFlightTransactionalRequestCorrelationId(clientRequest.correlationId()); log.debug("Sending transactional request {} to node {}", requestBuilder, targetNode); - client.send(clientRequest, now); return true; } @@ -374,11 +444,9 @@ private boolean maybeSendTransactionalRequest(long now) { break; } } - time.sleep(retryBackoffMs); metadata.requestUpdate(); } - transactionManager.retry(nextRequestHandler); return true; } @@ -445,8 +513,7 @@ private void maybeWaitForProducerId() { break; } } else { - log.debug("Could not find an available broker to send InitProducerIdRequest to. " + - "We will back off and try again."); + log.debug("Could not find an available broker to send InitProducerIdRequest to. Will back off and retry."); } } catch (UnsupportedVersionException e) { transactionManager.transitionToFatalError(e); @@ -469,7 +536,7 @@ private void handleProduceResponse(ClientResponse response, Map= RecordBatch.MAGIC_VALUE_V2 || batch.isCompressed())) { // If the batch is too large, we split the batch and send the split batches again. We do not decrement // the retry attempts in this case. - log.warn("Got error produce response in correlation id {} on topic-partition {}, splitting and retrying ({} attempts left). Error: {}", - correlationId, - batch.topicPartition, - this.retries - batch.attempts(), - error); + log.warn( + "Got error produce response in correlation id {} on topic-partition {}, splitting and retrying ({} attempts left). Error: {}", + correlationId, + batch.topicPartition, + this.retries - batch.attempts(), + error); if (transactionManager != null) transactionManager.removeInFlightBatch(batch); this.accumulator.splitAndReenqueue(batch); this.accumulator.deallocate(batch); this.sensors.recordBatchSplit(); } else if (error != Errors.NONE) { - if (canRetry(batch, response)) { - log.warn("Got error produce response with correlation id {} on topic-partition {}, retrying ({} attempts left). Error: {}", - correlationId, - batch.topicPartition, - this.retries - batch.attempts() - 1, - error); + if (canRetry(batch, response, now)) { + log.warn( + "Got error produce response with correlation id {} on topic-partition {}, retrying ({} attempts left). Error: {}", + correlationId, + batch.topicPartition, + this.retries - batch.attempts() - 1, + error); if (transactionManager == null) { reenqueueBatch(batch, now); } else if (transactionManager.hasProducerIdAndEpoch(batch.producerId(), batch.producerEpoch())) { @@ -567,14 +636,14 @@ else if (error == Errors.CLUSTER_AUTHORIZATION_FAILED) if (error.exception() instanceof InvalidMetadataException) { if (error.exception() instanceof UnknownTopicOrPartitionException) { log.warn("Received unknown topic or partition error in produce request on partition {}. The " + - "topic/partition may not exist or the user may not have Describe access to it", batch.topicPartition); + "topic-partition may not exist or the user may not have Describe access to it", + batch.topicPartition); } else { log.warn("Received invalid metadata error in produce request on partition {} due to {}. Going " + "to request metadata update now", batch.topicPartition, error.exception().toString()); } metadata.requestUpdate(); } - } else { completeBatch(batch, response); } @@ -586,35 +655,43 @@ else if (error == Errors.CLUSTER_AUTHORIZATION_FAILED) private void reenqueueBatch(ProducerBatch batch, long currentTimeMs) { this.accumulator.reenqueue(batch, currentTimeMs); + maybeRemoveFromInflightBatches(batch); this.sensors.recordRetries(batch.topicPartition.topic(), batch.recordCount); } private void completeBatch(ProducerBatch batch, ProduceResponse.PartitionResponse response) { if (transactionManager != null) { if (transactionManager.hasProducerIdAndEpoch(batch.producerId(), batch.producerEpoch())) { - transactionManager.maybeUpdateLastAckedSequence(batch.topicPartition, batch.baseSequence() + batch.recordCount - 1); - log.debug("ProducerId: {}; Set last ack'd sequence number for topic-partition {} to {}", batch.producerId(), batch.topicPartition, - transactionManager.lastAckedSequence(batch.topicPartition)); + transactionManager + .maybeUpdateLastAckedSequence(batch.topicPartition, batch.baseSequence() + batch.recordCount - 1); + log.debug("ProducerId: {}; Set last ack'd sequence number for topic-partition {} to {}", + batch.producerId(), + batch.topicPartition, + transactionManager.lastAckedSequence(batch.topicPartition)); } transactionManager.updateLastAckedOffset(response, batch); transactionManager.removeInFlightBatch(batch); } - if (batch.done(response.baseOffset, response.logAppendTime, null)) + if (batch.done(response.baseOffset, response.logAppendTime, null)) { + maybeRemoveFromInflightBatches(batch); this.accumulator.deallocate(batch); + } } - private void failBatch(ProducerBatch batch, ProduceResponse.PartitionResponse response, RuntimeException exception, boolean adjustSequenceNumbers) { + private void failBatch(ProducerBatch batch, ProduceResponse.PartitionResponse response, RuntimeException exception, + boolean adjustSequenceNumbers) { failBatch(batch, response.baseOffset, response.logAppendTime, exception, adjustSequenceNumbers); } - private void failBatch(ProducerBatch batch, long baseOffset, long logAppendTime, RuntimeException exception, boolean adjustSequenceNumbers) { + private void failBatch(ProducerBatch batch, long baseOffset, long logAppendTime, RuntimeException exception, + boolean adjustSequenceNumbers) { if (transactionManager != null) { if (exception instanceof OutOfOrderSequenceException && !transactionManager.isTransactional() && transactionManager.hasProducerId(batch.producerId())) { log.error("The broker returned {} for topic-partition " + - "{} at offset {}. This indicates data loss on the broker, and should be investigated.", + "{} at offset {}. This indicates data loss on the broker, and should be investigated.", exception, batch.topicPartition, baseOffset); // Reset the transaction state since we have hit an irrecoverable exception and cannot make any guarantees @@ -636,20 +713,23 @@ private void failBatch(ProducerBatch batch, long baseOffset, long logAppendTime, this.sensors.recordErrors(batch.topicPartition.topic(), batch.recordCount); - if (batch.done(baseOffset, logAppendTime, exception)) + if (batch.done(baseOffset, logAppendTime, exception)) { + maybeRemoveFromInflightBatches(batch); this.accumulator.deallocate(batch); + } } /** * We can retry a send if the error is transient and the number of attempts taken is fewer than the maximum allowed. - * We can also retry OutOfOrderSequence exceptions for future batches, since if the first batch has failed, the future - * batches are certain to fail with an OutOfOrderSequence exception. + * We can also retry OutOfOrderSequence exceptions for future batches, since if the first batch has failed, the + * future batches are certain to fail with an OutOfOrderSequence exception. */ - private boolean canRetry(ProducerBatch batch, ProduceResponse.PartitionResponse response) { - return batch.attempts() < this.retries && - batch.finalState() == null && - ((response.error.exception() instanceof RetriableException) || - (transactionManager != null && transactionManager.canRetry(response, batch))); + private boolean canRetry(ProducerBatch batch, ProduceResponse.PartitionResponse response, long now) { + return !batch.hasReachedDeliveryTimeout(accumulator.getDeliveryTimeoutMs(), now) && + batch.attempts() < this.retries && + batch.finalState() == null && + ((response.error.exception() instanceof RetriableException) || + (transactionManager != null && transactionManager.canRetry(response, batch))); } /** diff --git a/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java b/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java index 1713d782078a8..4f8420b3acec5 100644 --- a/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java +++ b/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java @@ -65,7 +65,7 @@ public AbstractConfig(ConfigDef definition, Map originals, boolean doLog) this.values.put(update.getKey(), update.getValue()); } definition.parse(this.values); - this.used = Collections.synchronizedSet(new HashSet()); + this.used = Collections.synchronizedSet(new HashSet<>()); this.definition = definition; if (doLog) logAll(); diff --git a/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java b/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java index 7ad4c31fb5057..12e467cee4123 100644 --- a/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java +++ b/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java @@ -595,10 +595,8 @@ private void validate(String name, Map parsed, Map recommendedValues = key.recommender.validValues(name, parsed); @@ -845,9 +843,14 @@ public static class Range implements Validator { private final Number min; private final Number max; - private Range(Number min, Number maxInclusive) { + /** + * A numeric range with inclusive upper bound and inclusive lower bound + * @param min the lower bound + * @param max the upper bound + */ + private Range(Number min, Number max) { this.min = min; - this.max = maxInclusive; + this.max = max; } /** diff --git a/clients/src/test/java/org/apache/kafka/clients/MockClient.java b/clients/src/test/java/org/apache/kafka/clients/MockClient.java index 0a88f6963c062..a586af82245b6 100644 --- a/clients/src/test/java/org/apache/kafka/clients/MockClient.java +++ b/clients/src/test/java/org/apache/kafka/clients/MockClient.java @@ -351,9 +351,8 @@ public void respondToRequest(ClientRequest clientRequest, AbstractResponse respo public void respond(AbstractResponse response, boolean disconnected) { ClientRequest request = null; - if (requests.size() > 0) { + if (requests.size() > 0) request = requests.remove(); - } short version = request.requestBuilder().latestAllowedVersion(); responses.add(new ClientResponse(request.makeHeader(version), request.callback(), request.destination(), request.createdTimeMs(), time.milliseconds(), disconnected, null, null, response)); diff --git a/clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java b/clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java index c83fe06ebd020..634a1ab77cf19 100644 --- a/clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java @@ -1679,7 +1679,7 @@ private OffsetFetchResponse offsetResponse(Map offsets, Er } private ListOffsetResponse listOffsetsResponse(Map offsets) { - return listOffsetsResponse(offsets, Collections.emptyMap()); + return listOffsetsResponse(offsets, Collections.emptyMap()); } private ListOffsetResponse listOffsetsResponse(Map partitionOffsets, @@ -1818,7 +1818,7 @@ private KafkaConsumer newConsumer(Time time, requestTimeoutMs, IsolationLevel.READ_UNCOMMITTED); - return new KafkaConsumer( + return new KafkaConsumer<>( loggerFactory, clientId, consumerCoordinator, diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java index 9272f49242eba..13b0d1ba1c5de 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java @@ -83,10 +83,9 @@ public class RecordAccumulatorTest { private MockTime time = new MockTime(); private byte[] key = "key".getBytes(); private byte[] value = "value".getBytes(); - private int msgSize = DefaultRecord.sizeInBytes(0, 0, key.length, value.length, - Record.EMPTY_HEADERS); + private int msgSize = DefaultRecord.sizeInBytes(0, 0, key.length, value.length, Record.EMPTY_HEADERS); private Cluster cluster = new Cluster(null, Arrays.asList(node1, node2), Arrays.asList(part1, part2, part3), - Collections.emptySet(), Collections.emptySet()); + Collections.emptySet(), Collections.emptySet()); private Metrics metrics = new Metrics(time); private final long maxBlockTimeMs = 1000; private final LogContext logContext = new LogContext(); @@ -104,7 +103,7 @@ public void testFull() throws Exception { int batchSize = 1025; RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10L * batchSize, CompressionType.NONE, 10L); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10L * batchSize, CompressionType.NONE, 10L); int appends = expectedNumAppends(batchSize); for (int i = 0; i < appends; i++) { // append to the first batch @@ -153,7 +152,7 @@ private void testAppendLarge(CompressionType compressionType) throws Exception { int batchSize = 512; byte[] value = new byte[2 * batchSize]; RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, compressionType, 0L); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, compressionType, 0L); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("Our partition's leader should be ready", Collections.singleton(node1), accum.ready(cluster, time.milliseconds()).readyNodes); @@ -189,10 +188,10 @@ private void testAppendLargeOldMessageFormat(CompressionType compressionType) th ApiVersions apiVersions = new ApiVersions(); apiVersions.update(node1.idString(), NodeApiVersions.create(Collections.singleton( - new ApiVersionsResponse.ApiVersion(ApiKeys.PRODUCE.id, (short) 0, (short) 2)))); + new ApiVersionsResponse.ApiVersion(ApiKeys.PRODUCE.id, (short) 0, (short) 2)))); RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, compressionType, 0L); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, compressionType, 0L); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("Our partition's leader should be ready", Collections.singleton(node1), accum.ready(cluster, time.milliseconds()).readyNodes); @@ -216,7 +215,7 @@ private void testAppendLargeOldMessageFormat(CompressionType compressionType) th public void testLinger() throws Exception { long lingerMs = 10L; RecordAccumulator accum = createTestRecordAccumulator( - 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, lingerMs); + 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, lingerMs); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("No partitions should be ready", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); time.sleep(10); @@ -235,7 +234,7 @@ public void testLinger() throws Exception { @Test public void testPartialDrain() throws Exception { RecordAccumulator accum = createTestRecordAccumulator( - 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, 10L); + 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * 1024, CompressionType.NONE, 10L); int appends = 1024 / msgSize + 1; List partitions = asList(tp1, tp2); for (TopicPartition tp : partitions) { @@ -299,8 +298,8 @@ public void testNextReadyCheckDelay() throws Exception { // test case assumes that the records do not fill the batch completely int batchSize = 1025; - RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); + RecordAccumulator accum = createTestRecordAccumulator(batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, + 10 * batchSize, CompressionType.NONE, lingerMs); // Just short of going over the limit so we trigger linger time int appends = expectedNumAppends(batchSize); @@ -341,7 +340,7 @@ public void testRetryBackoff() throws Exception { String metricGrpName = "producer-metrics"; final RecordAccumulator accum = new RecordAccumulator(logContext, batchSize, - CompressionType.NONE, lingerMs, retryBackoffMs, requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, time, new ApiVersions(), null, + CompressionType.NONE, lingerMs, retryBackoffMs, deliveryTimeoutMs, metrics, metricGrpName, time, new ApiVersions(), null, new BufferPool(totalSize, batchSize, metrics, time, metricGrpName)); long now = time.milliseconds(); @@ -378,9 +377,9 @@ CompressionType.NONE, lingerMs, retryBackoffMs, requestTimeoutMs, deliveryTimeou @Test public void testFlush() throws Exception { - long lingerMs = Long.MAX_VALUE; + long lingerMs = Integer.MAX_VALUE; final RecordAccumulator accum = createTestRecordAccumulator( - 4 * 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); + 4 * 1024 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); for (int i = 0; i < 100; i++) { accum.append(new TopicPartition(topic, i % 3), 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); @@ -436,7 +435,7 @@ public void testAwaitFlushComplete() throws Exception { @Test public void testAbortIncompleteBatches() throws Exception { - long lingerMs = Long.MAX_VALUE; + int lingerMs = Integer.MAX_VALUE; int numRecords = 100; final AtomicInteger numExceptionReceivedInCallback = new AtomicInteger(0); @@ -475,12 +474,12 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { @Test public void testAbortUnsentBatches() throws Exception { - long lingerMs = Long.MAX_VALUE; + int lingerMs = Integer.MAX_VALUE; int numRecords = 100; final AtomicInteger numExceptionReceivedInCallback = new AtomicInteger(0); final RecordAccumulator accum = createTestRecordAccumulator( - 128 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); + 128 + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 64 * 1024, CompressionType.NONE, lingerMs); final KafkaException cause = new KafkaException(); class TestCallback implements Callback { @@ -495,7 +494,7 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { RecordAccumulator.ReadyCheckResult result = accum.ready(cluster, time.milliseconds()); assertFalse(result.readyNodes.isEmpty()); Map> drained = accum.drain(cluster, result.readyNodes, Integer.MAX_VALUE, - time.milliseconds()); + time.milliseconds()); assertTrue(accum.hasUndrained()); assertTrue(accum.hasIncomplete()); @@ -517,64 +516,51 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { } private void doExpireBatchSingle(long deliveryTimeoutMs) throws InterruptedException { - long lingerMs = 3000L; + long lingerMs = 300L; List muteStates = Arrays.asList(false, true); Set readyNodes = null; List expiredBatches = new ArrayList<>(); - // test case assumes that the records do not fill the batch completely int batchSize = 1025; - RecordAccumulator accum = createTestRecordAccumulator(deliveryTimeoutMs, batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); // Make the batches ready due to linger. These batches are not in retry for (Boolean mute: muteStates) { + if (time.milliseconds() < System.currentTimeMillis()) + time.setCurrentTimeMs(System.currentTimeMillis()); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("No partition should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); time.sleep(lingerMs); readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; assertEquals("Our partition's leader should be ready", Collections.singleton(node1), readyNodes); - List batches = accum.expiredBatches(time.milliseconds()); - expiredBatches.addAll(batches); - assertEquals("The batch should not expire when just linger has passed", 0, batches.size()); + expiredBatches = accum.expiredBatches(time.milliseconds()); + assertEquals("The batch should not expire when just linger has passed", 0, expiredBatches.size()); if (mute) accum.mutePartition(tp1); else accum.unmutePartition(tp1, 0L); - System.out.println(tp1 + " is muted ? " + mute); - System.out.println("deliveryTimeoutMs: " + deliveryTimeoutMs + ", lingerMs: " + lingerMs); // Advance the clock to expire the batch. time.sleep(deliveryTimeoutMs - lingerMs); - - System.out.println("time.milliseconds = " + time.milliseconds()); - batches = accum.expiredBatches(time.milliseconds()); - // < 0 ? Long.MAX_VALUE : time.milliseconds()); - expiredBatches.addAll(batches); - - if (expiredBatches.size() != 1) { - System.out.println("mute ? " + mute); - } + expiredBatches = accum.expiredBatches(time.milliseconds()); + assertEquals("The batch may expire when the partition is muted", 1, expiredBatches.size()); + assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); } - assertEquals("The batch may expire when the partition is muted", 1, expiredBatches.size()); - - assertEquals("No partitions should be ready.", 0, - accum.ready(cluster, time.milliseconds()).readyNodes.size()); } - //@Test - //public void testExpiredBatchSingle() throws InterruptedException { - // doExpireBatchSingle(3200L); - //} + @Test + public void testExpiredBatchSingle() throws InterruptedException { + doExpireBatchSingle(3200L); + } - //@Test - //public void testExpiredBatchSingleMaxValue() throws InterruptedException { - // doExpireBatchSingle(Long.MAX_VALUE); - //} + @Test + public void testExpiredBatchSingleMaxValue() throws InterruptedException { + doExpireBatchSingle(Long.MAX_VALUE); + } @Test public void testExpiredBatches() throws InterruptedException { @@ -584,20 +570,17 @@ public void testExpiredBatches() throws InterruptedException { long deliveryTimeoutMs = 3200L; // test case assumes that the records do not fill the batch completely - int batchPayloadSize = 1025; - int batchSize = batchPayloadSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD; - RecordAccumulator accum = createTestRecordAccumulator( - deliveryTimeoutMs, batchSize, 10 * batchPayloadSize, CompressionType.NONE, lingerMs); - int appends = expectedNumAppends(batchPayloadSize); + int batchSize = 1025; - System.out.println("Expected # of append is " + appends + " for batch size " + batchSize); + RecordAccumulator accum = createTestRecordAccumulator( + deliveryTimeoutMs, batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); + int appends = expectedNumAppends(batchSize); // Test batches not in retry for (int i = 0; i < appends; i++) { accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); } - // Make the batches ready due to batch full accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, 0); Set readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; @@ -605,45 +588,26 @@ public void testExpiredBatches() throws InterruptedException { // Advance the clock to expire the batch. time.sleep(deliveryTimeoutMs + 1); accum.mutePartition(tp1); - - Map> drainResult = accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); - List expiredBatches = accum.expiredBatches(time.milliseconds()); - assertEquals("The batch should not be expired when the partition is muted", 0, expiredBatches.size()); + assertEquals("The batches will be muted no matter if the partition is muted or not", 2, expiredBatches.size()); accum.unmutePartition(tp1, 0L); - - //time.sleep(deliveryTimeMs - requestTimeout - lingerMs); - - - drainResult = accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); - - System.out.println("=================================================="); - for (Map.Entry> entry: drainResult.entrySet()) { - System.out.println(entry.getKey() + " :-->: " + entry.getValue()); - } - - expiredBatches = accum.expiredBatches(time.milliseconds()); - - assertEquals("The batch should be expired", 1, expiredBatches.size()); - assertEquals("One partitions should be ready.", 1, accum.ready(cluster, time.milliseconds()).readyNodes.size()); + assertEquals("All batches should have been expired earlier", 0, expiredBatches.size()); + assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); // Advance the clock to make the next batch ready due to linger.ms time.sleep(lingerMs); assertEquals("Our partition's leader should be ready", Collections.singleton(node1), readyNodes); time.sleep(requestTimeout + 1); - accum.mutePartition(tp1); - drainResult = accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when metadata is still available and partition is muted", 0, expiredBatches.size()); accum.unmutePartition(tp1, 0L); - drainResult = accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); expiredBatches = accum.expiredBatches(time.milliseconds()); - assertEquals("The batch should be expired when the partition is not muted", 1, expiredBatches.size()); + assertEquals("All batches should have been expired", 0, expiredBatches.size()); assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); // Test batches in retry. @@ -658,21 +622,18 @@ public void testExpiredBatches() throws InterruptedException { accum.reenqueue(drained.get(node1.id()).get(0), time.milliseconds()); // test expiration. - time.sleep(deliveryTimeoutMs); + time.sleep(requestTimeout + retryBackoffMs); expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired.", 0, expiredBatches.size()); time.sleep(1L); accum.mutePartition(tp1); - accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when the partition is muted", 0, expiredBatches.size()); accum.unmutePartition(tp1, 0L); - accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); expiredBatches = accum.expiredBatches(time.milliseconds()); - assertEquals("The batch should be expired when the partition is not muted.", 1, expiredBatches.size()); - + assertEquals("All batches should have been expired.", 0, expiredBatches.size()); // Test that when being throttled muted batches are expired before the throttle time is over. accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, 0); @@ -680,25 +641,22 @@ public void testExpiredBatches() throws InterruptedException { readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; assertEquals("Our partition's leader should be ready", Collections.singleton(node1), readyNodes); // Advance the clock to expire the batch. - time.sleep(deliveryTimeoutMs - lingerMs); + time.sleep(requestTimeout + 1); accum.mutePartition(tp1); - accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when the partition is muted", 0, expiredBatches.size()); long throttleTimeMs = 100L; accum.unmutePartition(tp1, time.milliseconds() + throttleTimeMs); // The batch shouldn't be expired yet. - accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); expiredBatches = accum.expiredBatches(time.milliseconds()); assertEquals("The batch should not be expired when the partition is muted", 0, expiredBatches.size()); // Once the throttle time is over, the batch can be expired. time.sleep(throttleTimeMs); - accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); expiredBatches = accum.expiredBatches(time.milliseconds()); - assertEquals("The batch should be expired", 1, expiredBatches.size()); - assertEquals("No partitions should be ready.", 0, accum.ready(cluster, time.milliseconds()).readyNodes.size()); + assertEquals("All batches should have been expired earlier", 0, expiredBatches.size()); + assertEquals("No partitions should be ready.", 1, accum.ready(cluster, time.milliseconds()).readyNodes.size()); } @Test @@ -708,7 +666,7 @@ public void testMutedPartitions() throws InterruptedException { int batchSize = 1025; RecordAccumulator accum = createTestRecordAccumulator( - batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, 10); + batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, 10); int appends = expectedNumAppends(batchSize); for (int i = 0; i < appends; i++) { accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); @@ -752,7 +710,7 @@ public void testIdempotenceWithOldMagic() throws InterruptedException { apiVersions.update("foobar", NodeApiVersions.create(Arrays.asList(new ApiVersionsResponse.ApiVersion(ApiKeys.PRODUCE.id, (short) 0, (short) 2)))); RecordAccumulator accum = new RecordAccumulator(logContext, batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, - CompressionType.NONE, lingerMs, retryBackoffMs, requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, new TransactionManager(), + CompressionType.NONE, lingerMs, retryBackoffMs, deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, new TransactionManager(), new BufferPool(totalSize, batchSize, metrics, time, metricGrpName)); accum.append(tp1, 0L, key, value, Record.EMPTY_HEADERS, null, 0); } @@ -831,9 +789,9 @@ public void testSplitBatchOffAccumulator() throws InterruptedException { assertFalse(drained.get(node1.id()).isEmpty()); } assertTrue("All the batches should have been drained.", - accum.ready(cluster, time.milliseconds()).readyNodes.isEmpty()); + accum.ready(cluster, time.milliseconds()).readyNodes.isEmpty()); assertEquals("The split batches should be allocated off the accumulator", - bufferCapacity, accum.bufferPoolAvailableMemory()); + bufferCapacity, accum.bufferPoolAvailableMemory()); } @Test @@ -853,7 +811,7 @@ public void testSplitFrequency() throws InterruptedException { for (int i = 0; i < numMessages; i++) { int dice = random.nextInt(100); byte[] value = (dice < goodCompRatioPercentage) ? - bytesWithGoodCompression(random) : bytesWithPoorCompression(random, 100); + bytesWithGoodCompression(random) : bytesWithPoorCompression(random, 100); accum.append(tp1, 0L, null, value, Record.EMPTY_HEADERS, null, 0); BatchDrainedResult result = completeOrSplitBatches(accum, batchSize); numSplit += result.numSplit; @@ -881,7 +839,7 @@ public void testSoonToExpireBatchesArePickedUpForExpiry() throws InterruptedExce Set readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; Map> drained = accum.drain(cluster, readyNodes, Integer.MAX_VALUE, time.milliseconds()); assertTrue(drained.isEmpty()); - assertTrue(accum.soonToExpireInFlightBatches().isEmpty()); + //assertTrue(accum.soonToExpireInFlightBatches().isEmpty()); // advanced clock and send one batch out but it should not be included in soon to expire inflight // batches because batch's expiry is quite far. @@ -889,7 +847,7 @@ public void testSoonToExpireBatchesArePickedUpForExpiry() throws InterruptedExce readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; drained = accum.drain(cluster, readyNodes, Integer.MAX_VALUE, time.milliseconds()); assertEquals("A batch did not drain after linger", 1, drained.size()); - assertTrue(accum.soonToExpireInFlightBatches().isEmpty()); + //assertTrue(accum.soonToExpireInFlightBatches().isEmpty()); // Queue another batch and advance clock such that batch expiry time is earlier than request timeout. accum.append(tp2, 0L, key, value, Record.EMPTY_HEADERS, null, maxBlockTimeMs); @@ -899,21 +857,19 @@ public void testSoonToExpireBatchesArePickedUpForExpiry() throws InterruptedExce readyNodes = accum.ready(cluster, time.milliseconds()).readyNodes; drained = accum.drain(cluster, readyNodes, Integer.MAX_VALUE, time.milliseconds()); assertEquals("A batch did not drain after linger", 1, drained.size()); - assertEquals("A soon to expire batch was ignored", 1, accum.soonToExpireInFlightBatches().size()); } @Test public void testExpiredBatchesRetry() throws InterruptedException { - long lingerMs = 3000L; - long rtt = 1000; - long deliveryTimeoutMs = 3200L; - Set readyNodes = null; - List expiredBatches = null; + int lingerMs = 3000; + int rtt = 1000; + int deliveryTimeoutMs = 3200; + Set readyNodes; + List expiredBatches; List muteStates = Arrays.asList(false, true); // test case assumes that the records do not fill the batch completely int batchSize = 1025; - RecordAccumulator accum = createTestRecordAccumulator( batchSize + DefaultRecordBatch.RECORD_BATCH_OVERHEAD, 10 * batchSize, CompressionType.NONE, lingerMs); @@ -937,9 +893,7 @@ public void testExpiredBatchesRetry() throws InterruptedException { time.sleep(deliveryTimeoutMs - rtt); accum.drain(cluster, Collections.singleton(node1), Integer.MAX_VALUE, time.milliseconds()); expiredBatches = accum.expiredBatches(time.milliseconds()); - - System.out.println("mute = " + mute); - assertEquals("The batch in retry may expire.", mute ? 0 : 1, expiredBatches.size()); + assertEquals("RecordAccumulator has expired batches if the partition is not muted", mute ? 1 : 0, expiredBatches.size()); } } @@ -1058,7 +1012,6 @@ private RecordAccumulator createTestRecordAccumulator(long deliveryTimeoutMs, in type, lingerMs, retryBackoffMs, - requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java index 1ff28b813a301..2fbe3df20675b 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java @@ -132,10 +132,12 @@ public void testSimple() throws Exception { sender.run(time.milliseconds()); // connect sender.run(time.milliseconds()); // send produce request assertEquals("We should have a single produce request in flight.", 1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); assertTrue(client.hasInFlightRequests()); client.respond(produceResponse(tp0, offset, Errors.NONE, 0)); sender.run(time.milliseconds()); assertEquals("All requests completed.", 0, client.inFlightRequestCount()); + assertEquals(0, sender.inFlightBatches(tp0).size()); assertFalse(client.hasInFlightRequests()); sender.run(time.milliseconds()); assertTrue("Request should be completed", future.isDone()); @@ -329,33 +331,42 @@ public void testRetries() throws Exception { Node node = new Node(Integer.parseInt(id), "localhost", 0); assertEquals(1, client.inFlightRequestCount()); assertTrue(client.hasInFlightRequests()); + assertEquals(1, sender.inFlightBatches(tp0).size()); assertTrue("Client ready status should be true", client.isReady(node, 0L)); client.disconnect(id); assertEquals(0, client.inFlightRequestCount()); assertFalse(client.hasInFlightRequests()); assertFalse("Client ready status should be false", client.isReady(node, 0L)); + // the batch is in accumulator.inFlightBatches until it expires + assertEquals(1, sender.inFlightBatches(tp0).size()); sender.run(time.milliseconds()); // receive error sender.run(time.milliseconds()); // reconnect sender.run(time.milliseconds()); // resend assertEquals(1, client.inFlightRequestCount()); assertTrue(client.hasInFlightRequests()); + assertEquals(1, sender.inFlightBatches(tp0).size()); long offset = 0; client.respond(produceResponse(tp0, offset, Errors.NONE, 0)); sender.run(time.milliseconds()); assertTrue("Request should have retried and completed", future.isDone()); assertEquals(offset, future.get().offset()); + assertEquals(0, sender.inFlightBatches(tp0).size()); // do an unsuccessful retry future = accumulator.append(tp0, 0L, "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send produce request + assertEquals(1, sender.inFlightBatches(tp0).size()); for (int i = 0; i < maxRetries + 1; i++) { client.disconnect(client.requests().peek().destination()); sender.run(time.milliseconds()); // receive error + assertEquals(0, sender.inFlightBatches(tp0).size()); sender.run(time.milliseconds()); // reconnect sender.run(time.milliseconds()); // resend + assertEquals(i > 0 ? 0 : 1, sender.inFlightBatches(tp0).size()); } sender.run(time.milliseconds()); assertFutureFailure(future, NetworkException.class); + assertEquals(0, sender.inFlightBatches(tp0).size()); } finally { m.close(); } @@ -372,7 +383,7 @@ public void testSendInOrder() throws Exception { senderMetrics, time, REQUEST_TIMEOUT, 50, null, apiVersions); // Create a two broker cluster, with partition 0 on broker 0 and partition 1 on broker 1 Cluster cluster1 = TestUtils.clusterWith(2, "test", 2); - metadata.update(cluster1, Collections.emptySet(), time.milliseconds()); + metadata.update(cluster1, Collections.emptySet(), time.milliseconds()); // Send the first message. TopicPartition tp2 = new TopicPartition("test", 1); @@ -385,6 +396,7 @@ public void testSendInOrder() throws Exception { assertEquals(1, client.inFlightRequestCount()); assertTrue(client.hasInFlightRequests()); assertTrue("Client ready status should be true", client.isReady(node, 0L)); + assertEquals(1, sender.inFlightBatches(tp2).size()); time.sleep(900); // Now send another message to tp2 @@ -392,11 +404,13 @@ public void testSendInOrder() throws Exception { // Update metadata before sender receives response from broker 0. Now partition 2 moves to broker 0 Cluster cluster2 = TestUtils.singletonCluster("test", 2); - metadata.update(cluster2, Collections.emptySet(), time.milliseconds()); + metadata.update(cluster2, Collections.emptySet(), time.milliseconds()); // Sender should not send the second message to node 0. - sender.run(time.milliseconds()); + assertEquals(1, sender.inFlightBatches(tp2).size()); + sender.run(time.milliseconds()); // receive the response for the previous send, and send the new batch assertEquals(1, client.inFlightRequestCount()); assertTrue(client.hasInFlightRequests()); + assertEquals(1, sender.inFlightBatches(tp2).size()); } finally { m.close(); } @@ -432,7 +446,9 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { time.sleep(10000); Node clusterNode = this.cluster.nodes().get(0); - accumulator.drain(cluster, Collections.singleton(clusterNode), Integer.MAX_VALUE, time.milliseconds()); + Map> drainedBatches = + accumulator.drain(cluster, Collections.singleton(clusterNode), Integer.MAX_VALUE, time.milliseconds()); + sender.addToInflightBatches(drainedBatches); // Disconnect the target node for the pending produce request. This will ensure that sender will try to // expire the batch. @@ -440,7 +456,6 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { client.blackout(clusterNode, 100); sender.run(time.milliseconds()); // We should try to flush the batch, but we expire it instead without sending anything. - assertEquals("Callbacks not invoked for expiry", messagesPerBatch, expiryCallbackCount.get()); assertNull("Unexpected exception", unexpectedException.get()); // Make sure that the reconds were appended back to the batch. @@ -467,6 +482,7 @@ public void testMetadataTopicExpiry() throws Exception { sender.run(time.milliseconds()); assertEquals("Request completed.", 0, client.inFlightRequestCount()); assertFalse(client.hasInFlightRequests()); + assertEquals(0, sender.inFlightBatches(tp0).size()); sender.run(time.milliseconds()); assertTrue("Request should be completed", future.isDone()); @@ -483,6 +499,7 @@ public void testMetadataTopicExpiry() throws Exception { sender.run(time.milliseconds()); assertEquals("Request completed.", 0, client.inFlightRequestCount()); assertFalse(client.hasInFlightRequests()); + assertEquals(0, sender.inFlightBatches(tp0).size()); sender.run(time.milliseconds()); assertTrue("Request should be completed", future.isDone()); } @@ -524,6 +541,7 @@ public void testCanRetryWithoutIdempotence() throws Exception { Node node = new Node(Integer.parseInt(id), "localhost", 0); assertEquals(1, client.inFlightRequestCount()); assertTrue(client.hasInFlightRequests()); + assertEquals(1, sender.inFlightBatches(tp0).size()); assertTrue("Client ready status should be true", client.isReady(node, 0L)); assertFalse(future.isDone()); @@ -587,6 +605,7 @@ public void testIdempotenceWithMultipleInflights() throws Exception { sender.run(time.milliseconds()); // receive response 1 assertEquals(1, transactionManager.lastAckedSequence(tp0)); assertFalse(client.hasInFlightRequests()); + assertEquals(0, sender.inFlightBatches(tp0).size()); assertTrue(request2.isDone()); assertEquals(1, request2.get().offset()); } @@ -658,11 +677,12 @@ public void testIdempotenceWithMultipleInflightsRetriedInOrder() throws Exceptio assertEquals(0, transactionManager.lastAckedSequence(tp0)); assertTrue(request1.isDone()); assertEquals(0, request1.get().offset()); - - assertFalse(client.hasInFlightRequests()); + assertEquals(0, sender.inFlightBatches(tp0).size()); + sender.run(time.milliseconds()); // send request 2; assertEquals(1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); sendIdempotentProducerResponse(1, tp0, Errors.NONE, 1L); sender.run(time.milliseconds()); // receive response 2 @@ -671,17 +691,19 @@ public void testIdempotenceWithMultipleInflightsRetriedInOrder() throws Exceptio assertEquals(1, request2.get().offset()); assertFalse(client.hasInFlightRequests()); + assertEquals(0, sender.inFlightBatches(tp0).size()); sender.run(time.milliseconds()); // send request 3 assertEquals(1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); sendIdempotentProducerResponse(2, tp0, Errors.NONE, 2L); sender.run(time.milliseconds()); // receive response 3, send request 4 since we are out of 'retry' mode. assertEquals(2, transactionManager.lastAckedSequence(tp0)); assertTrue(request3.isDone()); assertEquals(2, request3.get().offset()); - assertEquals(1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); sendIdempotentProducerResponse(3, tp0, Errors.NONE, 3L); sender.run(time.milliseconds()); // receive response 4 @@ -799,7 +821,6 @@ public void testCorrectHandlingOfOutOfOrderResponses() throws Exception { setupWithTransactionState(transactionManager); prepareAndReceiveInitProducerId(producerId, Errors.NONE); assertTrue(transactionManager.hasProducerId()); - assertEquals(0, transactionManager.sequenceNumber(tp0).longValue()); // Send first ProduceRequest @@ -969,10 +990,9 @@ public void testExpiryOfUnsentBatchesShouldNotCauseUnresolvedSequences() throws public void testExpiryOfFirstBatchShouldNotCauseUnresolvedSequencesIfFutureBatchesSucceed() throws Exception { final long producerId = 343434L; TransactionManager transactionManager = new TransactionManager(); - setupWithTransactionState(transactionManager); + setupWithTransactionState(transactionManager, false, null); prepareAndReceiveInitProducerId(producerId, Errors.NONE); assertTrue(transactionManager.hasProducerId()); - assertEquals(0, transactionManager.sequenceNumber(tp0).longValue()); // Send first ProduceRequest @@ -984,11 +1004,12 @@ public void testExpiryOfFirstBatchShouldNotCauseUnresolvedSequencesIfFutureBatch Future request2 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request - assertEquals(2, client.inFlightRequestCount()); + assertEquals(2, sender.inFlightBatches(tp0).size()); sendIdempotentProducerResponse(0, tp0, Errors.REQUEST_TIMED_OUT, -1); sender.run(time.milliseconds()); // receive first response + assertEquals(1, sender.inFlightBatches(tp0).size()); Node node = this.cluster.nodes().get(0); // We add 600 millis to expire the first batch but not the second. @@ -1000,20 +1021,23 @@ public void testExpiryOfFirstBatchShouldNotCauseUnresolvedSequencesIfFutureBatch sender.run(time.milliseconds()); // now expire the first batch. assertFutureFailure(request1, TimeoutException.class); assertTrue(transactionManager.hasUnresolvedSequence(tp0)); + assertEquals(0, sender.inFlightBatches(tp0).size()); + // let's enqueue another batch, which should not be dequeued until the unresolved state is clear. Future request3 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; - time.sleep(20); - assertFalse(request2.isDone()); sender.run(time.milliseconds()); // send second request sendIdempotentProducerResponse(1, tp0, Errors.NONE, 1); + assertEquals(1, sender.inFlightBatches(tp0).size()); + sender.run(time.milliseconds()); // receive second response, the third request shouldn't be sent since we are in an unresolved state. assertTrue(request2.isDone()); assertEquals(1, request2.get().offset()); - Deque batches = accumulator.batches().get(tp0); + assertEquals(0, sender.inFlightBatches(tp0).size()); + Deque batches = accumulator.batches().get(tp0); assertEquals(1, batches.size()); assertFalse(batches.peekFirst().hasSequence()); assertFalse(client.hasInFlightRequests()); @@ -1026,6 +1050,7 @@ public void testExpiryOfFirstBatchShouldNotCauseUnresolvedSequencesIfFutureBatch assertEquals(0, batches.size()); assertEquals(1, client.inFlightRequestCount()); assertFalse(request3.isDone()); + assertEquals(1, sender.inFlightBatches(tp0).size()); } @Test @@ -1035,16 +1060,13 @@ public void testExpiryOfFirstBatchShouldCauseResetIfFutureBatchesFail() throws E setupWithTransactionState(transactionManager); prepareAndReceiveInitProducerId(producerId, Errors.NONE); assertTrue(transactionManager.hasProducerId()); - assertEquals(0, transactionManager.sequenceNumber(tp0).longValue()); // Send first ProduceRequest Future request1 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request - // We separate the two appends by 1 second so that the two batches - // don't expire at the same time. - time.sleep(1000L); + time.sleep(1000L); Future request2 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request @@ -1054,9 +1076,7 @@ public void testExpiryOfFirstBatchShouldCauseResetIfFutureBatchesFail() throws E sender.run(time.milliseconds()); // receive first response Node node = this.cluster.nodes().get(0); - // We add 600 millis to expire the first batch but not the second. - // Note deliveryTimeoutMs is 1500. - time.sleep(600L); + time.sleep(1000L); client.disconnect(node.idString()); client.blackout(node, 10); @@ -1067,9 +1087,7 @@ public void testExpiryOfFirstBatchShouldCauseResetIfFutureBatchesFail() throws E Future request3 = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; time.sleep(20); - assertFalse(request2.isDone()); - sender.run(time.milliseconds()); // send second request sendIdempotentProducerResponse(1, tp0, Errors.OUT_OF_ORDER_SEQUENCE_NUMBER, 1); sender.run(time.milliseconds()); // receive second response, the third request shouldn't be sent since we are in an unresolved state. @@ -1101,6 +1119,7 @@ public void testExpiryOfAllSentBatchesShouldCauseUnresolvedSequences() throws Ex Future request1 = accumulator.append(tp0, 0L, "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request sendIdempotentProducerResponse(0, tp0, Errors.NOT_LEADER_FOR_PARTITION, -1); + sender.run(time.milliseconds()); // receive response assertEquals(1L, transactionManager.sequenceNumber(tp0).longValue()); @@ -1533,7 +1552,6 @@ public boolean matches(AbstractRequest body) { RecordBatch firstBatch = batchIterator.next(); assertFalse(batchIterator.hasNext()); assertEquals(expectedSequence, firstBatch.baseSequence()); - return true; } }, produceResponse(tp, responseOffset, responseError, 0, logStartOffset)); @@ -1767,11 +1785,13 @@ public void testResetWhenOutOfOrderSequenceReceived() throws InterruptedExceptio sender.run(time.milliseconds()); // send. assertEquals(1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); client.respond(produceResponse(tp0, 0, Errors.OUT_OF_ORDER_SEQUENCE_NUMBER, 0)); sender.run(time.milliseconds()); assertTrue(responseFuture.isDone()); + assertEquals(0, sender.inFlightBatches(tp0).size()); assertFalse("Expected transaction state to be reset upon receiving an OutOfOrderSequenceException", transactionManager.hasProducerId()); } @@ -1807,7 +1827,6 @@ private void testSplitBatchAndSend(TransactionManager txnManager, TopicPartition tp) throws Exception { int maxRetries = 1; String topic = tp.topic(); - int requestTimeoutMs = 1500; long deliveryTimeoutMs = 3000L; long totalSize = 1024 * 1024; String metricGrpName = "producer-metrics"; @@ -1815,7 +1834,7 @@ private void testSplitBatchAndSend(TransactionManager txnManager, CompressionRatioEstimator.setEstimation(topic, CompressionType.GZIP, 0.2f); try (Metrics m = new Metrics()) { accumulator = new RecordAccumulator(logContext, batchSize, CompressionType.GZIP, - 0L, 0L, requestTimeoutMs, deliveryTimeoutMs, m, metricGrpName, time, new ApiVersions(), txnManager, + 0L, 0L, deliveryTimeoutMs, m, metricGrpName, time, new ApiVersions(), txnManager, new BufferPool(totalSize, batchSize, metrics, time, "producer-internal-metrics")); SenderMetricsRegistry senderMetrics = new SenderMetricsRegistry(m); Sender sender = new Sender(logContext, client, metadata, this.accumulator, true, MAX_REQUEST_SIZE, ACKS_ALL, maxRetries, @@ -1883,9 +1902,7 @@ private void testSplitBatchAndSend(TransactionManager txnManager, assertEquals("The last ack'd sequence number should be 1", 1, txnManager.lastAckedSequence(tp)); assertEquals("Offset of the first message should be 1", 1L, f2.get().offset()); assertTrue("There should be no batch in the accumulator", accumulator.batches().get(tp).isEmpty()); - - assertTrue("There should be a split", - m.metrics().get(senderMetrics.batchSplitRate).value() > 0); + assertTrue("There should be a split", m.metrics().get(senderMetrics.batchSplitRate).value() > 0); } } @@ -1902,32 +1919,32 @@ public void testNoDoubleDeallocation() throws Exception { accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request assertEquals(1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); + time.sleep(deliverTimeoutMs); + assertFalse(pool.allMatch()); + sender.run(time.milliseconds()); // expire the batch assertTrue(request1.isDone()); - // make sure the batch got deallocated right after expiry + assertTrue("The batch should have been de-allocated", pool.allMatch()); assertTrue(pool.allMatch()); + sender.run(time.milliseconds()); + assertTrue("The batch should have been de-allocated", pool.allMatch()); assertEquals(0, client.inFlightRequestCount()); - //client.respond(produceResponse(tp0, -1, Errors.NOT_LEADER_FOR_PARTITION, -1)); // return a retriable error - - //sender.run(time.milliseconds()); // receive first response and do not reenqueue. - //assertEquals(0, client.inFlightRequestCount()); - //sender.run(time.milliseconds()); // run again and must not send anything. - //assertEquals(0, client.inFlightRequestCount()); - //assertTrue(pool.allMatch()); + assertEquals(0, sender.inFlightBatches(tp0).size()); } @Test public void testInflightBatchesExpireOnDeliveryTimeout() throws InterruptedException { long deliveryTimeoutMs = 1500L; - setupWithTransactionState(null, true, null); // Send first ProduceRequest Future request = accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT).future; sender.run(time.milliseconds()); // send request assertEquals(1, client.inFlightRequestCount()); + assertEquals("Expect one in-flight batch in accumulator", 1, sender.inFlightBatches(tp0).size()); Map responseMap = new HashMap<>(); responseMap.put(tp0, new ProduceResponse.PartitionResponse(Errors.NONE, 0L, 0L, 0L)); @@ -1935,6 +1952,7 @@ public void testInflightBatchesExpireOnDeliveryTimeout() throws InterruptedExcep time.sleep(deliveryTimeoutMs); sender.run(time.milliseconds()); // receive first response + assertEquals("Expect zero in-flight batch in accumulator", 0, sender.inFlightBatches(tp0).size()); try { request.get(); fail("The expired batch should throw a TimeoutException"); @@ -1946,13 +1964,13 @@ public void testInflightBatchesExpireOnDeliveryTimeout() throws InterruptedExcep @Test public void testWhenFirstBatchExpireNoSendSecondBatchIfGuaranteeOrder() throws InterruptedException { long deliveryTimeoutMs = 1500L; - setupWithTransactionState(null, true, null); // Send first ProduceRequest accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT); sender.run(time.milliseconds()); // send request assertEquals(1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); time.sleep(deliveryTimeoutMs / 2); @@ -1960,17 +1978,18 @@ public void testWhenFirstBatchExpireNoSendSecondBatchIfGuaranteeOrder() throws I accumulator.append(tp0, time.milliseconds(), "key".getBytes(), "value".getBytes(), null, null, MAX_BLOCK_TIMEOUT); sender.run(time.milliseconds()); // must not send request because the partition is muted assertEquals(1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); time.sleep(deliveryTimeoutMs / 2); // expire the first batch only - sender.run(time.milliseconds()); - assertEquals(1, client.inFlightRequestCount()); client.respond(produceResponse(tp0, 0L, Errors.NONE, 0, 0L)); sender.run(time.milliseconds()); // receive response (offset=0) assertEquals(0, client.inFlightRequestCount()); + assertEquals(0, sender.inFlightBatches(tp0).size()); sender.run(time.milliseconds()); // Drain the second request only this time assertEquals(1, client.inFlightRequestCount()); + assertEquals(1, sender.inFlightBatches(tp0).size()); } @Test @@ -1992,15 +2011,16 @@ public void testExpiredBatchDoesNotRetry() throws Exception { sender.run(time.milliseconds()); // expire the batch assertTrue(request1.isDone()); - - System.out.println("client.inFlightRequestCount() = " + client.inFlightRequestCount()); assertEquals(0, client.inFlightRequestCount()); + assertEquals(0, sender.inFlightBatches(tp0).size()); + sender.run(time.milliseconds()); // receive first response and do not reenqueue. + assertEquals(0, client.inFlightRequestCount()); + assertEquals(0, sender.inFlightBatches(tp0).size()); - //sender.run(time.milliseconds()); // receive first response and do not reenqueue. - //assertEquals(0, client.inFlightRequestCount()); - //sender.run(time.milliseconds()); // run again and must not send anything. - //assertEquals(0, client.inFlightRequestCount()); + sender.run(time.milliseconds()); // run again and must not send anything. + assertEquals(0, client.inFlightRequestCount()); + assertEquals(0, sender.inFlightBatches(tp0).size()); } private class MatchingBufferPool extends BufferPool { @@ -2108,14 +2128,13 @@ private void setupWithTransactionState(TransactionManager transactionManager, bo private void setupWithTransactionState(TransactionManager transactionManager, boolean guaranteeOrder, Map metricTags, BufferPool pool) { long deliveryTimeoutMs = 1500L; - int requestTimeoutMs = 1500; String metricGrpName = "producer-metrics"; this.accumulator = new RecordAccumulator(logContext, batchSize, CompressionType.NONE, 0L, 0L, - requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, transactionManager, pool); + deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, transactionManager, pool); this.senderMetricsRegistry = new SenderMetricsRegistry(this.metrics); this.sender = new Sender(logContext, this.client, this.metadata, this.accumulator, guaranteeOrder, MAX_REQUEST_SIZE, ACKS_ALL, Integer.MAX_VALUE, this.senderMetricsRegistry, this.time, REQUEST_TIMEOUT, 50, transactionManager, apiVersions); - this.metadata.update(this.cluster, Collections.emptySet(), time.milliseconds()); + this.metadata.update(this.cluster, Collections.emptySet(), time.milliseconds()); } private void assertSendFailure(Class expectedError) throws Exception { diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java index 108099e92f42c..550d003406b3d 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java @@ -129,7 +129,7 @@ public void setup() { Metrics metrics = new Metrics(metricConfig, time); SenderMetricsRegistry senderMetrics = new SenderMetricsRegistry(metrics); - this.accumulator = new RecordAccumulator(logContext, batchSize, CompressionType.NONE, 0L, 0L, requestTimeoutMs, deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, transactionManager, new BufferPool(totalSize, batchSize, metrics, time, metricGrpName)); + this.accumulator = new RecordAccumulator(logContext, batchSize, CompressionType.NONE, 0L, 0L, deliveryTimeoutMs, metrics, metricGrpName, time, apiVersions, transactionManager, new BufferPool(totalSize, batchSize, metrics, time, metricGrpName)); this.sender = new Sender(logContext, this.client, this.metadata, this.accumulator, true, MAX_REQUEST_SIZE, ACKS_ALL, MAX_RETRIES, senderMetrics, this.time, REQUEST_TIMEOUT, 50, transactionManager, apiVersions); this.metadata.update(this.cluster, Collections.emptySet(), time.milliseconds()); diff --git a/core/src/test/scala/integration/kafka/api/BaseProducerSendTest.scala b/core/src/test/scala/integration/kafka/api/BaseProducerSendTest.scala index dc4041f1d63fc..739675ead6e49 100644 --- a/core/src/test/scala/integration/kafka/api/BaseProducerSendTest.scala +++ b/core/src/test/scala/integration/kafka/api/BaseProducerSendTest.scala @@ -68,7 +68,7 @@ abstract class BaseProducerSendTest extends KafkaServerTestHarness { super.tearDown() } - protected def createProducer(brokerList: String, retries: Int = 0, lingerMs: Long = 0, props: Option[Properties] = None): KafkaProducer[Array[Byte],Array[Byte]] = { + protected def createProducer(brokerList: String, retries: Int = 0, lingerMs: Int = 0, props: Option[Properties] = None): KafkaProducer[Array[Byte],Array[Byte]] = { val producer = TestUtils.createProducer(brokerList, securityProtocol = securityProtocol, trustStoreFile = trustStoreFile, saslProperties = clientSaslProperties, retries = retries, lingerMs = lingerMs, props = props) registerProducer(producer) @@ -170,13 +170,13 @@ abstract class BaseProducerSendTest extends KafkaServerTestHarness { def testSendCompressedMessageWithCreateTime() { val producerProps = new Properties() producerProps.setProperty(ProducerConfig.COMPRESSION_TYPE_CONFIG, "gzip") - val producer = createProducer(brokerList = brokerList, lingerMs = Long.MaxValue, props = Some(producerProps)) + val producer = createProducer(brokerList = brokerList, lingerMs = Int.MaxValue, props = Some(producerProps)) sendAndVerifyTimestamp(producer, TimestampType.CREATE_TIME) } @Test def testSendNonCompressedMessageWithCreateTime() { - val producer = createProducer(brokerList = brokerList, lingerMs = Long.MaxValue) + val producer = createProducer(brokerList = brokerList, lingerMs = Int.MaxValue) sendAndVerifyTimestamp(producer, TimestampType.CREATE_TIME) } @@ -409,7 +409,7 @@ abstract class BaseProducerSendTest extends KafkaServerTestHarness { */ @Test def testFlush() { - val producer = createProducer(brokerList, lingerMs = Long.MaxValue) + val producer = createProducer(brokerList, lingerMs = Int.MaxValue) try { createTopic(topic, 2, 2) val record = new ProducerRecord[Array[Byte], Array[Byte]](topic, @@ -438,7 +438,7 @@ abstract class BaseProducerSendTest extends KafkaServerTestHarness { // Test closing from caller thread. for (_ <- 0 until 50) { - val producer = createProducer(brokerList, lingerMs = Long.MaxValue) + val producer = createProducer(brokerList, lingerMs = Int.MaxValue) val responses = (0 until numRecords) map (_ => producer.send(record0)) assertTrue("No request is complete.", responses.forall(!_.isDone())) producer.close(0, TimeUnit.MILLISECONDS) @@ -478,7 +478,7 @@ abstract class BaseProducerSendTest extends KafkaServerTestHarness { } } for (i <- 0 until 50) { - val producer = createProducer(brokerList, lingerMs = Long.MaxValue) + val producer = createProducer(brokerList, lingerMs = Int.MaxValue) try { // send message to partition 0 // Only send the records in the first callback since we close the producer in the callback and no records diff --git a/core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala b/core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala index 02396dd1bb24d..ba4df7d64f65b 100644 --- a/core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala +++ b/core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala @@ -617,9 +617,9 @@ class PlaintextConsumerTest extends BaseConsumerTest { private def sendCompressedMessages(numRecords: Int, tp: TopicPartition) { val producerProps = new Properties() producerProps.setProperty(ProducerConfig.COMPRESSION_TYPE_CONFIG, CompressionType.GZIP.name) - producerProps.setProperty(ProducerConfig.LINGER_MS_CONFIG, Long.MaxValue.toString) + producerProps.setProperty(ProducerConfig.LINGER_MS_CONFIG, Int.MaxValue.toString) val producer = TestUtils.createProducer(brokerList, securityProtocol = securityProtocol, trustStoreFile = trustStoreFile, - saslProperties = clientSaslProperties, retries = 0, lingerMs = Long.MaxValue, props = Some(producerProps)) + saslProperties = clientSaslProperties, retries = 0, lingerMs = Int.MaxValue, props = Some(producerProps)) (0 until numRecords).foreach { i => producer.send(new ProducerRecord(tp.topic, tp.partition, i.toLong, s"key $i".getBytes, s"value $i".getBytes)) } diff --git a/core/src/test/scala/integration/kafka/api/PlaintextProducerSendTest.scala b/core/src/test/scala/integration/kafka/api/PlaintextProducerSendTest.scala index 929dbe40291cf..1da6f9ee3694d 100644 --- a/core/src/test/scala/integration/kafka/api/PlaintextProducerSendTest.scala +++ b/core/src/test/scala/integration/kafka/api/PlaintextProducerSendTest.scala @@ -45,7 +45,7 @@ class PlaintextProducerSendTest extends BaseProducerSendTest { def testBatchSizeZero() { val producerProps = new Properties() producerProps.setProperty(ProducerConfig.BATCH_SIZE_CONFIG, "0") - val producer = createProducer(brokerList = brokerList, lingerMs = Long.MaxValue, props = Some(producerProps)) + val producer = createProducer(brokerList = brokerList, lingerMs = Int.MaxValue, props = Some(producerProps)) sendAndVerify(producer) } @@ -53,13 +53,13 @@ class PlaintextProducerSendTest extends BaseProducerSendTest { def testSendCompressedMessageWithLogAppendTime() { val producerProps = new Properties() producerProps.setProperty(ProducerConfig.COMPRESSION_TYPE_CONFIG, "gzip") - val producer = createProducer(brokerList = brokerList, lingerMs = Long.MaxValue, props = Some(producerProps)) + val producer = createProducer(brokerList = brokerList, lingerMs = Int.MaxValue, props = Some(producerProps)) sendAndVerifyTimestamp(producer, TimestampType.LOG_APPEND_TIME) } @Test def testSendNonCompressedMessageWithLogAppendTime() { - val producer = createProducer(brokerList = brokerList, lingerMs = Long.MaxValue) + val producer = createProducer(brokerList = brokerList, lingerMs = Int.MaxValue) sendAndVerifyTimestamp(producer, TimestampType.LOG_APPEND_TIME) } diff --git a/core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala b/core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala index 9b77c2d4169f4..0227690e928f9 100644 --- a/core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala +++ b/core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala @@ -64,11 +64,11 @@ class ProducerFailureHandlingTest extends KafkaServerTestHarness { override def setUp() { super.setUp() - producer1 = TestUtils.createProducer(brokerList, acks = 0, requestTimeoutMs = 30000L, maxBlockMs = 10000L, + producer1 = TestUtils.createProducer(brokerList, acks = 0, requestTimeoutMs = 30000, maxBlockMs = 10000L, bufferSize = producerBufferSize) - producer2 = TestUtils.createProducer(brokerList, acks = 1, requestTimeoutMs = 30000L, maxBlockMs = 10000L, + producer2 = TestUtils.createProducer(brokerList, acks = 1, requestTimeoutMs = 30000, maxBlockMs = 10000L, bufferSize = producerBufferSize) - producer3 = TestUtils.createProducer(brokerList, acks = -1, requestTimeoutMs = 30000L, maxBlockMs = 10000L, + producer3 = TestUtils.createProducer(brokerList, acks = -1, requestTimeoutMs = 30000, maxBlockMs = 10000L, bufferSize = producerBufferSize) } diff --git a/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala b/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala index 49553e8d0716f..61d59192154c5 100644 --- a/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala +++ b/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala @@ -1368,11 +1368,11 @@ class DynamicBrokerReconfigurationTest extends ZooKeeperTestHarness with SaslSet private case class ProducerBuilder() extends ClientBuilder[KafkaProducer[String, String]] { private var _retries = 0 private var _acks = -1 - private var _requestTimeoutMs = 30000L + private var _requestTimeoutMs = 30000 def maxRetries(retries: Int): ProducerBuilder = { _retries = retries; this } def acks(acks: Int): ProducerBuilder = { _acks = acks; this } - def requestTimeoutMs(timeoutMs: Long): ProducerBuilder = { _requestTimeoutMs = timeoutMs; this } + def requestTimeoutMs(timeoutMs: Int): ProducerBuilder = { _requestTimeoutMs = timeoutMs; this } override def build(): KafkaProducer[String, String] = { val producer = TestUtils.createProducer(bootstrapServers, diff --git a/core/src/test/scala/unit/kafka/server/FetchRequestTest.scala b/core/src/test/scala/unit/kafka/server/FetchRequestTest.scala index 67f33ebdb4542..57aca1e73ba84 100644 --- a/core/src/test/scala/unit/kafka/server/FetchRequestTest.scala +++ b/core/src/test/scala/unit/kafka/server/FetchRequestTest.scala @@ -204,7 +204,7 @@ class FetchRequestTest extends BaseRequestTest { val propsOverride = new Properties propsOverride.put(ProducerConfig.BATCH_SIZE_CONFIG, batchSize.toString) val producer = TestUtils.createProducer(TestUtils.getBrokerListStrFromServers(servers), - retries = 5, lingerMs = Long.MaxValue, + retries = 5, lingerMs = Int.MaxValue, keySerializer = new StringSerializer, valueSerializer = new ByteArraySerializer, props = Some(propsOverride)) val bytes = new Array[Byte](msgValueLen) val futures = try { diff --git a/core/src/test/scala/unit/kafka/utils/TestUtils.scala b/core/src/test/scala/unit/kafka/utils/TestUtils.scala index 00e434f74afa8..aa902f2582b7c 100755 --- a/core/src/test/scala/unit/kafka/utils/TestUtils.scala +++ b/core/src/test/scala/unit/kafka/utils/TestUtils.scala @@ -548,8 +548,8 @@ object TestUtils extends Logging { maxBlockMs: Long = 60 * 1000L, bufferSize: Long = 1024L * 1024L, retries: Int = 0, - lingerMs: Long = 0, - requestTimeoutMs: Long = 30 * 1000L, + lingerMs: Int = 0, + requestTimeoutMs: Int = 30 * 1000, securityProtocol: SecurityProtocol = SecurityProtocol.PLAINTEXT, trustStoreFile: Option[File] = None, saslProperties: Option[Properties] = None, @@ -567,7 +567,7 @@ object TestUtils extends Logging { producerProps.put(ProducerConfig.LINGER_MS_CONFIG, lingerMs.toString) // In case of overflow set maximum possible value for deliveryTimeoutMs - val deliveryTimeoutMs = if (lingerMs + requestTimeoutMs < 0) Long.MaxValue else lingerMs + requestTimeoutMs + val deliveryTimeoutMs = if (lingerMs + requestTimeoutMs < 0) Int.MaxValue else lingerMs + requestTimeoutMs producerProps.put(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG, deliveryTimeoutMs.toString) /* Only use these if not already set */ diff --git a/docs/upgrade.html b/docs/upgrade.html index ccfab95897f2e..2ae5c2c86d04d 100644 --- a/docs/upgrade.html +++ b/docs/upgrade.html @@ -30,6 +30,10 @@

Upgrading from 0.8.x, 0.9.x, 0.1 offset retention period (or the one set by broker) has passed since their last commit.
  • The default for console consumer's enable.auto.commit property when no group.id is provided is now set to false. This is to avoid polluting the consumer coordinator cache as the auto-generated group is not likely to be used by other consumers.
    • +
  • The default value for retries was changed to INT.MAX_VALUE, as we introduced delivery.timeout.ms + for KIP-91 + that sets the upper bound of message deliver timeout. +
    • From 2c73e61cfd7c72ccd233edbec0a84942157ea430 Mon Sep 17 00:00:00 2001 From: Jason Gustafson Date: Thu, 26 Jul 2018 08:49:44 -0700 Subject: [PATCH 3/4] Remove unused method --- .../kafka/clients/producer/internals/RecordAccumulator.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java b/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java index 44078e1a2be07..964ac3c15585d 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java @@ -284,10 +284,6 @@ public void maybeUpdateNextBatchExpiryTime(ProducerBatch batch) { } } - public Set getTopicPartitions() { - return this.batches.keySet(); - } - /** * Get a list of batches which have been sitting in the accumulator too long and need to be expired. */ From b9aa6b1706e2e374c20d710567a64b0328fe3119 Mon Sep 17 00:00:00 2001 From: Jason Gustafson Date: Thu, 26 Jul 2018 08:50:22 -0700 Subject: [PATCH 4/4] Additional detail in upgrade notes --- docs/upgrade.html | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/upgrade.html b/docs/upgrade.html index 2ae5c2c86d04d..ac1388eb44005 100644 --- a/docs/upgrade.html +++ b/docs/upgrade.html @@ -30,9 +30,10 @@

    Upgrading from 0.8.x, 0.9.x, 0.1 offset retention period (or the one set by broker) has passed since their last commit.
  • The default for console consumer's enable.auto.commit property when no group.id is provided is now set to false. This is to avoid polluting the consumer coordinator cache as the auto-generated group is not likely to be used by other consumers.
    • -
  • The default value for retries was changed to INT.MAX_VALUE, as we introduced delivery.timeout.ms - for KIP-91 - that sets the upper bound of message deliver timeout. +
  • The default value for the producer's retries config was changed to Integer.MAX_VALUE, as we introduced delivery.timeout.ms + in KIP-91, + which sets an upper bound on the total time between sending a record and receiving acknowledgement from the broker. By default, + the delivery timeout is set to 2 minutes.