From d565b928be2b36bcb20a623fd44315cad125697c Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 26 Aug 2024 23:22:34 +0300 Subject: [PATCH 01/58] Refactor dispatching filtering logic - take more messages until maximum amount of messages has been picked --- .../MessageRedeliveryController.java | 32 +- ...PersistentDispatcherMultipleConsumers.java | 47 +-- ...tStickyKeyDispatcherMultipleConsumers.java | 309 ++++++++---------- .../ConcurrentBitmapSortedLongPairSet.java | 27 +- .../MessageRedeliveryControllerTest.java | 4 +- .../client/api/KeySharedSubscriptionTest.java | 2 +- 6 files changed, 217 insertions(+), 204 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java index 9d29b93ca450d..9dd3c39bddb6b 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java @@ -22,7 +22,10 @@ import java.util.ArrayList; import java.util.List; import java.util.NavigableSet; +import java.util.Optional; import java.util.Set; +import java.util.TreeSet; +import java.util.function.Predicate; import javax.annotation.concurrent.NotThreadSafe; import org.apache.bookkeeper.mledger.Position; import org.apache.bookkeeper.mledger.PositionFactory; @@ -146,8 +149,33 @@ public boolean containsStickyKeyHashes(Set stickyKeyHashes) { return false; } - public NavigableSet getMessagesToReplayNow(int maxMessagesToRead) { - return messagesToRedeliver.items(maxMessagesToRead, PositionFactory::create); + public boolean containsStickyKeyHash(int stickyKeyHash) { + return !allowOutOfOrderDelivery && hashesRefCount.containsKey(stickyKeyHash); + } + + public Optional getFirstPositionInReplay() { + NavigableSet items = messagesToRedeliver.items(1, PositionFactory::create); + return items.isEmpty() ? Optional.empty() : Optional.of(items.first()); + } + + /** + * Get the messages to replay now. + * + * @param maxMessagesToRead + * the max messages to read + * @param filter + * the filter to use to select the messages to replay + * @return the messages to replay now + */ + public NavigableSet getMessagesToReplayNow(int maxMessagesToRead, Predicate filter) { + NavigableSet items = new TreeSet<>(); + messagesToRedeliver.processItems(PositionFactory::create, item -> { + if (filter.test(item)) { + items.add(item); + } + return items.size() < maxMessagesToRead; + }); + return items; } /** diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 631a728ccce4d..8b51362bc1599 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -346,25 +346,22 @@ public synchronized void readMoreEntries() { } NavigableSet messagesToReplayNow = getMessagesToReplayNow(messagesToRead); - NavigableSet messagesToReplayFiltered = filterOutEntriesWillBeDiscarded(messagesToReplayNow); - if (!messagesToReplayFiltered.isEmpty()) { + if (!messagesToReplayNow.isEmpty()) { if (log.isDebugEnabled()) { log.debug("[{}] Schedule replay of {} messages for {} consumers", name, - messagesToReplayFiltered.size(), consumerList.size()); + messagesToReplayNow.size(), consumerList.size()); } - havePendingReplayRead = true; - minReplayedPosition = messagesToReplayNow.first(); + updateMinReplayedPosition(); Set deletedMessages = topic.isDelayedDeliveryEnabled() - ? asyncReplayEntriesInOrder(messagesToReplayFiltered) - : asyncReplayEntries(messagesToReplayFiltered); + ? asyncReplayEntriesInOrder(messagesToReplayNow) + : asyncReplayEntries(messagesToReplayNow); // clear already acked positions from replay bucket - deletedMessages.forEach(position -> redeliveryMessages.remove(position.getLedgerId(), position.getEntryId())); // if all the entries are acked-entries and cleared up from redeliveryMessages, try to read // next entries as readCompletedEntries-callback was never called - if ((messagesToReplayFiltered.size() - deletedMessages.size()) == 0) { + if ((messagesToReplayNow.size() - deletedMessages.size()) == 0) { havePendingReplayRead = false; readMoreEntriesAsync(); } @@ -386,13 +383,7 @@ public synchronized void readMoreEntries() { consumerList.size()); } havePendingRead = true; - NavigableSet toReplay = getMessagesToReplayNow(1); - if (!toReplay.isEmpty()) { - minReplayedPosition = toReplay.first(); - redeliveryMessages.add(minReplayedPosition.getLedgerId(), minReplayedPosition.getEntryId()); - } else { - minReplayedPosition = null; - } + updateMinReplayedPosition(); // Filter out and skip read delayed messages exist in DelayedDeliveryTracker if (delayedDeliveryTracker.isPresent()) { @@ -427,6 +418,15 @@ public synchronized void readMoreEntries() { } } + private void updateMinReplayedPosition() { + Optional firstPositionInReplay = getFirstPositionInReplay(); + if (firstPositionInReplay.isPresent()) { + minReplayedPosition = firstPositionInReplay.get(); + } else { + minReplayedPosition = null; + } + } + private boolean shouldPauseOnAckStatePersist(ReadType readType) { // Allows new consumers to consume redelivered messages caused by the just-closed consumer. if (readType != ReadType.Normal) { @@ -1232,16 +1232,20 @@ protected synchronized NavigableSet getMessagesToReplayNow(int maxMess delayedDeliveryTracker.get().getScheduledMessages(maxMessagesToRead); messagesAvailableNow.forEach(p -> redeliveryMessages.add(p.getLedgerId(), p.getEntryId())); } - if (!redeliveryMessages.isEmpty()) { - return redeliveryMessages.getMessagesToReplayNow(maxMessagesToRead); + return redeliveryMessages.getMessagesToReplayNow(maxMessagesToRead, createFilterForReplay()); } else { return Collections.emptyNavigableSet(); } } + protected Optional getFirstPositionInReplay() { + return redeliveryMessages.getFirstPositionInReplay(); + } + /** - * This is a mode method designed for Key_Shared mode. + * Creates a stateful filter for filtering replay positions. + * This is only used for Key_Shared mode to skip replaying certain entries. * Filter out the entries that will be discarded due to the order guarantee mechanism of Key_Shared mode. * This method is in order to avoid the scenario below: * - Get positions from the Replay queue. @@ -1249,8 +1253,9 @@ protected synchronized NavigableSet getMessagesToReplayNow(int maxMess * - The order guarantee mechanism of Key_Shared mode filtered out all the entries. * - Delivery non entry to the client, but we did a BK read. */ - protected NavigableSet filterOutEntriesWillBeDiscarded(NavigableSet src) { - return src; + protected Predicate createFilterForReplay() { + // pick all positions from the replay + return position -> true; } /** diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 97e6c943b0baa..f5b89b443c426 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -19,7 +19,6 @@ package org.apache.pulsar.broker.service.persistent; import com.google.common.annotations.VisibleForTesting; -import io.netty.util.concurrent.FastThreadLocal; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -29,11 +28,11 @@ import java.util.List; import java.util.Map; import java.util.NavigableSet; +import java.util.Optional; import java.util.Set; -import java.util.TreeSet; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicInteger; -import java.util.stream.Collectors; +import java.util.function.Predicate; import javax.annotation.Nullable; import org.apache.bookkeeper.mledger.Entry; import org.apache.bookkeeper.mledger.ManagedCursor; @@ -42,6 +41,7 @@ import org.apache.bookkeeper.mledger.impl.ManagedCursorImpl; import org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl; import org.apache.commons.collections4.MapUtils; +import org.apache.commons.lang3.mutable.MutableInt; import org.apache.pulsar.broker.ServiceConfiguration; import org.apache.pulsar.broker.service.BrokerServiceException; import org.apache.pulsar.broker.service.ConsistentHashingStickyKeyConsumerSelector; @@ -183,22 +183,6 @@ public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceE } } - private static final FastThreadLocal>> localGroupedEntries = - new FastThreadLocal>>() { - @Override - protected Map> initialValue() throws Exception { - return new HashMap<>(); - } - }; - - private static final FastThreadLocal>> localGroupedPositions = - new FastThreadLocal>>() { - @Override - protected Map> initialValue() throws Exception { - return new HashMap<>(); - } - }; - @Override protected synchronized boolean trySendMessagesToConsumers(ReadType readType, List entries) { lastNumberOfEntriesDispatched = 0; @@ -221,15 +205,9 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis if (!allowOutOfOrderDelivery) { // A corner case that we have to retry a readMoreEntries in order to preserver order delivery. // This may happen when consumer closed. See issue #12885 for details. - NavigableSet messagesToReplayNow = this.getMessagesToReplayNow(1); - if (messagesToReplayNow != null && !messagesToReplayNow.isEmpty()) { - Position replayPosition = messagesToReplayNow.first(); - - // We have received a message potentially from the delayed tracker and, since we're not using it - // right now, it needs to be added to the redelivery tracker or we won't attempt anymore to - // resend it (until we disconnect consumer). - redeliveryMessages.add(replayPosition.getLedgerId(), replayPosition.getEntryId()); - + Optional firstReplayPosition = getFirstPositionInReplay(); + if (firstReplayPosition.isPresent()) { + Position replayPosition = firstReplayPosition.get(); if (this.minReplayedPosition != null) { // If relayPosition is a new entry wither smaller position is inserted for redelivery during this // async read, it is possible that this relayPosition should dispatch to consumer first. So in @@ -274,96 +252,54 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis } } - final Map> groupedEntries = localGroupedEntries.get(); - groupedEntries.clear(); - final Map> consumerStickyKeyHashesMap = new HashMap<>(); + final Map> entriesByConsumerForDispatching = + filterAndGroupEntriesForDispatching(entries, readType); - for (Entry entry : entries) { - int stickyKeyHash = getStickyKeyHash(entry); - Consumer c = selector.select(stickyKeyHash); - if (c != null) { - groupedEntries.computeIfAbsent(c, k -> new ArrayList<>()).add(entry); - consumerStickyKeyHashesMap.computeIfAbsent(c, k -> new HashSet<>()).add(stickyKeyHash); - } else { - addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); - entry.release(); - } - } - - AtomicInteger keyNumbers = new AtomicInteger(groupedEntries.size()); - - int currentThreadKeyNumber = groupedEntries.size(); - if (currentThreadKeyNumber == 0) { - currentThreadKeyNumber = -1; - } - for (Map.Entry> current : groupedEntries.entrySet()) { + AtomicInteger remainingConsumersToFinishSending = new AtomicInteger(entriesByConsumerForDispatching.size()); + for (Map.Entry> current : entriesByConsumerForDispatching.entrySet()) { Consumer consumer = current.getKey(); - assert consumer != null; // checked when added to groupedEntries - List entriesWithSameKey = current.getValue(); - int entriesWithSameKeyCount = entriesWithSameKey.size(); - int availablePermits = getAvailablePermits(consumer); - int messagesForC = getRestrictedMaxEntriesForConsumer(consumer, - entriesWithSameKey.stream().map(Entry::getPosition).collect(Collectors.toList()), availablePermits, - readType, consumerStickyKeyHashesMap.get(consumer)); + List entriesForConsumer = current.getValue(); if (log.isDebugEnabled()) { log.debug("[{}] select consumer {} with messages num {}, read type is {}", - name, consumer.consumerName(), messagesForC, readType); + name, consumer.consumerName(), entriesForConsumer.size(), readType); } - - if (messagesForC < entriesWithSameKeyCount) { - // We are not able to push all the messages with given key to its consumer, - // so we discard for now and mark them for later redelivery - for (int i = messagesForC; i < entriesWithSameKeyCount; i++) { - Entry entry = entriesWithSameKey.get(i); - long stickyKeyHash = getStickyKeyHash(entry); - addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); - entry.release(); - entriesWithSameKey.set(i, null); + final ManagedLedgerImpl managedLedger = ((ManagedLedgerImpl) cursor.getManagedLedger()); + for (Entry entry : entriesForConsumer) { + // remove positions first from replay list first : sendMessages recycles entries + if (readType == ReadType.Replay) { + redeliveryMessages.remove(entry.getLedgerId(), entry.getEntryId()); } - } - - if (messagesForC > 0) { - final ManagedLedgerImpl managedLedger = ((ManagedLedgerImpl) cursor.getManagedLedger()); - for (int i = 0; i < messagesForC; i++) { - final Entry entry = entriesWithSameKey.get(i); - // remove positions first from replay list first : sendMessages recycles entries - if (readType == ReadType.Replay) { - redeliveryMessages.remove(entry.getLedgerId(), entry.getEntryId()); - } - // Add positions to individuallySentPositions if necessary - if (!allowOutOfOrderDelivery) { - final Position position = entry.getPosition(); - // Store to individuallySentPositions even if lastSentPosition is null - if ((lastSentPosition == null || position.compareTo(lastSentPosition) > 0) - && !individuallySentPositions.contains(position.getLedgerId(), position.getEntryId())) { - final Position previousPosition = managedLedger.getPreviousPosition(position); - individuallySentPositions.addOpenClosed(previousPosition.getLedgerId(), - previousPosition.getEntryId(), position.getLedgerId(), position.getEntryId()); - } + // Add positions to individuallySentPositions if necessary + if (!allowOutOfOrderDelivery) { + final Position position = entry.getPosition(); + // Store to individuallySentPositions even if lastSentPosition is null + if ((lastSentPosition == null || position.compareTo(lastSentPosition) > 0) + && !individuallySentPositions.contains(position.getLedgerId(), position.getEntryId())) { + final Position previousPosition = managedLedger.getPreviousPosition(position); + individuallySentPositions.addOpenClosed(previousPosition.getLedgerId(), + previousPosition.getEntryId(), position.getLedgerId(), position.getEntryId()); } } + } - SendMessageInfo sendMessageInfo = SendMessageInfo.getThreadLocal(); - EntryBatchSizes batchSizes = EntryBatchSizes.get(messagesForC); - EntryBatchIndexesAcks batchIndexesAcks = EntryBatchIndexesAcks.get(messagesForC); - totalEntries += filterEntriesForConsumer(entriesWithSameKey, batchSizes, sendMessageInfo, - batchIndexesAcks, cursor, readType == ReadType.Replay, consumer); - consumer.sendMessages(entriesWithSameKey, batchSizes, batchIndexesAcks, - sendMessageInfo.getTotalMessages(), - sendMessageInfo.getTotalBytes(), sendMessageInfo.getTotalChunkedMessages(), - getRedeliveryTracker()).addListener(future -> { - if (future.isDone() && keyNumbers.decrementAndGet() == 0) { - readMoreEntries(); - } - }); + SendMessageInfo sendMessageInfo = SendMessageInfo.getThreadLocal(); + EntryBatchSizes batchSizes = EntryBatchSizes.get(entriesForConsumer.size()); + EntryBatchIndexesAcks batchIndexesAcks = EntryBatchIndexesAcks.get(entriesForConsumer.size()); + totalEntries += filterEntriesForConsumer(entriesForConsumer, batchSizes, sendMessageInfo, + batchIndexesAcks, cursor, readType == ReadType.Replay, consumer); + consumer.sendMessages(entriesForConsumer, batchSizes, batchIndexesAcks, + sendMessageInfo.getTotalMessages(), + sendMessageInfo.getTotalBytes(), sendMessageInfo.getTotalChunkedMessages(), + getRedeliveryTracker()).addListener(future -> { + if (future.isDone() && remainingConsumersToFinishSending.decrementAndGet() == 0) { + readMoreEntries(); + } + }); - TOTAL_AVAILABLE_PERMITS_UPDATER.getAndAdd(this, - -(sendMessageInfo.getTotalMessages() - batchIndexesAcks.getTotalAckedIndexCount())); - totalMessagesSent += sendMessageInfo.getTotalMessages(); - totalBytesSent += sendMessageInfo.getTotalBytes(); - } else { - currentThreadKeyNumber = keyNumbers.decrementAndGet(); - } + TOTAL_AVAILABLE_PERMITS_UPDATER.getAndAdd(this, + -(sendMessageInfo.getTotalMessages() - batchIndexesAcks.getTotalAckedIndexCount())); + totalMessagesSent += sendMessageInfo.getTotalMessages(); + totalBytesSent += sendMessageInfo.getTotalBytes(); } // Update the last sent position and remove ranges from individuallySentPositions if necessary @@ -441,26 +377,103 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis // position. isDispatcherStuckOnReplays = true; return true; - } else if (currentThreadKeyNumber == 0) { + } else if (entriesByConsumerForDispatching.size() == 0) { return true; } return false; } - private int getRestrictedMaxEntriesForConsumer(Consumer consumer, List entries, - int availablePermits, ReadType readType, Set stickyKeyHashes) { - int maxMessages = Math.min(entries.size(), availablePermits); - if (maxMessages == 0) { - return 0; + private Map> filterAndGroupEntriesForDispatching(List entries, ReadType readType) { + Map> groupedByConsumer = new HashMap<>(); + Map availablePermitsMap = new HashMap<>(); + Set remainingEntriesFilteredForConsumer = new HashSet<>(); + for (Entry entry : entries) { + int stickyKeyHash = getStickyKeyHash(entry); + Consumer consumer = selector.select(stickyKeyHash); + MutableInt availablePermits = null; + boolean dispatchEntry = false; + if (consumer != null) { + if (remainingEntriesFilteredForConsumer.contains(consumer)) { + dispatchEntry = false; + } else { + availablePermits = + availablePermitsMap.computeIfAbsent(consumer, + k -> new MutableInt(getAvailablePermits(consumer))); + dispatchEntry = + canDispatchEntry(consumer, entry, availablePermits.intValue(), readType, + stickyKeyHash); + } + } + if (dispatchEntry) { + availablePermits.decrement(); + groupedByConsumer.computeIfAbsent(consumer, k -> new ArrayList<>()).add(entry); + } else { + // stop sending further messages to this consumer so that ordering is preserved + remainingEntriesFilteredForConsumer.add(consumer); + // If the consumer is not selected for dispatching, we need to add the message to replay + addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); + entry.release(); + } } - if (readType == ReadType.Normal && stickyKeyHashes != null - && redeliveryMessages.containsStickyKeyHashes(stickyKeyHashes)) { - // If redeliveryMessages contains messages that correspond to the same hash as the messages - // that the dispatcher is trying to send, do not send those messages for order guarantee - return 0; + return groupedByConsumer; + } + + private boolean canDispatchEntry(Consumer consumer, Entry entry, int availablePermits, + ReadType readType, int stickyKeyHash) { + if (availablePermits <= 0) { + return false; } + + Position maxLastSentPosition = resolveMaxLastSentPosition(consumer, readType); + if (maxLastSentPosition != null && entry.getPosition().compareTo(maxLastSentPosition) > 0) { + return false; + } + + // If redeliveryMessages contains messages that correspond to the same hash as the entry to be dispatched + // do not send those messages for order guarantee + if (readType == ReadType.Normal && redeliveryMessages.containsStickyKeyHash(stickyKeyHash)) { + return false; + } + + return true; + } + + @Override + protected Predicate createFilterForReplay() { + Map availablePermitsMap = new HashMap<>(); + return new Predicate() { + @Override + public boolean test(Position position) { + if (isAllowOutOfOrderDelivery()) { + return true; + } + Long stickyKeyHash = redeliveryMessages.getHash(position.getLedgerId(), position.getEntryId()); + if (stickyKeyHash == null) { + return true; + } + Consumer consumer = selector.select(stickyKeyHash.intValue()); + if (consumer == null) { + return false; + } + MutableInt availablePermits = + availablePermitsMap.computeIfAbsent(consumer, + k -> new MutableInt(getAvailablePermits(consumer))); + if (availablePermits.intValue() <= 0) { + return false; + } + Position maxLastSentPosition = resolveMaxLastSentPosition(consumer, ReadType.Replay); + if (maxLastSentPosition != null && position.compareTo(maxLastSentPosition) > 0) { + return false; + } + availablePermits.decrement(); + return true; + } + }; + } + + private Position resolveMaxLastSentPosition(Consumer consumer, ReadType readType) { if (recentlyJoinedConsumers == null) { - return maxMessages; + return null; } removeConsumersFromRecentJoinedConsumers(); Position maxLastSentPosition = recentlyJoinedConsumers.get(consumer); @@ -468,7 +481,7 @@ private int getRestrictedMaxEntriesForConsumer(Consumer consumer, List 0) { - // We have already crossed the divider line. All messages in the list are now - // newer than what we can currently dispatch to this consumer - return i; - } - } - return maxMessages; + return maxLastSentPosition; } + @Override public void markDeletePositionMoveForward() { // Execute the notification in different thread to avoid a mutex chain here @@ -576,48 +581,6 @@ private int getAvailablePermits(Consumer c) { return availablePermits; } - @Override - protected synchronized NavigableSet filterOutEntriesWillBeDiscarded(NavigableSet src) { - // The variable "hashesToBeBlocked" and "recentlyJoinedConsumers" will be null if "isAllowOutOfOrderDelivery()", - // So skip this filter out. - if (isAllowOutOfOrderDelivery()) { - return src; - } - if (src.isEmpty()) { - return src; - } - NavigableSet res = new TreeSet<>(); - // Group positions. - final Map> groupedPositions = localGroupedPositions.get(); - groupedPositions.clear(); - for (Position pos : src) { - Long stickyKeyHash = redeliveryMessages.getHash(pos.getLedgerId(), pos.getEntryId()); - if (stickyKeyHash == null) { - res.add(pos); - continue; - } - Consumer c = selector.select(stickyKeyHash.intValue()); - if (c == null) { - // Maybe using HashRangeExclusiveStickyKeyConsumerSelector. - continue; - } - groupedPositions.computeIfAbsent(c, k -> new ArrayList<>()).add(pos); - } - // Filter positions by the Recently Joined Position rule. - for (Map.Entry> item : groupedPositions.entrySet()) { - int availablePermits = getAvailablePermits(item.getKey()); - if (availablePermits == 0) { - continue; - } - int posCountToRead = getRestrictedMaxEntriesForConsumer(item.getKey(), item.getValue(), availablePermits, - ReadType.Replay, null); - if (posCountToRead > 0) { - res.addAll(item.getValue().subList(0, posCountToRead)); - } - } - return res; - } - /** * In Key_Shared mode, the consumer will not receive any entries from a normal reading if it is included in * {@link #recentlyJoinedConsumers}, they can only receive entries from replay reads. diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java b/pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java index e42cae2580b78..419793ba4720d 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java @@ -93,25 +93,42 @@ public void removeUpTo(long item1, long item2) { } } - public > NavigableSet items(int numberOfItems, LongPairSet.LongPairFunction longPairConverter) { NavigableSet items = new TreeSet<>(); + processItems(longPairConverter, item -> { + items.add(item); + return items.size() < numberOfItems; + }); + return items; + } + + public interface ItemProcessor> { + /** + * @param item + * @return false if there is no further processing required + */ + boolean process(T item); + } + + public > void processItems(LongPairSet.LongPairFunction longPairConverter, + ItemProcessor itemProcessor) { lock.readLock().lock(); try { for (Map.Entry entry : map.entrySet()) { Iterator iterator = entry.getValue().stream().iterator(); - while (iterator.hasNext() && items.size() < numberOfItems) { - items.add(longPairConverter.apply(entry.getKey(), iterator.next())); + boolean continueProcessing = true; + while (continueProcessing && iterator.hasNext()) { + T item = longPairConverter.apply(entry.getKey(), iterator.next()); + continueProcessing = itemProcessor.process(item); } - if (items.size() == numberOfItems) { + if (!continueProcessing) { break; } } } finally { lock.readLock().unlock(); } - return items; } public boolean isEmpty() { diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryControllerTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryControllerTest.java index 2222c8156e011..1708dc7bc2536 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryControllerTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryControllerTest.java @@ -225,12 +225,12 @@ public void testGetMessagesToReplayNow(boolean allowOutOfOrderDelivery) throws E if (allowOutOfOrderDelivery) { // The entries are sorted by ledger ID but not by entry ID - Position[] actual1 = controller.getMessagesToReplayNow(3).toArray(new Position[3]); + Position[] actual1 = controller.getMessagesToReplayNow(3, item -> true).toArray(new Position[3]); Position[] expected1 = { PositionFactory.create(1, 1), PositionFactory.create(1, 2), PositionFactory.create(1, 3) }; assertEqualsNoOrder(actual1, expected1); } else { // The entries are completely sorted - Set actual2 = controller.getMessagesToReplayNow(6); + Set actual2 = controller.getMessagesToReplayNow(6, item -> true); Set expected2 = new TreeSet<>(); expected2.add(PositionFactory.create(1, 1)); expected2.add(PositionFactory.create(1, 2)); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java index e8fd537831673..c122906d87111 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java @@ -1296,7 +1296,7 @@ public void testCheckBetweenSkippingAndRecentlyJoinedConsumers(boolean preSend) redeliveryMessagesField.setAccessible(true); final MessageRedeliveryController redeliveryMessages = (MessageRedeliveryController) redeliveryMessagesField.get(dispatcher); - final Set replayMsgSet = redeliveryMessages.getMessagesToReplayNow(3); + final Set replayMsgSet = redeliveryMessages.getMessagesToReplayNow(3, item -> true); assertEquals(replayMsgSet.size(), 1); final Position replayMsg = replayMsgSet.stream().findAny().get(); assertEquals(replayMsg, PositionFactory.create(msg1Id.getLedgerId(), msg1Id.getEntryId())); From a5bade975ec17464c2ec1da195a265e27bc4cb3a Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 27 Aug 2024 09:11:00 +0300 Subject: [PATCH 02/58] Limit Key_Shared look ahead --- conf/broker.conf | 26 ++++++++++ conf/standalone.conf | 26 ++++++++++ .../pulsar/broker/ServiceConfiguration.java | 42 +++++++++++++++ ...PersistentDispatcherMultipleConsumers.java | 12 ++++- ...tStickyKeyDispatcherMultipleConsumers.java | 51 ++++++++++++++----- 5 files changed, 142 insertions(+), 15 deletions(-) diff --git a/conf/broker.conf b/conf/broker.conf index ed59e5c456695..d1b500b6d3ab9 100644 --- a/conf/broker.conf +++ b/conf/broker.conf @@ -355,6 +355,32 @@ maxUnackedMessagesPerBroker=0 # limit/2 messages maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 +# For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer +# or a blocked key hash (because of ordering constraints), the broker will continue reading more +# messages from the backlog and attempt to dispatch them to consumers until the number of replay +# messages reaches the calculated threshold. +# Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer * +# connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription). +# Setting this value to 0 will disable the limit calculated per consumer. +keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer=1000 + +# For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer +# or a blocked key hash (because of ordering constraints), the broker will continue reading more +# messages from the backlog and attempt to dispatch them to consumers until the number of replay +# messages reaches the calculated threshold. +# Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer * +# connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription). +# This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. +# Setting this value to 0 will disable the limit calculated per subscription. +keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription=10000 + +# For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer +# or a blocked key hash (because of ordering constraints), the broker will continue reading more +# messages from the backlog and attempt to dispatch them to consumers until the number of replay +# messages reaches the calculated threshold. +# This setting controls whether look ahead is enabled when recently joined consumers are present. +keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist=false + # Broker periodically checks if subscription is stuck and unblock if flag is enabled. (Default is disabled) unblockStuckSubscriptionEnabled=false diff --git a/conf/standalone.conf b/conf/standalone.conf index d5d79e0383e1f..47d760b9a0efa 100644 --- a/conf/standalone.conf +++ b/conf/standalone.conf @@ -232,6 +232,32 @@ maxUnackedMessagesPerBroker=0 # limit/2 messages maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 +# For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer +# or a blocked key hash (because of ordering constraints), the broker will continue reading more +# messages from the backlog and attempt to dispatch them to consumers until the number of replay +# messages reaches the calculated threshold. +# Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer * +# connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription). +# Setting this value to 0 will disable the limit calculated per consumer. +keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer=1000 + +# For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer +# or a blocked key hash (because of ordering constraints), the broker will continue reading more +# messages from the backlog and attempt to dispatch them to consumers until the number of replay +# messages reaches the calculated threshold. +# Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer * +# connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription). +# This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. +# Setting this value to 0 will disable the limit calculated per subscription. +keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription=10000 + +# For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer +# or a blocked key hash (because of ordering constraints), the broker will continue reading more +# messages from the backlog and attempt to dispatch them to consumers until the number of replay +# messages reaches the calculated threshold. +# This setting controls whether look ahead is enabled when recently joined consumers are present. +keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist=false + # Tick time to schedule task that checks topic publish rate limiting across all topics # Reducing to lower value can give more accuracy while throttling publish but # it uses more CPU to perform frequent check. (Disable publish throttling with value 0) diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java index 60f37f52b6b8c..589878905a31b 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java @@ -949,6 +949,48 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " back and unack count reaches to `limit/2`. Using a value of 0, is disabling unackedMessage-limit" + " check and broker doesn't block dispatchers") private int maxUnackedMessagesPerBroker = 0; + + @FieldContext( + category = CATEGORY_POLICIES, + doc = "For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer" + + " or a blocked key hash (because of ordering constraints), the broker will continue reading more" + + " messages from the backlog and attempt to dispatch them to consumers until the number of replay" + + " messages reaches the calculated threshold.\n" + + "Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer *" + + " connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription)" + + ".\n" + + "Setting this value to 0 will disable the limit calculated per consumer.", + dynamic = true + ) + private int keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer = 1000; + + @FieldContext( + category = CATEGORY_POLICIES, + doc = "For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer" + + " or a blocked key hash (because of ordering constraints), the broker will continue reading more" + + " messages from the backlog and attempt to dispatch them to consumers until the number of replay" + + " messages reaches the calculated threshold.\n" + + "Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer *" + + " connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription)" + + ".\n" + + "This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist.\n" + + "Setting this value to 0 will disable the limit calculated per subscription.\n", + dynamic = true + ) + private int keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription = 10000; + + + @FieldContext( + category = CATEGORY_POLICIES, + doc = "For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer" + + " or a blocked key hash (because of ordering constraints), the broker will continue reading more" + + " messages from the backlog and attempt to dispatch them to consumers until the number of replay" + + " messages reaches the calculated threshold.\n" + + "This setting controls whether look ahead is enabled when recently joined consumers are present.", + dynamic = true + ) + private boolean keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist = false; + @FieldContext( category = CATEGORY_POLICIES, doc = "Once broker reaches maxUnackedMessagesPerBroker limit, it blocks subscriptions which has higher " diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 8b51362bc1599..a78f7d807e60b 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -345,7 +345,8 @@ public synchronized void readMoreEntries() { return; } - NavigableSet messagesToReplayNow = getMessagesToReplayNow(messagesToRead); + Set messagesToReplayNow = + canReplayMessages() ? getMessagesToReplayNow(messagesToRead) : Collections.emptySet(); if (!messagesToReplayNow.isEmpty()) { if (log.isDebugEnabled()) { log.debug("[{}] Schedule replay of {} messages for {} consumers", name, @@ -418,6 +419,15 @@ public synchronized void readMoreEntries() { } } + /** + * Controls whether replaying entries is currently enabled. + * Subclasses can override this method to temporarily disable replaying entries. + * @return true if replaying entries is currently enabled + */ + protected boolean canReplayMessages() { + return true; + } + private void updateMinReplayedPosition() { Optional firstPositionInReplay = getFirstPositionInReplay(); if (firstPositionInReplay.isPresent()) { diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index f5b89b443c426..c74cf2a6a57dc 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -20,14 +20,12 @@ import com.google.common.annotations.VisibleForTesting; import java.util.ArrayList; -import java.util.Collections; 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.NavigableSet; import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -68,7 +66,7 @@ public class PersistentStickyKeyDispatcherMultipleConsumers extends PersistentDi private final boolean allowOutOfOrderDelivery; private final StickyKeyConsumerSelector selector; - private boolean isDispatcherStuckOnReplays = false; + private boolean skipNextReplayToTriggerLookAhead = false; private final KeySharedMode keySharedMode; /** @@ -362,7 +360,7 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis // acquire message-dispatch permits for already delivered messages acquirePermitsForDeliveredMessages(topic, cursor, totalEntries, totalMessagesSent, totalBytesSent); - if (totalMessagesSent == 0 && (recentlyJoinedConsumers == null || recentlyJoinedConsumers.isEmpty())) { + if (totalMessagesSent == 0 && isLookAheadAllowed()) { // This means, that all the messages we've just read cannot be dispatched right now. // This condition can only happen when: // 1. We have consumers ready to accept messages (otherwise the would not haven been triggered) @@ -375,14 +373,30 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis // ahead in the stream while the new consumers are not ready to accept the new messages, // therefore would be most likely only increase the distance between read-position and mark-delete // position. - isDispatcherStuckOnReplays = true; + skipNextReplayToTriggerLookAhead = true; return true; } else if (entriesByConsumerForDispatching.size() == 0) { + // There should be a backoff delay before retrying return true; } return false; } + private boolean isLookAheadAllowed() { + if (serviceConfig.isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist() + || (recentlyJoinedConsumers == null || recentlyJoinedConsumers.isEmpty())) { + long keySharedNumberOfReplayMessagesThresholdForLookAhead = Math.max( + serviceConfig.getKeySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer() + * consumerList.size(), + serviceConfig.getKeySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription()); + if (keySharedNumberOfReplayMessagesThresholdForLookAhead == 0 + || redeliveryMessages.size() < keySharedNumberOfReplayMessagesThresholdForLookAhead) { + return true; + } + } + return false; + } + private Map> filterAndGroupEntriesForDispatching(List entries, ReadType readType) { Map> groupedByConsumer = new HashMap<>(); Map availablePermitsMap = new HashMap<>(); @@ -558,17 +572,26 @@ private synchronized Position updateIfNeededAndGetLastSentPosition() { return lastSentPosition; } + /** + * The dispatcher will skip replaying messages when all messages in the replay queue are filtered out when + * isDispatcherStuckOnReplays=true. The flag gets resetted after the call. + * + * If we're stuck on replay, we want to move forward reading on the topic (until the configured look ahead + * limits kick in), instead of keep replaying the same old messages, since the consumer that these + * messages are routing to might be busy at the moment. + * + * Please see {@link ServiceConfiguration#keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer}, + * {@link ServiceConfiguration#keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription} and + * {@link ServiceConfiguration#keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist} for configuring + * the limits. + */ @Override - protected synchronized NavigableSet getMessagesToReplayNow(int maxMessagesToRead) { - if (isDispatcherStuckOnReplays) { - // If we're stuck on replay, we want to move forward reading on the topic (until the overall max-unacked - // messages kicks in), instead of keep replaying the same old messages, since the consumer that these - // messages are routing to might be busy at the moment - this.isDispatcherStuckOnReplays = false; - return Collections.emptyNavigableSet(); - } else { - return super.getMessagesToReplayNow(maxMessagesToRead); + protected synchronized boolean canReplayMessages() { + if (skipNextReplayToTriggerLookAhead) { + skipNextReplayToTriggerLookAhead = false; + return false; } + return true; } private int getAvailablePermits(Consumer c) { From ff2db407c9a3715a8cfe6525c54b17982c36ef50 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 27 Aug 2024 10:42:43 +0300 Subject: [PATCH 03/58] Move KeySharedSubscriptionTest out of flaky test group --- .../org/apache/pulsar/client/api/KeySharedSubscriptionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java index c122906d87111..d10868fdd73e9 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java @@ -91,7 +91,7 @@ import org.testng.annotations.DataProvider; import org.testng.annotations.Test; -@Test(groups = "flaky") +@Test(groups = "broker-impl") public class KeySharedSubscriptionTest extends ProducerConsumerBase { private static final Logger log = LoggerFactory.getLogger(KeySharedSubscriptionTest.class); From 25e685812bd25069ee1d1915fbf89d3a5df59995 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 27 Aug 2024 11:25:04 +0300 Subject: [PATCH 04/58] Simplify method --- .../persistent/PersistentDispatcherMultipleConsumers.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index a78f7d807e60b..1d69127a3a836 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -429,12 +429,7 @@ protected boolean canReplayMessages() { } private void updateMinReplayedPosition() { - Optional firstPositionInReplay = getFirstPositionInReplay(); - if (firstPositionInReplay.isPresent()) { - minReplayedPosition = firstPositionInReplay.get(); - } else { - minReplayedPosition = null; - } + minReplayedPosition = getFirstPositionInReplay().orElse(null); } private boolean shouldPauseOnAckStatePersist(ReadType readType) { From 8e4328fd8e0af5036f323c0cbfa649b3b568a476 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 27 Aug 2024 12:28:54 +0300 Subject: [PATCH 05/58] More code comments --- ...tStickyKeyDispatcherMultipleConsumers.java | 139 ++++++++++++------ 1 file changed, 91 insertions(+), 48 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index c74cf2a6a57dc..a867de05e6038 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -397,48 +397,60 @@ private boolean isLookAheadAllowed() { return false; } + // groups the entries by consumer and filters out the entries that should not be dispatched + // the entries are handled in the order they are received instead of first grouping them by consumer and + // then filtering them private Map> filterAndGroupEntriesForDispatching(List entries, ReadType readType) { - Map> groupedByConsumer = new HashMap<>(); - Map availablePermitsMap = new HashMap<>(); + // entries grouped by consumer + Map> entriesGroupedByConsumer = new HashMap<>(); + // state of the available permits for each consumer + Map availablePermitsByConsumer = new HashMap<>(); + // consumers for which all remaining entries should be discarded Set remainingEntriesFilteredForConsumer = new HashSet<>(); + for (Entry entry : entries) { int stickyKeyHash = getStickyKeyHash(entry); Consumer consumer = selector.select(stickyKeyHash); MutableInt availablePermits = null; boolean dispatchEntry = false; - if (consumer != null) { - if (remainingEntriesFilteredForConsumer.contains(consumer)) { - dispatchEntry = false; - } else { - availablePermits = - availablePermitsMap.computeIfAbsent(consumer, - k -> new MutableInt(getAvailablePermits(consumer))); - dispatchEntry = - canDispatchEntry(consumer, entry, availablePermits.intValue(), readType, - stickyKeyHash); - } + // a consumer was found for the sticky key hash and the consumer can get more entries + if (consumer != null && !remainingEntriesFilteredForConsumer.contains(consumer)) { + // lookup the available permits for the consumer + availablePermits = + availablePermitsByConsumer.computeIfAbsent(consumer, + k -> new MutableInt(getAvailablePermits(consumer))); + // check if the entry can be dispatched to the consumer + dispatchEntry = + canDispatchEntry(consumer, entry, availablePermits.intValue(), readType, + stickyKeyHash); } if (dispatchEntry) { + // decrement the available permits for the consumer availablePermits.decrement(); - groupedByConsumer.computeIfAbsent(consumer, k -> new ArrayList<>()).add(entry); + // add the entry to consumer's entry list for dispatching + List consumerEntries = + entriesGroupedByConsumer.computeIfAbsent(consumer, k -> new ArrayList<>()); + consumerEntries.add(entry); } else { - // stop sending further messages to this consumer so that ordering is preserved - remainingEntriesFilteredForConsumer.add(consumer); - // If the consumer is not selected for dispatching, we need to add the message to replay + // add the message to replay addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); + // release the entry as it will not be dispatched entry.release(); + // stop sending further entries to this consumer so that ordering is preserved + remainingEntriesFilteredForConsumer.add(consumer); } } - return groupedByConsumer; + return entriesGroupedByConsumer; } + // checks if the entry can be dispatched to the consumer private boolean canDispatchEntry(Consumer consumer, Entry entry, int availablePermits, ReadType readType, int stickyKeyHash) { if (availablePermits <= 0) { return false; } - Position maxLastSentPosition = resolveMaxLastSentPosition(consumer, readType); + Position maxLastSentPosition = resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType); if (maxLastSentPosition != null && entry.getPosition().compareTo(maxLastSentPosition) > 0) { return false; } @@ -452,40 +464,72 @@ private boolean canDispatchEntry(Consumer consumer, Entry entry, int availablePe return true; } + /** + * Creates a filter for replaying messages. The filter is stateful and shouldn't be cached or reused. + * @see PersistentDispatcherMultipleConsumers#createFilterForReplay() + */ @Override protected Predicate createFilterForReplay() { - Map availablePermitsMap = new HashMap<>(); - return new Predicate() { - @Override - public boolean test(Position position) { - if (isAllowOutOfOrderDelivery()) { - return true; - } - Long stickyKeyHash = redeliveryMessages.getHash(position.getLedgerId(), position.getEntryId()); - if (stickyKeyHash == null) { - return true; - } - Consumer consumer = selector.select(stickyKeyHash.intValue()); - if (consumer == null) { - return false; - } - MutableInt availablePermits = - availablePermitsMap.computeIfAbsent(consumer, - k -> new MutableInt(getAvailablePermits(consumer))); - if (availablePermits.intValue() <= 0) { - return false; - } - Position maxLastSentPosition = resolveMaxLastSentPosition(consumer, ReadType.Replay); - if (maxLastSentPosition != null && position.compareTo(maxLastSentPosition) > 0) { - return false; - } - availablePermits.decrement(); + return new ReplayPositionFilter(); + } + + /** + * Filter for replaying messages. The filter is stateful for a single invocation and shouldn't be cached, shared + * or reused. This is a short-lived object, and optimizing it for the "no garbage" coding style of Pulsar is + * unnecessary since the JVM can optimize allocations for short-lived objects. + */ + private class ReplayPositionFilter implements Predicate { + // tracks the available permits for each consumer for the duration of the filter usage + // the filter is stateful and shouldn't be shared or reused later + private final Map availablePermitsMap = new HashMap<>(); + + @Override + public boolean test(Position position) { + // if out of order delivery is allowed, then any position will be replayed + if (isAllowOutOfOrderDelivery()) { return true; } - }; + // lookup the sticky key hash for the message at the replay position + Long stickyKeyHash = redeliveryMessages.getHash(position.getLedgerId(), position.getEntryId()); + if (stickyKeyHash == null) { + // the sticky key hash is missing for delayed messages, the filtering will happen at the time of + // dispatch after reading the entry from the ledger + log.debug("[{}] replay of message at position {} doesn't contain sticky key hash.", name, position); + return true; + } + // find the consumer for the sticky key hash + Consumer consumer = selector.select(stickyKeyHash.intValue()); + // skip replaying the message position if there's no assigned consumer + if (consumer == null) { + return false; + } + // lookup the available permits for the consumer + MutableInt availablePermits = + availablePermitsMap.computeIfAbsent(consumer, + k -> new MutableInt(getAvailablePermits(consumer))); + // skip replaying the message position if the consumer has no available permits + if (availablePermits.intValue() <= 0) { + return false; + } + // check if the message position can be replayed to a recently joined consumer + Position maxLastSentPosition = + resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, ReadType.Replay); + if (maxLastSentPosition != null && position.compareTo(maxLastSentPosition) > 0) { + return false; + } + // the message position can be replayed, + // decrement the available permits for the consumer which is tracked for the duration of the filter usage + availablePermits.decrement(); + return true; + } } - private Position resolveMaxLastSentPosition(Consumer consumer, ReadType readType) { + /** + * Contains the logic to resolve the max last sent position for a consumer + * when the consumer has recently joined. This is only applicable for key shared mode when + * allowOutOfOrderDelivery=false. + */ + private Position resolveMaxLastSentPositionForRecentlyJoinedConsumer(Consumer consumer, ReadType readType) { if (recentlyJoinedConsumers == null) { return null; } @@ -688,5 +732,4 @@ public Map> getConsumerKeyHashRanges() { } private static final Logger log = LoggerFactory.getLogger(PersistentStickyKeyDispatcherMultipleConsumers.class); - } From b577134ca689d0a52f735576e200a2cf6e1ca110 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 27 Aug 2024 12:55:38 +0300 Subject: [PATCH 06/58] Fix handling of permits --- ...PersistentDispatcherMultipleConsumers.java | 5 +- ...tStickyKeyDispatcherMultipleConsumers.java | 52 ++++++++++++------- 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 1d69127a3a836..5fe8ca433a6d6 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -756,8 +756,9 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis int remainingMessages = 0; boolean hasChunk = false; for (int i = 0; i < metadataArray.length; i++) { - final MessageMetadata metadata = Commands.peekAndCopyMessageMetadata( - entries.get(i).getDataBuffer(), subscription.toString(), -1); + Entry entry = entries.get(i); + MessageMetadata metadata = entry instanceof EntryAndMetadata ? ((EntryAndMetadata) entry).getMetadata() + : Commands.peekAndCopyMessageMetadata(entry.getDataBuffer(), subscription.toString(), -1); if (metadata != null) { remainingMessages += metadata.getNumMessagesInBatch(); if (!hasChunk && metadata.hasUuid()) { diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index a867de05e6038..a50fb0a5cac06 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -44,6 +44,7 @@ import org.apache.pulsar.broker.service.BrokerServiceException; import org.apache.pulsar.broker.service.ConsistentHashingStickyKeyConsumerSelector; import org.apache.pulsar.broker.service.Consumer; +import org.apache.pulsar.broker.service.EntryAndMetadata; import org.apache.pulsar.broker.service.EntryBatchIndexesAcks; import org.apache.pulsar.broker.service.EntryBatchSizes; import org.apache.pulsar.broker.service.HashRangeAutoSplitStickyKeyConsumerSelector; @@ -55,6 +56,8 @@ import org.apache.pulsar.common.api.proto.CommandSubscribe.SubType; import org.apache.pulsar.common.api.proto.KeySharedMeta; import org.apache.pulsar.common.api.proto.KeySharedMode; +import org.apache.pulsar.common.api.proto.MessageMetadata; +import org.apache.pulsar.common.protocol.Commands; import org.apache.pulsar.common.util.FutureUtil; import org.apache.pulsar.common.util.collections.ConcurrentOpenLongPairRangeSet; import org.apache.pulsar.common.util.collections.LongPairRangeSet; @@ -413,24 +416,39 @@ private Map> filterAndGroupEntriesForDispatching(List new MutableInt(getAvailablePermits(consumer))); - // check if the entry can be dispatched to the consumer - dispatchEntry = - canDispatchEntry(consumer, entry, availablePermits.intValue(), readType, - stickyKeyHash); + if (availablePermits.intValue() > 0 && canDispatchEntry(consumer, entry, readType, stickyKeyHash)) { + // parse the metadata to get the number of messages in the batch + MessageMetadata metadata = Commands + .peekAndCopyMessageMetadata(entry.getDataBuffer(), subscription.toString(), -1); + entryAndMetadata = EntryAndMetadata.create(entry, metadata); + numberOfMessages = + Math.max(metadata.hasNumMessagesInBatch() ? metadata.getNumMessagesInBatch() : 0, 1); + // check if the consumer has enough permits to dispatch the entry + // this ignores acknowledgmentAtBatchIndexLevelEnabled / enableBatchIndexAcknowledgment + // since the approximate number of messages is sufficient for filtering + dispatchEntry = availablePermits.intValue() >= numberOfMessages; + } } if (dispatchEntry) { - // decrement the available permits for the consumer - availablePermits.decrement(); // add the entry to consumer's entry list for dispatching List consumerEntries = entriesGroupedByConsumer.computeIfAbsent(consumer, k -> new ArrayList<>()); - consumerEntries.add(entry); + // decrement the number of messages from the remaining available permits for the consumer + // this ignores acknowledgmentAtBatchIndexLevelEnabled / enableBatchIndexAcknowledgment + // since the approximate number of messages is sufficient for filtering + availablePermits.add(-numberOfMessages); + // add the EntryAndMetadata instance to the consumer's entry list + // EntryAndMetadata is used to keep the entry and metadata together and avoids the need to + // parse the metadata again when the entry is dispatched + consumerEntries.add(entryAndMetadata); } else { // add the message to replay addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); @@ -444,12 +462,8 @@ private Map> filterAndGroupEntriesForDispatching(List 0) { return false; @@ -489,12 +503,12 @@ public boolean test(Position position) { if (isAllowOutOfOrderDelivery()) { return true; } - // lookup the sticky key hash for the message at the replay position + // lookup the sticky key hash for the entry at the replay position Long stickyKeyHash = redeliveryMessages.getHash(position.getLedgerId(), position.getEntryId()); if (stickyKeyHash == null) { // the sticky key hash is missing for delayed messages, the filtering will happen at the time of // dispatch after reading the entry from the ledger - log.debug("[{}] replay of message at position {} doesn't contain sticky key hash.", name, position); + log.debug("[{}] replay of entry at position {} doesn't contain sticky key hash.", name, position); return true; } // find the consumer for the sticky key hash @@ -511,15 +525,17 @@ public boolean test(Position position) { if (availablePermits.intValue() <= 0) { return false; } - // check if the message position can be replayed to a recently joined consumer + // check if the entry position can be replayed to a recently joined consumer Position maxLastSentPosition = resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, ReadType.Replay); if (maxLastSentPosition != null && position.compareTo(maxLastSentPosition) > 0) { return false; } - // the message position can be replayed, + // the position can be replayed, // decrement the available permits for the consumer which is tracked for the duration of the filter usage - availablePermits.decrement(); + // this is an approximation, the actual permits are decremented when the entry is dispatched + int requiredPermits = -Math.max(consumer.getAvgMessagesPerEntry(), 1); + availablePermits.add(requiredPermits); return true; } } From b9dad9f2165e48c4279f038897df5e528c343791 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 27 Aug 2024 15:06:56 +0300 Subject: [PATCH 07/58] Fix assertions --- .../apache/pulsar/client/impl/KeySharedSubscriptionTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeySharedSubscriptionTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeySharedSubscriptionTest.java index 1d534176e8d61..3bee66b649bce 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeySharedSubscriptionTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeySharedSubscriptionTest.java @@ -153,8 +153,8 @@ public void testCanRecoverConsumptionWhenLiftMaxUnAckedMessagesRestriction(Subsc //Determine if all messages have been received. //If the dispatcher is stuck, we can not receive enough messages. - Assert.assertEquals(pubMessages.size(), totalMsg); - Assert.assertEquals(pubMessages.size(), recMessages.size()); + Assert.assertEquals(totalMsg, pubMessages.size()); + Assert.assertEquals(recMessages.size(), pubMessages.size()); Assert.assertTrue(recMessages.containsAll(pubMessages)); // cleanup From 749d6996ac5ece0298df73e29ded871f2826fdaa Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 27 Aug 2024 15:18:15 +0300 Subject: [PATCH 08/58] Improve readability --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index a50fb0a5cac06..9cd49fdb4e5c5 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -534,8 +534,8 @@ public boolean test(Position position) { // the position can be replayed, // decrement the available permits for the consumer which is tracked for the duration of the filter usage // this is an approximation, the actual permits are decremented when the entry is dispatched - int requiredPermits = -Math.max(consumer.getAvgMessagesPerEntry(), 1); - availablePermits.add(requiredPermits); + int requiredPermits = Math.max(consumer.getAvgMessagesPerEntry(), 1); + availablePermits.add(-requiredPermits); return true; } } From d1f13a8b982a6f9a2cd0521b05c22a16607b7a49 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 27 Aug 2024 15:39:16 +0300 Subject: [PATCH 09/58] Remove permits check when entries have already been read - discarding entries at this point is a waste - it's unnecessary to be accurate with permits. The message sending will handle subtracting permits. --- ...tStickyKeyDispatcherMultipleConsumers.java | 34 ++----------------- 1 file changed, 2 insertions(+), 32 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 9cd49fdb4e5c5..4aac793f3e3f2 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -44,7 +44,6 @@ import org.apache.pulsar.broker.service.BrokerServiceException; import org.apache.pulsar.broker.service.ConsistentHashingStickyKeyConsumerSelector; import org.apache.pulsar.broker.service.Consumer; -import org.apache.pulsar.broker.service.EntryAndMetadata; import org.apache.pulsar.broker.service.EntryBatchIndexesAcks; import org.apache.pulsar.broker.service.EntryBatchSizes; import org.apache.pulsar.broker.service.HashRangeAutoSplitStickyKeyConsumerSelector; @@ -56,8 +55,6 @@ import org.apache.pulsar.common.api.proto.CommandSubscribe.SubType; import org.apache.pulsar.common.api.proto.KeySharedMeta; import org.apache.pulsar.common.api.proto.KeySharedMode; -import org.apache.pulsar.common.api.proto.MessageMetadata; -import org.apache.pulsar.common.protocol.Commands; import org.apache.pulsar.common.util.FutureUtil; import org.apache.pulsar.common.util.collections.ConcurrentOpenLongPairRangeSet; import org.apache.pulsar.common.util.collections.LongPairRangeSet; @@ -406,49 +403,22 @@ private boolean isLookAheadAllowed() { private Map> filterAndGroupEntriesForDispatching(List entries, ReadType readType) { // entries grouped by consumer Map> entriesGroupedByConsumer = new HashMap<>(); - // state of the available permits for each consumer - Map availablePermitsByConsumer = new HashMap<>(); // consumers for which all remaining entries should be discarded Set remainingEntriesFilteredForConsumer = new HashSet<>(); for (Entry entry : entries) { int stickyKeyHash = getStickyKeyHash(entry); Consumer consumer = selector.select(stickyKeyHash); - MutableInt availablePermits = null; boolean dispatchEntry = false; - EntryAndMetadata entryAndMetadata = null; - int numberOfMessages = 1; // a consumer was found for the sticky key hash and the consumer can get more entries if (consumer != null && !remainingEntriesFilteredForConsumer.contains(consumer)) { - // lookup the available permits for the consumer - availablePermits = - availablePermitsByConsumer.computeIfAbsent(consumer, - k -> new MutableInt(getAvailablePermits(consumer))); - if (availablePermits.intValue() > 0 && canDispatchEntry(consumer, entry, readType, stickyKeyHash)) { - // parse the metadata to get the number of messages in the batch - MessageMetadata metadata = Commands - .peekAndCopyMessageMetadata(entry.getDataBuffer(), subscription.toString(), -1); - entryAndMetadata = EntryAndMetadata.create(entry, metadata); - numberOfMessages = - Math.max(metadata.hasNumMessagesInBatch() ? metadata.getNumMessagesInBatch() : 0, 1); - // check if the consumer has enough permits to dispatch the entry - // this ignores acknowledgmentAtBatchIndexLevelEnabled / enableBatchIndexAcknowledgment - // since the approximate number of messages is sufficient for filtering - dispatchEntry = availablePermits.intValue() >= numberOfMessages; - } + dispatchEntry = canDispatchEntry(consumer, entry, readType, stickyKeyHash); } if (dispatchEntry) { // add the entry to consumer's entry list for dispatching List consumerEntries = entriesGroupedByConsumer.computeIfAbsent(consumer, k -> new ArrayList<>()); - // decrement the number of messages from the remaining available permits for the consumer - // this ignores acknowledgmentAtBatchIndexLevelEnabled / enableBatchIndexAcknowledgment - // since the approximate number of messages is sufficient for filtering - availablePermits.add(-numberOfMessages); - // add the EntryAndMetadata instance to the consumer's entry list - // EntryAndMetadata is used to keep the entry and metadata together and avoids the need to - // parse the metadata again when the entry is dispatched - consumerEntries.add(entryAndMetadata); + consumerEntries.add(entry); } else { // add the message to replay addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); From a263ad4ff10f1ebad82f31d77f612b7d0eb8765c Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 28 Aug 2024 08:00:17 +0300 Subject: [PATCH 10/58] Rename configuration property that uses msgInReplay concept --- conf/broker.conf | 12 ++++++------ conf/standalone.conf | 12 ++++++------ .../apache/pulsar/broker/ServiceConfiguration.java | 12 ++++++------ ...rsistentStickyKeyDispatcherMultipleConsumers.java | 8 ++++---- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/conf/broker.conf b/conf/broker.conf index d1b500b6d3ab9..03363b5799ff0 100644 --- a/conf/broker.conf +++ b/conf/broker.conf @@ -359,20 +359,20 @@ maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 # or a blocked key hash (because of ordering constraints), the broker will continue reading more # messages from the backlog and attempt to dispatch them to consumers until the number of replay # messages reaches the calculated threshold. -# Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer * -# connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription). +# Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer * +# connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # Setting this value to 0 will disable the limit calculated per consumer. -keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer=1000 +keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 # For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer # or a blocked key hash (because of ordering constraints), the broker will continue reading more # messages from the backlog and attempt to dispatch them to consumers until the number of replay # messages reaches the calculated threshold. -# Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer * -# connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription). +# Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer * +# connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. # Setting this value to 0 will disable the limit calculated per subscription. -keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription=10000 +keySharedLookAheadMsgInReplayThresholdPerSubscription=10000 # For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer # or a blocked key hash (because of ordering constraints), the broker will continue reading more diff --git a/conf/standalone.conf b/conf/standalone.conf index 47d760b9a0efa..0f35d1f90c4f1 100644 --- a/conf/standalone.conf +++ b/conf/standalone.conf @@ -236,20 +236,20 @@ maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 # or a blocked key hash (because of ordering constraints), the broker will continue reading more # messages from the backlog and attempt to dispatch them to consumers until the number of replay # messages reaches the calculated threshold. -# Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer * -# connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription). +# Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer * +# connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # Setting this value to 0 will disable the limit calculated per consumer. -keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer=1000 +keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 # For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer # or a blocked key hash (because of ordering constraints), the broker will continue reading more # messages from the backlog and attempt to dispatch them to consumers until the number of replay # messages reaches the calculated threshold. -# Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer * -# connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription). +# Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer * +# connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. # Setting this value to 0 will disable the limit calculated per subscription. -keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription=10000 +keySharedLookAheadMsgInReplayThresholdPerSubscription=10000 # For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer # or a blocked key hash (because of ordering constraints), the broker will continue reading more diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java index 589878905a31b..38b9c9472eb2a 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java @@ -956,13 +956,13 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " or a blocked key hash (because of ordering constraints), the broker will continue reading more" + " messages from the backlog and attempt to dispatch them to consumers until the number of replay" + " messages reaches the calculated threshold.\n" - + "Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer *" - + " connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription)" + + "Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer *" + + " connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription)" + ".\n" + "Setting this value to 0 will disable the limit calculated per consumer.", dynamic = true ) - private int keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer = 1000; + private int keySharedLookAheadMsgInReplayThresholdPerConsumer = 1000; @FieldContext( category = CATEGORY_POLICIES, @@ -970,14 +970,14 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " or a blocked key hash (because of ordering constraints), the broker will continue reading more" + " messages from the backlog and attempt to dispatch them to consumers until the number of replay" + " messages reaches the calculated threshold.\n" - + "Formula: threshold = max(keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer *" - + " connected consumer count, keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription)" + + "Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer *" + + " connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription)" + ".\n" + "This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist.\n" + "Setting this value to 0 will disable the limit calculated per subscription.\n", dynamic = true ) - private int keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription = 10000; + private int keySharedLookAheadMsgInReplayThresholdPerSubscription = 10000; @FieldContext( diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 4aac793f3e3f2..6b19ee4149266 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -386,9 +386,9 @@ private boolean isLookAheadAllowed() { if (serviceConfig.isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist() || (recentlyJoinedConsumers == null || recentlyJoinedConsumers.isEmpty())) { long keySharedNumberOfReplayMessagesThresholdForLookAhead = Math.max( - serviceConfig.getKeySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer() + serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerConsumer() * consumerList.size(), - serviceConfig.getKeySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription()); + serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerSubscription()); if (keySharedNumberOfReplayMessagesThresholdForLookAhead == 0 || redeliveryMessages.size() < keySharedNumberOfReplayMessagesThresholdForLookAhead) { return true; @@ -610,8 +610,8 @@ private synchronized Position updateIfNeededAndGetLastSentPosition() { * limits kick in), instead of keep replaying the same old messages, since the consumer that these * messages are routing to might be busy at the moment. * - * Please see {@link ServiceConfiguration#keySharedLookAheadNumberOfReplayMessagesThresholdPerConsumer}, - * {@link ServiceConfiguration#keySharedLookAheadNumberOfReplayMessagesThresholdPerSubscription} and + * Please see {@link ServiceConfiguration#keySharedLookAheadMsgInReplayThresholdPerConsumer}, + * {@link ServiceConfiguration#keySharedLookAheadMsgInReplayThresholdPerSubscription} and * {@link ServiceConfiguration#keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist} for configuring * the limits. */ From 5e8d5b5487ed53827a637ba5753e3bcc3fb83cad Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 30 Aug 2024 05:15:07 -0700 Subject: [PATCH 11/58] Update pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java Co-authored-by: Matteo Merli --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 6b19ee4149266..49d6ec5a12df0 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -478,7 +478,9 @@ public boolean test(Position position) { if (stickyKeyHash == null) { // the sticky key hash is missing for delayed messages, the filtering will happen at the time of // dispatch after reading the entry from the ledger - log.debug("[{}] replay of entry at position {} doesn't contain sticky key hash.", name, position); + if (log.isDebugEnabled()) { + log.debug("[{}] replay of entry at position {} doesn't contain sticky key hash.", name, position); + } return true; } // find the consumer for the sticky key hash From 01bac5d3d0b0c4bdac7fb9519511c4020a4b6a5d Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 30 Aug 2024 18:40:38 +0300 Subject: [PATCH 12/58] Take available permits into account in dispatching --- .../service/AbstractBaseDispatcher.java | 16 ++++++- .../service/DispatcherDiscardFilter.java | 43 +++++++++++++++++++ ...PersistentDispatcherMultipleConsumers.java | 30 +++++++++---- ...tStickyKeyDispatcherMultipleConsumers.java | 32 +++++++++++++- 4 files changed, 108 insertions(+), 13 deletions(-) create mode 100644 pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java index fb5c457fcc874..3f96320461097 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java @@ -23,6 +23,7 @@ import io.netty.buffer.ByteBuf; import io.prometheus.client.Gauge; import java.util.ArrayList; +import java.util.BitSet; import java.util.Collections; import java.util.List; import java.util.Map; @@ -101,7 +102,7 @@ public int filterEntriesForConsumer(List entries, EntryBatchSiz ManagedCursor cursor, boolean isReplayRead, Consumer consumer) { return filterEntriesForConsumer(null, 0, entries, batchSizes, sendMessageInfo, indexesAcks, cursor, - isReplayRead, consumer); + isReplayRead, consumer, null); } /** @@ -117,7 +118,8 @@ public int filterEntriesForConsumer(@Nullable MessageMetadata[] metadataArray, i List entries, EntryBatchSizes batchSizes, SendMessageInfo sendMessageInfo, EntryBatchIndexesAcks indexesAcks, ManagedCursor cursor, - boolean isReplayRead, Consumer consumer) { + boolean isReplayRead, Consumer consumer, + DispatcherDiscardFilter discardFilter) { int totalMessages = 0; long totalBytes = 0; int totalChunkedMessages = 0; @@ -272,6 +274,16 @@ public int filterEntriesForConsumer(@Nullable MessageMetadata[] metadataArray, i } } + + long[] finalAckSet = ackSet; + if (discardFilter != null && discardFilter.shouldDiscardEntry(consumer, entry, msgMetadata, isReplayRead, + () -> batchSize - (finalAckSet != null ? BitSet.valueOf(finalAckSet).cardinality() : 0))) { + entries.set(i, null); + indexesAcks.setIndexesAcks(i, null); + entry.release(); + continue; + } + totalEntries++; totalMessages += batchSize; totalBytes += metadataAndPayload.readableBytes(); diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java new file mode 100644 index 0000000000000..c738791d81dab --- /dev/null +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.pulsar.broker.service; + +import java.util.function.Supplier; +import org.apache.bookkeeper.mledger.Entry; +import org.apache.pulsar.common.api.proto.MessageMetadata; + +/** + * Callback to determine if an entry should be discarded by the dispatcher. + */ +public interface DispatcherDiscardFilter { + /** + * The dispatcher will release the entry and not dispatch it if this filter returns true. + * + * @param consumer The consumer that is consuming the message + * @param entry The entry to be dispatched + * @param msgMetadata The metadata of the message + * @param isReplayRead If the entry is being read during a replay + * @param unackedMessagesInEntry The number of unacked messages in the entry batch + * @return true if the entry should be discarded, false otherwise + */ + boolean shouldDiscardEntry(Consumer consumer, Entry entry, + MessageMetadata msgMetadata, boolean isReplayRead, + Supplier unackedMessagesInEntry); +} diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 5fe8ca433a6d6..6159e88e9799b 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -59,6 +59,7 @@ import org.apache.pulsar.broker.service.BrokerServiceException.ConsumerBusyException; import org.apache.pulsar.broker.service.Consumer; import org.apache.pulsar.broker.service.Dispatcher; +import org.apache.pulsar.broker.service.DispatcherDiscardFilter; import org.apache.pulsar.broker.service.EntryAndMetadata; import org.apache.pulsar.broker.service.EntryBatchIndexesAcks; import org.apache.pulsar.broker.service.EntryBatchSizes; @@ -802,11 +803,11 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis c, c.getAvailablePermits()); } - int messagesForC = Math.min(Math.min(remainingMessages, availablePermits), - serviceConfig.getDispatcherMaxRoundRobinBatchSize()); - messagesForC = Math.max(messagesForC / avgBatchSizePerMsg, 1); + int maxMessagesInThisBatch = Math.max(Math.min(Math.min(remainingMessages, availablePermits), + serviceConfig.getDispatcherMaxRoundRobinBatchSize()), 1); + int maxEntriesInThisBatch = Math.max(maxMessagesInThisBatch / avgBatchSizePerMsg, 1); - int end = Math.min(start + messagesForC, entries.size()); + int end = Math.min(start + maxEntriesInThisBatch, entries.size()); List entriesForThisConsumer = entries.subList(start, end); // remove positions first from replay list first : sendMessages recycles entries @@ -820,17 +821,20 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis EntryBatchSizes batchSizes = EntryBatchSizes.get(entriesForThisConsumer.size()); EntryBatchIndexesAcks batchIndexesAcks = EntryBatchIndexesAcks.get(entriesForThisConsumer.size()); + + DispatcherDiscardFilter discardFilter = createDispatcherDiscardFilter(maxMessagesInThisBatch); + totalEntries += filterEntriesForConsumer(metadataArray, start, entriesForThisConsumer, batchSizes, sendMessageInfo, batchIndexesAcks, cursor, - readType == ReadType.Replay, c); + readType == ReadType.Replay, c, discardFilter); c.sendMessages(entriesForThisConsumer, batchSizes, batchIndexesAcks, sendMessageInfo.getTotalMessages(), sendMessageInfo.getTotalBytes(), sendMessageInfo.getTotalChunkedMessages(), redeliveryTracker); int msgSent = sendMessageInfo.getTotalMessages(); remainingMessages -= msgSent; - start += messagesForC; - entriesToDispatch -= messagesForC; + start += maxEntriesInThisBatch; + entriesToDispatch -= maxEntriesInThisBatch; TOTAL_AVAILABLE_PERMITS_UPDATER.addAndGet(this, -(msgSent - batchIndexesAcks.getTotalAckedIndexCount())); if (log.isDebugEnabled()) { @@ -850,8 +854,7 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis entries.size() - start); } entries.subList(start, entries.size()).forEach(entry -> { - long stickyKeyHash = getStickyKeyHash(entry); - addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); + addEntryToReplay(entry); entry.release(); }); @@ -860,6 +863,15 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis return true; } + protected DispatcherDiscardFilter createDispatcherDiscardFilter(int maxMessagesInThisBatch) { + return null; + } + + protected void addEntryToReplay(Entry entry) { + long stickyKeyHash = getStickyKeyHash(entry); + addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); + } + private boolean sendChunkedMessagesToConsumers(ReadType readType, List entries, MessageMetadata[] metadataArray) { diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 49d6ec5a12df0..94f362fec5490 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -31,6 +31,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Predicate; +import java.util.function.Supplier; import javax.annotation.Nullable; import org.apache.bookkeeper.mledger.Entry; import org.apache.bookkeeper.mledger.ManagedCursor; @@ -44,6 +45,7 @@ import org.apache.pulsar.broker.service.BrokerServiceException; import org.apache.pulsar.broker.service.ConsistentHashingStickyKeyConsumerSelector; import org.apache.pulsar.broker.service.Consumer; +import org.apache.pulsar.broker.service.DispatcherDiscardFilter; import org.apache.pulsar.broker.service.EntryBatchIndexesAcks; import org.apache.pulsar.broker.service.EntryBatchSizes; import org.apache.pulsar.broker.service.HashRangeAutoSplitStickyKeyConsumerSelector; @@ -55,6 +57,7 @@ import org.apache.pulsar.common.api.proto.CommandSubscribe.SubType; import org.apache.pulsar.common.api.proto.KeySharedMeta; import org.apache.pulsar.common.api.proto.KeySharedMode; +import org.apache.pulsar.common.api.proto.MessageMetadata; import org.apache.pulsar.common.util.FutureUtil; import org.apache.pulsar.common.util.collections.ConcurrentOpenLongPairRangeSet; import org.apache.pulsar.common.util.collections.LongPairRangeSet; @@ -283,8 +286,9 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis SendMessageInfo sendMessageInfo = SendMessageInfo.getThreadLocal(); EntryBatchSizes batchSizes = EntryBatchSizes.get(entriesForConsumer.size()); EntryBatchIndexesAcks batchIndexesAcks = EntryBatchIndexesAcks.get(entriesForConsumer.size()); - totalEntries += filterEntriesForConsumer(entriesForConsumer, batchSizes, sendMessageInfo, - batchIndexesAcks, cursor, readType == ReadType.Replay, consumer); + DispatcherDiscardFilter discardFilter = createDispatcherDiscardFilter(getAvailablePermits(consumer)); + totalEntries += filterEntriesForConsumer(null, 0, entriesForConsumer, batchSizes, sendMessageInfo, + batchIndexesAcks, cursor, readType == ReadType.Replay, consumer, discardFilter); consumer.sendMessages(entriesForConsumer, batchSizes, batchIndexesAcks, sendMessageInfo.getTotalMessages(), sendMessageInfo.getTotalBytes(), sendMessageInfo.getTotalChunkedMessages(), @@ -636,6 +640,30 @@ private int getAvailablePermits(Consumer c) { return availablePermits; } + + @Override + protected DispatcherDiscardFilter createDispatcherDiscardFilter(int maxMessagesInThisBatch) { + DispatcherDiscardFilter discardFilter = new DispatcherDiscardFilter() { + int remainingMessagePermits = maxMessagesInThisBatch; + @Override + public boolean shouldDiscardEntry(Consumer consumer, Entry entry, MessageMetadata msgMetadata, + boolean isReplayRead, Supplier unackedMessagesInEntry) { + if (remainingMessagePermits <= 0) { + addEntryToReplay(entry); + return true; + } + int requiredPermits = unackedMessagesInEntry.get(); + if (requiredPermits > remainingMessagePermits) { + addEntryToReplay(entry); + return true; + } + remainingMessagePermits -= requiredPermits; + return false; + } + }; + return discardFilter; + } + /** * In Key_Shared mode, the consumer will not receive any entries from a normal reading if it is included in * {@link #recentlyJoinedConsumers}, they can only receive entries from replay reads. From b4ca8e886e48633372c942e1744b4acec3e2365d Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 30 Aug 2024 19:26:22 +0300 Subject: [PATCH 13/58] Add basic initial filtering --- ...PersistentStickyKeyDispatcherMultipleConsumers.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 94f362fec5490..5d867faf38a61 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -409,6 +409,8 @@ private Map> filterAndGroupEntriesForDispatching(List> entriesGroupedByConsumer = new HashMap<>(); // consumers for which all remaining entries should be discarded Set remainingEntriesFilteredForConsumer = new HashSet<>(); + // permits for consumer, won't be adjusted with batch message counts at initial filtering + Map permitsForConsumer = new HashMap<>(); for (Entry entry : entries) { int stickyKeyHash = getStickyKeyHash(entry); @@ -417,6 +419,14 @@ private Map> filterAndGroupEntriesForDispatching(List new MutableInt(getAvailablePermits(consumer))); + if (permits.intValue() > 0) { + // use 1 permit per batch at this stage to avoid parsing the message metadata here + permits.decrement(); + } else { + dispatchEntry = false; + } } if (dispatchEntry) { // add the entry to consumer's entry list for dispatching From 88a05d2eb048f566ec0d5fb400e5ec7866f8d91a Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 08:10:48 -0700 Subject: [PATCH 14/58] Update field name in javadoc Co-authored-by: Yuri Mizushima --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 5d867faf38a61..9d49a4d57e7ef 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -620,7 +620,7 @@ private synchronized Position updateIfNeededAndGetLastSentPosition() { /** * The dispatcher will skip replaying messages when all messages in the replay queue are filtered out when - * isDispatcherStuckOnReplays=true. The flag gets resetted after the call. + * skipNextReplayToTriggerLookAhead=true. The flag gets resetted after the call. * * If we're stuck on replay, we want to move forward reading on the topic (until the configured look ahead * limits kick in), instead of keep replaying the same old messages, since the consumer that these From dd1eca5e7f43a9323d4176b10fbaa5f6b6b053d7 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 20:21:16 +0300 Subject: [PATCH 15/58] Permits are for entries/batches, not individual messages --- ...tentStickyKeyDispatcherMultipleConsumers.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 9d49a4d57e7ef..25b6342597d7b 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -422,7 +422,6 @@ private Map> filterAndGroupEntriesForDispatching(List new MutableInt(getAvailablePermits(consumer))); if (permits.intValue() > 0) { - // use 1 permit per batch at this stage to avoid parsing the message metadata here permits.decrement(); } else { dispatchEntry = false; @@ -517,11 +516,7 @@ public boolean test(Position position) { if (maxLastSentPosition != null && position.compareTo(maxLastSentPosition) > 0) { return false; } - // the position can be replayed, - // decrement the available permits for the consumer which is tracked for the duration of the filter usage - // this is an approximation, the actual permits are decremented when the entry is dispatched - int requiredPermits = Math.max(consumer.getAvgMessagesPerEntry(), 1); - availablePermits.add(-requiredPermits); + availablePermits.decrement(); return true; } } @@ -643,9 +638,12 @@ protected synchronized boolean canReplayMessages() { private int getAvailablePermits(Consumer c) { int availablePermits = Math.max(c.getAvailablePermits(), 0); if (c.getMaxUnackedMessages() > 0) { - // Avoid negative number - int remainUnAckedMessages = Math.max(c.getMaxUnackedMessages() - c.getUnackedMessages(), 0); - availablePermits = Math.min(availablePermits, remainUnAckedMessages); + // Calculate the maximum number of additional unacked messages allowed + int maxAdditionalUnackedMessages = Math.max(c.getMaxUnackedMessages() - c.getUnackedMessages(), 0); + // Estimate the remaining permits based on the average messages per entry + int estimatedRemainingPermits = maxAdditionalUnackedMessages / Math.max(c.getAvgMessagesPerEntry(), 1); + // Update available permits to be the minimum of current available permits and estimated remaining permits + availablePermits = Math.min(availablePermits, estimatedRemainingPermits); } return availablePermits; } From b61da7e7ba7cce06dd81eeb358d6f53fa1567e51 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 21:16:49 +0300 Subject: [PATCH 16/58] Remove message level handling related to permits since permits are at entry/batch level --- .../broker/service/AbstractBaseDispatcher.java | 6 +----- .../broker/service/DispatcherDiscardFilter.java | 5 +---- .../PersistentDispatcherMultipleConsumers.java | 2 +- ...tentStickyKeyDispatcherMultipleConsumers.java | 16 +++++----------- 4 files changed, 8 insertions(+), 21 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java index 3f96320461097..4251de5c99897 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java @@ -23,7 +23,6 @@ import io.netty.buffer.ByteBuf; import io.prometheus.client.Gauge; import java.util.ArrayList; -import java.util.BitSet; import java.util.Collections; import java.util.List; import java.util.Map; @@ -274,10 +273,7 @@ public int filterEntriesForConsumer(@Nullable MessageMetadata[] metadataArray, i } } - - long[] finalAckSet = ackSet; - if (discardFilter != null && discardFilter.shouldDiscardEntry(consumer, entry, msgMetadata, isReplayRead, - () -> batchSize - (finalAckSet != null ? BitSet.valueOf(finalAckSet).cardinality() : 0))) { + if (discardFilter != null && discardFilter.shouldDiscardEntry(consumer, entry, msgMetadata, isReplayRead)) { entries.set(i, null); indexesAcks.setIndexesAcks(i, null); entry.release(); diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java index c738791d81dab..cd61c040f8552 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java @@ -19,7 +19,6 @@ package org.apache.pulsar.broker.service; -import java.util.function.Supplier; import org.apache.bookkeeper.mledger.Entry; import org.apache.pulsar.common.api.proto.MessageMetadata; @@ -34,10 +33,8 @@ public interface DispatcherDiscardFilter { * @param entry The entry to be dispatched * @param msgMetadata The metadata of the message * @param isReplayRead If the entry is being read during a replay - * @param unackedMessagesInEntry The number of unacked messages in the entry batch * @return true if the entry should be discarded, false otherwise */ boolean shouldDiscardEntry(Consumer consumer, Entry entry, - MessageMetadata msgMetadata, boolean isReplayRead, - Supplier unackedMessagesInEntry); + MessageMetadata msgMetadata, boolean isReplayRead); } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 6159e88e9799b..0b97d1bb315a7 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -863,7 +863,7 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis return true; } - protected DispatcherDiscardFilter createDispatcherDiscardFilter(int maxMessagesInThisBatch) { + protected DispatcherDiscardFilter createDispatcherDiscardFilter(int availablePermits) { return null; } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 25b6342597d7b..11d184209da11 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -31,7 +31,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Predicate; -import java.util.function.Supplier; import javax.annotation.Nullable; import org.apache.bookkeeper.mledger.Entry; import org.apache.bookkeeper.mledger.ManagedCursor; @@ -650,22 +649,17 @@ private int getAvailablePermits(Consumer c) { @Override - protected DispatcherDiscardFilter createDispatcherDiscardFilter(int maxMessagesInThisBatch) { + protected DispatcherDiscardFilter createDispatcherDiscardFilter(int availablePermits) { DispatcherDiscardFilter discardFilter = new DispatcherDiscardFilter() { - int remainingMessagePermits = maxMessagesInThisBatch; + int remainingPermits = availablePermits; @Override public boolean shouldDiscardEntry(Consumer consumer, Entry entry, MessageMetadata msgMetadata, - boolean isReplayRead, Supplier unackedMessagesInEntry) { - if (remainingMessagePermits <= 0) { + boolean isReplayRead) { + if (remainingPermits <= 0) { addEntryToReplay(entry); return true; } - int requiredPermits = unackedMessagesInEntry.get(); - if (requiredPermits > remainingMessagePermits) { - addEntryToReplay(entry); - return true; - } - remainingMessagePermits -= requiredPermits; + remainingPermits--; return false; } }; From d344a47c7c2365bd0959c97bd024814fb6af6885 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 21:25:58 +0300 Subject: [PATCH 17/58] Remove DispatcherDiscardFilter from PersistentDispatcherMultipleConsumers since it's not used there --- .../PersistentDispatcherMultipleConsumers.java | 9 +-------- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 3 +-- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 0b97d1bb315a7..4f756b5b6b49a 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -59,7 +59,6 @@ import org.apache.pulsar.broker.service.BrokerServiceException.ConsumerBusyException; import org.apache.pulsar.broker.service.Consumer; import org.apache.pulsar.broker.service.Dispatcher; -import org.apache.pulsar.broker.service.DispatcherDiscardFilter; import org.apache.pulsar.broker.service.EntryAndMetadata; import org.apache.pulsar.broker.service.EntryBatchIndexesAcks; import org.apache.pulsar.broker.service.EntryBatchSizes; @@ -822,11 +821,9 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis EntryBatchSizes batchSizes = EntryBatchSizes.get(entriesForThisConsumer.size()); EntryBatchIndexesAcks batchIndexesAcks = EntryBatchIndexesAcks.get(entriesForThisConsumer.size()); - DispatcherDiscardFilter discardFilter = createDispatcherDiscardFilter(maxMessagesInThisBatch); - totalEntries += filterEntriesForConsumer(metadataArray, start, entriesForThisConsumer, batchSizes, sendMessageInfo, batchIndexesAcks, cursor, - readType == ReadType.Replay, c, discardFilter); + readType == ReadType.Replay, c, null); c.sendMessages(entriesForThisConsumer, batchSizes, batchIndexesAcks, sendMessageInfo.getTotalMessages(), sendMessageInfo.getTotalBytes(), sendMessageInfo.getTotalChunkedMessages(), redeliveryTracker); @@ -863,10 +860,6 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis return true; } - protected DispatcherDiscardFilter createDispatcherDiscardFilter(int availablePermits) { - return null; - } - protected void addEntryToReplay(Entry entry) { long stickyKeyHash = getStickyKeyHash(entry); addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 11d184209da11..a1d0266cd9b2c 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -648,8 +648,7 @@ private int getAvailablePermits(Consumer c) { } - @Override - protected DispatcherDiscardFilter createDispatcherDiscardFilter(int availablePermits) { + private DispatcherDiscardFilter createDispatcherDiscardFilter(int availablePermits) { DispatcherDiscardFilter discardFilter = new DispatcherDiscardFilter() { int remainingPermits = availablePermits; @Override From cfbd935c1cf2ff3ccd0e3fcfb62d9d9816467253 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 21:32:13 +0300 Subject: [PATCH 18/58] Use getAvailablePermits method to check if consumer has permits --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index a1d0266cd9b2c..db90a135ded6d 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -686,7 +686,7 @@ protected boolean hasConsumersNeededNormalRead() { if (recentlyJoinedConsumers.containsKey(consumer)) { continue; } - if (consumer.getAvailablePermits() > 0) { + if (getAvailablePermits(consumer) > 0) { return true; } } From 8a4b4c44dcad4df3719a0324da30f3b1e029de27 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 21:35:15 +0300 Subject: [PATCH 19/58] Add isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist handling in the extra check --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index db90a135ded6d..303123a07ccc1 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -683,7 +683,10 @@ protected boolean hasConsumersNeededNormalRead() { if (consumer == null || consumer.isBlocked()) { continue; } - if (recentlyJoinedConsumers.containsKey(consumer)) { + // skip the consumer if isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist is false + // and consumer exists in the recently joined consumer + if (!serviceConfig.isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist() + && recentlyJoinedConsumers.containsKey(consumer)) { continue; } if (getAvailablePermits(consumer) > 0) { From 1cf36e63a165ca5059ad2c08a76e605a139d6477 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 21:43:37 +0300 Subject: [PATCH 20/58] Rename "hasConsumersNeededNormalRead" to "isNormalReadAllowed" --- .../persistent/PersistentDispatcherMultipleConsumers.java | 6 +++--- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 7 ++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 4f756b5b6b49a..e18ade5227ff7 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -371,7 +371,7 @@ public synchronized void readMoreEntries() { log.debug("[{}] Dispatcher read is blocked due to unackMessages {} reached to max {}", name, totalUnackedMessages, topic.getMaxUnackedMessagesOnSubscription()); } - } else if (!havePendingRead && hasConsumersNeededNormalRead()) { + } else if (!havePendingRead && isNormalReadAllowed()) { if (shouldPauseOnAckStatePersist(ReadType.Normal)) { if (log.isDebugEnabled()) { log.debug("[{}] [{}] Skipping read for the topic, Due to blocked on ack state persistent.", @@ -1271,9 +1271,9 @@ protected Predicate createFilterForReplay() { /** * This is a mode method designed for Key_Shared mode, to avoid unnecessary stuck. - * See detail {@link PersistentStickyKeyDispatcherMultipleConsumers#hasConsumersNeededNormalRead}. + * See detail {@link PersistentStickyKeyDispatcherMultipleConsumers#isNormalReadAllowed}. */ - protected boolean hasConsumersNeededNormalRead() { + protected boolean isNormalReadAllowed() { return true; } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 303123a07ccc1..0b60e94bf7487 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -673,13 +673,13 @@ public boolean shouldDiscardEntry(Consumer consumer, Entry entry, MessageMetadat * stuck. See https://github.com/apache/pulsar/pull/7105. */ @Override - protected boolean hasConsumersNeededNormalRead() { - // The variable "hashesToBeBlocked" and "recentlyJoinedConsumers" will be null if "isAllowOutOfOrderDelivery()", - // So the method "filterOutEntriesWillBeDiscarded" will filter out nothing, just return "true" here. + protected boolean isNormalReadAllowed() { + // Always allow ordinary reads if out-of-order delivery is enabled if (isAllowOutOfOrderDelivery()) { return true; } for (Consumer consumer : consumerList) { + // skip blocked consumers if (consumer == null || consumer.isBlocked()) { continue; } @@ -689,6 +689,7 @@ protected boolean hasConsumersNeededNormalRead() { && recentlyJoinedConsumers.containsKey(consumer)) { continue; } + // before reading more, check that there's at least one consumer that has permits if (getAvailablePermits(consumer) > 0) { return true; } From c60d29ffec56df3c05a5ab718ffc9ed34f5f7104 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 21:43:52 +0300 Subject: [PATCH 21/58] Currently closing consumers shouldn't have permits for dispatching --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 0b60e94bf7487..56102851b11fc 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -635,6 +635,10 @@ protected synchronized boolean canReplayMessages() { } private int getAvailablePermits(Consumer c) { + // skip consumers that are currently closing + if (!c.cnx().isActive()) { + return 0; + } int availablePermits = Math.max(c.getAvailablePermits(), 0); if (c.getMaxUnackedMessages() > 0) { // Calculate the maximum number of additional unacked messages allowed From 3b6eb0da6f0de7d2e6e6fd00c1f6c3ed5d7d0948 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 22:18:20 +0300 Subject: [PATCH 22/58] Add utility methods for logging topic stats and internal stats in tests --- .../apache/pulsar/broker/BrokerTestUtil.java | 42 +++++++++++++++++++ .../auth/MockedPulsarServiceBaseTest.java | 5 +++ 2 files changed, 47 insertions(+) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java index bfb172d0711d4..79b7368b6b90c 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java @@ -18,8 +18,17 @@ */ package org.apache.pulsar.broker; +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.ObjectWriter; +import java.io.IOException; +import java.io.StringWriter; +import java.io.UncheckedIOException; import java.util.UUID; +import org.apache.pulsar.client.admin.PulsarAdmin; +import org.apache.pulsar.client.admin.PulsarAdminException; +import org.apache.pulsar.common.util.ObjectMapperFactory; import org.mockito.Mockito; +import org.slf4j.Logger; /** * Holds util methods used in test. @@ -77,4 +86,37 @@ public static T spyWithoutRecordingInvocations(T object) { .defaultAnswer(Mockito.CALLS_REAL_METHODS) .stubOnly()); } + + /** + * Uses Jackson to create a JSON string for the given object + * @param object to convert to JSON + * @return JSON string + */ + public static String toJson(Object object) { + ObjectWriter writer = ObjectMapperFactory.getMapper().writer(); + StringWriter stringWriter = new StringWriter(); + try (JsonGenerator generator = writer.createGenerator(stringWriter).useDefaultPrettyPrinter()) { + generator.writeObject(object); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + return stringWriter.toString(); + } + + /** + * Logs the topic stats and internal stats for the given topic + * @param logger logger to use + * @param pulsarAdmin PulsarAdmin client to use + * @param topic topic name + */ + public static void logTopicStats(Logger logger, PulsarAdmin pulsarAdmin, String topic) { + try { + logger.info("[{}] stats: {}", topic, toJson(pulsarAdmin.topics().getStats(topic))); + logger.info("[{}] internalStats: {}", topic, + toJson(pulsarAdmin.topics().getInternalStats(topic, true))); + } catch (PulsarAdminException e) { + logger.warn("Failed to get stats for topic {}", topic, e); + } + } + } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java index c83888b8022b3..8dd2fc1c3c26d 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java @@ -45,6 +45,7 @@ import javax.ws.rs.container.TimeoutHandler; import lombok.AllArgsConstructor; import lombok.Data; +import org.apache.pulsar.broker.BrokerTestUtil; import org.apache.pulsar.broker.PulsarService; import org.apache.pulsar.broker.ServiceConfiguration; import org.apache.pulsar.broker.authentication.AuthenticationProviderTls; @@ -759,5 +760,9 @@ protected void assertOtelMetricLongSumValue(String metricName, int value) { sum -> sum.hasPointsSatisfying(point -> point.hasValue(value)))); } + protected void logTopicStats(String topic) { + BrokerTestUtil.logTopicStats(log, admin, topic); + } + private static final Logger log = LoggerFactory.getLogger(MockedPulsarServiceBaseTest.class); } From 9a572d2258a7d43e6882d11f412d72a0c7905737 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 22:18:39 +0300 Subject: [PATCH 23/58] Log topic stats in tests for troubleshooting --- .../apache/pulsar/client/impl/KeySharedSubscriptionTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeySharedSubscriptionTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeySharedSubscriptionTest.java index 3bee66b649bce..7889b19e5b29e 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeySharedSubscriptionTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeySharedSubscriptionTest.java @@ -151,6 +151,8 @@ public void testCanRecoverConsumptionWhenLiftMaxUnAckedMessagesRestriction(Subsc .until(() -> (System.currentTimeMillis() - lastActiveTime.get()) > TimeUnit.SECONDS.toMillis(5)); + logTopicStats(topic); + //Determine if all messages have been received. //If the dispatcher is stuck, we can not receive enough messages. Assert.assertEquals(totalMsg, pubMessages.size()); From 648112f8aebd1224678e927a808aab8c2ad4449e Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 22:40:04 +0300 Subject: [PATCH 24/58] Fix getAvailablePermits calculation for key_shared --- ...tStickyKeyDispatcherMultipleConsumers.java | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 56102851b11fc..a8393f118c267 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -640,15 +640,24 @@ private int getAvailablePermits(Consumer c) { return 0; } int availablePermits = Math.max(c.getAvailablePermits(), 0); - if (c.getMaxUnackedMessages() > 0) { + if (availablePermits > 0 && c.getMaxUnackedMessages() > 0) { // Calculate the maximum number of additional unacked messages allowed int maxAdditionalUnackedMessages = Math.max(c.getMaxUnackedMessages() - c.getUnackedMessages(), 0); + if (maxAdditionalUnackedMessages == 0) { + // if the consumer has reached the max unacked messages, then no more messages can be dispatched + return 0; + } // Estimate the remaining permits based on the average messages per entry - int estimatedRemainingPermits = maxAdditionalUnackedMessages / Math.max(c.getAvgMessagesPerEntry(), 1); - // Update available permits to be the minimum of current available permits and estimated remaining permits - availablePermits = Math.min(availablePermits, estimatedRemainingPermits); + // add "avgMessagesPerEntry - 1" to round up the division to the next integer without the need to use + // floating point arithmetic + int avgMessagesPerEntry = Math.max(c.getAvgMessagesPerEntry(), 1); + int estimatedRemainingPermits = + (maxAdditionalUnackedMessages + avgMessagesPerEntry - 1) / avgMessagesPerEntry; + // return the minimum of current available permits and estimated remaining permits + return Math.min(availablePermits, estimatedRemainingPermits); + } else { + return availablePermits; } - return availablePermits; } From 898e715d7bd294e05e09c8ff22c036e2a71c6d18 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 22:51:07 +0300 Subject: [PATCH 25/58] Update comment about permits --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index a8393f118c267..c4eb925e3576b 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -408,7 +408,7 @@ private Map> filterAndGroupEntriesForDispatching(List> entriesGroupedByConsumer = new HashMap<>(); // consumers for which all remaining entries should be discarded Set remainingEntriesFilteredForConsumer = new HashSet<>(); - // permits for consumer, won't be adjusted with batch message counts at initial filtering + // permits for consumer, permits are for entries/batches Map permitsForConsumer = new HashMap<>(); for (Entry entry : entries) { From abfef0759bc57f59f10d320d33209171e6cec446 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 22:51:44 +0300 Subject: [PATCH 26/58] Drop unnecessary strict rule for stopping dispatching to a specific consumer --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index c4eb925e3576b..a141a3fbff247 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -21,7 +21,6 @@ import com.google.common.annotations.VisibleForTesting; import java.util.ArrayList; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; @@ -406,8 +405,6 @@ private boolean isLookAheadAllowed() { private Map> filterAndGroupEntriesForDispatching(List entries, ReadType readType) { // entries grouped by consumer Map> entriesGroupedByConsumer = new HashMap<>(); - // consumers for which all remaining entries should be discarded - Set remainingEntriesFilteredForConsumer = new HashSet<>(); // permits for consumer, permits are for entries/batches Map permitsForConsumer = new HashMap<>(); @@ -416,7 +413,7 @@ private Map> filterAndGroupEntriesForDispatching(List new MutableInt(getAvailablePermits(consumer))); @@ -436,8 +433,6 @@ private Map> filterAndGroupEntriesForDispatching(List Date: Mon, 2 Sep 2024 22:56:25 +0300 Subject: [PATCH 27/58] Simplify filtering logic --- ...PersistentStickyKeyDispatcherMultipleConsumers.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index a141a3fbff247..ac8eeb3a27d47 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -412,15 +412,15 @@ private Map> filterAndGroupEntriesForDispatching(List new MutableInt(getAvailablePermits(consumer))); if (permits.intValue() > 0) { + // decrement the permits for the consumer permits.decrement(); - } else { - dispatchEntry = false; + // allow the entry to be dispatched + dispatchEntry = true; } } if (dispatchEntry) { From d51701c6e744563e85c2af8cdddf379c9d5a4ced Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 23:00:32 +0300 Subject: [PATCH 28/58] Remove the unnecessary DispatcherDiscardFilter --- .../service/AbstractBaseDispatcher.java | 12 +----- .../service/DispatcherDiscardFilter.java | 40 ------------------- ...PersistentDispatcherMultipleConsumers.java | 2 +- ...tStickyKeyDispatcherMultipleConsumers.java | 23 +---------- 4 files changed, 4 insertions(+), 73 deletions(-) delete mode 100644 pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java index 4251de5c99897..fb5c457fcc874 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java @@ -101,7 +101,7 @@ public int filterEntriesForConsumer(List entries, EntryBatchSiz ManagedCursor cursor, boolean isReplayRead, Consumer consumer) { return filterEntriesForConsumer(null, 0, entries, batchSizes, sendMessageInfo, indexesAcks, cursor, - isReplayRead, consumer, null); + isReplayRead, consumer); } /** @@ -117,8 +117,7 @@ public int filterEntriesForConsumer(@Nullable MessageMetadata[] metadataArray, i List entries, EntryBatchSizes batchSizes, SendMessageInfo sendMessageInfo, EntryBatchIndexesAcks indexesAcks, ManagedCursor cursor, - boolean isReplayRead, Consumer consumer, - DispatcherDiscardFilter discardFilter) { + boolean isReplayRead, Consumer consumer) { int totalMessages = 0; long totalBytes = 0; int totalChunkedMessages = 0; @@ -273,13 +272,6 @@ public int filterEntriesForConsumer(@Nullable MessageMetadata[] metadataArray, i } } - if (discardFilter != null && discardFilter.shouldDiscardEntry(consumer, entry, msgMetadata, isReplayRead)) { - entries.set(i, null); - indexesAcks.setIndexesAcks(i, null); - entry.release(); - continue; - } - totalEntries++; totalMessages += batchSize; totalBytes += metadataAndPayload.readableBytes(); diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java deleted file mode 100644 index cd61c040f8552..0000000000000 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/DispatcherDiscardFilter.java +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.pulsar.broker.service; - -import org.apache.bookkeeper.mledger.Entry; -import org.apache.pulsar.common.api.proto.MessageMetadata; - -/** - * Callback to determine if an entry should be discarded by the dispatcher. - */ -public interface DispatcherDiscardFilter { - /** - * The dispatcher will release the entry and not dispatch it if this filter returns true. - * - * @param consumer The consumer that is consuming the message - * @param entry The entry to be dispatched - * @param msgMetadata The metadata of the message - * @param isReplayRead If the entry is being read during a replay - * @return true if the entry should be discarded, false otherwise - */ - boolean shouldDiscardEntry(Consumer consumer, Entry entry, - MessageMetadata msgMetadata, boolean isReplayRead); -} diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index e18ade5227ff7..5269238b4e6b5 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -823,7 +823,7 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis totalEntries += filterEntriesForConsumer(metadataArray, start, entriesForThisConsumer, batchSizes, sendMessageInfo, batchIndexesAcks, cursor, - readType == ReadType.Replay, c, null); + readType == ReadType.Replay, c); c.sendMessages(entriesForThisConsumer, batchSizes, batchIndexesAcks, sendMessageInfo.getTotalMessages(), sendMessageInfo.getTotalBytes(), sendMessageInfo.getTotalChunkedMessages(), redeliveryTracker); diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index ac8eeb3a27d47..632685b120215 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -43,7 +43,6 @@ import org.apache.pulsar.broker.service.BrokerServiceException; import org.apache.pulsar.broker.service.ConsistentHashingStickyKeyConsumerSelector; import org.apache.pulsar.broker.service.Consumer; -import org.apache.pulsar.broker.service.DispatcherDiscardFilter; import org.apache.pulsar.broker.service.EntryBatchIndexesAcks; import org.apache.pulsar.broker.service.EntryBatchSizes; import org.apache.pulsar.broker.service.HashRangeAutoSplitStickyKeyConsumerSelector; @@ -55,7 +54,6 @@ import org.apache.pulsar.common.api.proto.CommandSubscribe.SubType; import org.apache.pulsar.common.api.proto.KeySharedMeta; import org.apache.pulsar.common.api.proto.KeySharedMode; -import org.apache.pulsar.common.api.proto.MessageMetadata; import org.apache.pulsar.common.util.FutureUtil; import org.apache.pulsar.common.util.collections.ConcurrentOpenLongPairRangeSet; import org.apache.pulsar.common.util.collections.LongPairRangeSet; @@ -284,9 +282,8 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis SendMessageInfo sendMessageInfo = SendMessageInfo.getThreadLocal(); EntryBatchSizes batchSizes = EntryBatchSizes.get(entriesForConsumer.size()); EntryBatchIndexesAcks batchIndexesAcks = EntryBatchIndexesAcks.get(entriesForConsumer.size()); - DispatcherDiscardFilter discardFilter = createDispatcherDiscardFilter(getAvailablePermits(consumer)); totalEntries += filterEntriesForConsumer(null, 0, entriesForConsumer, batchSizes, sendMessageInfo, - batchIndexesAcks, cursor, readType == ReadType.Replay, consumer, discardFilter); + batchIndexesAcks, cursor, readType == ReadType.Replay, consumer); consumer.sendMessages(entriesForConsumer, batchSizes, batchIndexesAcks, sendMessageInfo.getTotalMessages(), sendMessageInfo.getTotalBytes(), sendMessageInfo.getTotalChunkedMessages(), @@ -655,24 +652,6 @@ private int getAvailablePermits(Consumer c) { } } - - private DispatcherDiscardFilter createDispatcherDiscardFilter(int availablePermits) { - DispatcherDiscardFilter discardFilter = new DispatcherDiscardFilter() { - int remainingPermits = availablePermits; - @Override - public boolean shouldDiscardEntry(Consumer consumer, Entry entry, MessageMetadata msgMetadata, - boolean isReplayRead) { - if (remainingPermits <= 0) { - addEntryToReplay(entry); - return true; - } - remainingPermits--; - return false; - } - }; - return discardFilter; - } - /** * In Key_Shared mode, the consumer will not receive any entries from a normal reading if it is included in * {@link #recentlyJoinedConsumers}, they can only receive entries from replay reads. From 115a10980aa863a10371d18adaa7f6735d65a961 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 23:39:48 +0300 Subject: [PATCH 29/58] skip consumers that have recently closed --- .../persistent/PersistentDispatcherMultipleConsumers.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 5269238b4e6b5..d28de943d8138 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -1029,7 +1029,7 @@ protected int getFirstAvailableConsumerPermits() { return 0; } for (Consumer consumer : consumerList) { - if (consumer != null && !consumer.isBlocked()) { + if (consumer != null && !consumer.isBlocked() && consumer.cnx().isActive()) { int availablePermits = consumer.getAvailablePermits(); if (availablePermits > 0) { return availablePermits; @@ -1053,7 +1053,8 @@ private boolean isConsumerWritable() { @Override public boolean isConsumerAvailable(Consumer consumer) { - return consumer != null && !consumer.isBlocked() && consumer.getAvailablePermits() > 0; + return consumer != null && !consumer.isBlocked() && consumer.cnx().isActive() + && consumer.getAvailablePermits() > 0; } @Override From dcb55da348297db0ed1cdb58cfce5b764175b849 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 2 Sep 2024 23:41:49 +0300 Subject: [PATCH 30/58] fix permit calculations also for Shared subscriptions - permits are for entries - unacked messages are messages and need to be converted to entries --- ...PersistentDispatcherMultipleConsumers.java | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index d28de943d8138..79394ed856b38 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -791,20 +791,25 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis // round-robin dispatch batch size for this consumer int availablePermits = c.isWritable() ? c.getAvailablePermits() : 1; - if (c.getMaxUnackedMessages() > 0) { - // Avoid negative number - int remainUnAckedMessages = Math.max(c.getMaxUnackedMessages() - c.getUnackedMessages(), 0); - availablePermits = Math.min(availablePermits, remainUnAckedMessages); - } if (log.isDebugEnabled() && !c.isWritable()) { log.debug("[{}-{}] consumer is not writable. dispatching only 1 message to {}; " + "availablePermits are {}", topic.getName(), name, c, c.getAvailablePermits()); } - int maxMessagesInThisBatch = Math.max(Math.min(Math.min(remainingMessages, availablePermits), - serviceConfig.getDispatcherMaxRoundRobinBatchSize()), 1); - int maxEntriesInThisBatch = Math.max(maxMessagesInThisBatch / avgBatchSizePerMsg, 1); + int maxMessagesInThisBatch = + Math.max(remainingMessages, serviceConfig.getDispatcherMaxRoundRobinBatchSize()); + if (c.getMaxUnackedMessages() > 0) { + // Calculate the maximum number of additional unacked messages allowed + int maxAdditionalUnackedMessages = Math.max(c.getMaxUnackedMessages() - c.getUnackedMessages(), 0); + maxMessagesInThisBatch = Math.min(maxMessagesInThisBatch, maxAdditionalUnackedMessages); + } + int maxEntriesInThisBatch = Math.min(availablePermits, + // use the average batch size per message to calculate the number of entries to + // dispatch. round up to the next integer without using floating point arithmetic. + (maxMessagesInThisBatch + avgBatchSizePerMsg - 1) / avgBatchSizePerMsg); + // pick at least one entry to dispatch + maxEntriesInThisBatch = Math.max(maxEntriesInThisBatch, 1); int end = Math.min(start + maxEntriesInThisBatch, entries.size()); List entriesForThisConsumer = entries.subList(start, end); From e66a018d668cb4bc91be12f8b3290ae57d373200 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 3 Sep 2024 00:04:50 +0300 Subject: [PATCH 31/58] Use different method signature - this was a change that could have been reverted when the DispatcherDiscardFilter added in this PR was removed --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 632685b120215..3d0343ae116da 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -282,7 +282,7 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis SendMessageInfo sendMessageInfo = SendMessageInfo.getThreadLocal(); EntryBatchSizes batchSizes = EntryBatchSizes.get(entriesForConsumer.size()); EntryBatchIndexesAcks batchIndexesAcks = EntryBatchIndexesAcks.get(entriesForConsumer.size()); - totalEntries += filterEntriesForConsumer(null, 0, entriesForConsumer, batchSizes, sendMessageInfo, + totalEntries += filterEntriesForConsumer(entriesForConsumer, batchSizes, sendMessageInfo, batchIndexesAcks, cursor, readType == ReadType.Replay, consumer); consumer.sendMessages(entriesForConsumer, batchSizes, batchIndexesAcks, sendMessageInfo.getTotalMessages(), From a3355bf82f41ef19ecfb0869f8d8d0e7afea5a3d Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 3 Sep 2024 10:37:56 +0300 Subject: [PATCH 32/58] Add mocking for consumer.cnx().isActive() --- ...ckyKeyDispatcherMultipleConsumersTest.java | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java index af99741d09bb6..a58d8650d05aa 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java @@ -74,6 +74,7 @@ import org.apache.pulsar.broker.service.EntryBatchSizes; import org.apache.pulsar.broker.service.RedeliveryTracker; import org.apache.pulsar.broker.service.StickyKeyConsumerSelector; +import org.apache.pulsar.broker.service.TransportCnx; import org.apache.pulsar.broker.service.plugin.EntryFilterProvider; import org.apache.pulsar.common.api.proto.KeySharedMeta; import org.apache.pulsar.common.api.proto.KeySharedMode; @@ -189,7 +190,7 @@ public void setup() throws Exception { doReturn(subscriptionName).when(cursorMock).getName(); doReturn(ledgerMock).when(cursorMock).getManagedLedger(); - consumerMock = mock(Consumer.class); + consumerMock = createMockConsumer(); channelMock = mock(ChannelPromise.class); doReturn("consumer1").when(consumerMock).consumerName(); consumerMockAvailablePermits = new AtomicInteger(1000); @@ -212,6 +213,14 @@ public void setup() throws Exception { new KeySharedMeta().setKeySharedMode(KeySharedMode.AUTO_SPLIT)); } + protected static Consumer createMockConsumer() { + Consumer consumerMock = mock(Consumer.class); + TransportCnx transportCnx = mock(TransportCnx.class); + doReturn(transportCnx).when(consumerMock).cnx(); + doReturn(true).when(transportCnx).isActive(); + return consumerMock; + } + @AfterMethod(alwaysRun = true) public void cleanup() { if (persistentDispatcher != null && !persistentDispatcher.isClosed()) { @@ -226,7 +235,7 @@ public void cleanup() { @Test(timeOut = 10000) public void testAddConsumerWhenClosed() throws Exception { persistentDispatcher.close().get(); - Consumer consumer = mock(Consumer.class); + Consumer consumer = createMockConsumer(); persistentDispatcher.addConsumer(consumer); verify(consumer, times(1)).disconnect(); assertEquals(0, persistentDispatcher.getConsumers().size()); @@ -284,7 +293,7 @@ public void testSendMessage() { .setStart(0) .setEnd(9); - Consumer consumerMock = mock(Consumer.class); + Consumer consumerMock = createMockConsumer(); doReturn(keySharedMeta).when(consumerMock).getKeySharedMeta(); persistentDispatcher.addConsumer(consumerMock); persistentDispatcher.consumerFlow(consumerMock, 1000); @@ -306,7 +315,7 @@ public void testSendMessage() { @Test public void testSkipRedeliverTemporally() { - final Consumer slowConsumerMock = mock(Consumer.class); + final Consumer slowConsumerMock = createMockConsumer(); final ChannelPromise slowChannelMock = mock(ChannelPromise.class); // add entries to redeliver and read target final List redeliverEntries = new ArrayList<>(); @@ -419,7 +428,7 @@ public void testMessageRedelivery() throws Exception { final List readEntries = new ArrayList<>(); readEntries.add(allEntries.get(2)); // message3 - final Consumer consumer1 = mock(Consumer.class); + final Consumer consumer1 = createMockConsumer(); doReturn("consumer1").when(consumer1).consumerName(); // Change availablePermits of consumer1 to 0 and then back to normal when(consumer1.getAvailablePermits()).thenReturn(0).thenReturn(10); @@ -435,7 +444,7 @@ public void testMessageRedelivery() throws Exception { }).when(consumer1).sendMessages(anyList(), any(EntryBatchSizes.class), any(EntryBatchIndexesAcks.class), anyInt(), anyLong(), anyLong(), any(RedeliveryTracker.class)); - final Consumer consumer2 = mock(Consumer.class); + final Consumer consumer2 = createMockConsumer(); doReturn("consumer2").when(consumer2).consumerName(); when(consumer2.getAvailablePermits()).thenReturn(10); doReturn(true).when(consumer2).isWritable(); @@ -617,7 +626,7 @@ public void testLastSentPositionAndIndividuallySentPositions(final boolean initi PositionFactory.create(1, -1), PositionFactory.create(3, 19))), 60); // Add a consumer - final Consumer consumer1 = mock(Consumer.class); + final Consumer consumer1 = createMockConsumer(); doReturn("consumer1").when(consumer1).consumerName(); when(consumer1.getAvailablePermits()).thenReturn(1000); doReturn(true).when(consumer1).isWritable(); From 88256d6a4c8659cee759ae90851cab1ee4a1ef21 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 09:49:32 +0300 Subject: [PATCH 33/58] Address review comment concern about performance regression --- ...ersistentStickyKeyDispatcherMultipleConsumers.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 3d0343ae116da..f6214a8cecc65 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -404,13 +404,16 @@ private Map> filterAndGroupEntriesForDispatching(List> entriesGroupedByConsumer = new HashMap<>(); // permits for consumer, permits are for entries/batches Map permitsForConsumer = new HashMap<>(); + // maxLastSentPosition cache for consumers + Map maxLastSentPositionCache = new HashMap<>(); for (Entry entry : entries) { int stickyKeyHash = getStickyKeyHash(entry); Consumer consumer = selector.select(stickyKeyHash); boolean dispatchEntry = false; // a consumer was found for the sticky key hash and the entry can be dispatched - if (consumer != null && canDispatchEntry(consumer, entry, readType, stickyKeyHash)) { + if (consumer != null && canDispatchEntry(consumer, maxLastSentPositionCache, entry, readType, + stickyKeyHash)) { MutableInt permits = permitsForConsumer.computeIfAbsent(consumer, k -> new MutableInt(getAvailablePermits(consumer))); if (permits.intValue() > 0) { @@ -436,9 +439,11 @@ private Map> filterAndGroupEntriesForDispatching(List maxLastSentPositionCache, Entry entry, + ReadType readType, int stickyKeyHash) { // check if the entry can be replayed to a recently joined consumer - Position maxLastSentPosition = resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType); + Position maxLastSentPosition = maxLastSentPositionCache.computeIfAbsent(consumer, + __ -> resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)); if (maxLastSentPosition != null && entry.getPosition().compareTo(maxLastSentPosition) > 0) { return false; } From e7e4c8310e86dae7864a427e1f192ad0d3f44a4a Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 10:21:07 +0300 Subject: [PATCH 34/58] Optimize further --- ...stentStickyKeyDispatcherMultipleConsumers.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index f6214a8cecc65..36d1ade54a161 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -404,8 +404,8 @@ private Map> filterAndGroupEntriesForDispatching(List> entriesGroupedByConsumer = new HashMap<>(); // permits for consumer, permits are for entries/batches Map permitsForConsumer = new HashMap<>(); - // maxLastSentPosition cache for consumers - Map maxLastSentPositionCache = new HashMap<>(); + // maxLastSentPosition cache for consumers, used when recently joined consumers exist + Map maxLastSentPositionCache = hasRecentlyJoinedConsumers() ? new HashMap<>() : null; for (Entry entry : entries) { int stickyKeyHash = getStickyKeyHash(entry); @@ -442,8 +442,9 @@ private Map> filterAndGroupEntriesForDispatching(List maxLastSentPositionCache, Entry entry, ReadType readType, int stickyKeyHash) { // check if the entry can be replayed to a recently joined consumer - Position maxLastSentPosition = maxLastSentPositionCache.computeIfAbsent(consumer, - __ -> resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)); + Position maxLastSentPosition = + hasRecentlyJoinedConsumers() ? maxLastSentPositionCache.computeIfAbsent(consumer, + __ -> resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)) : null; if (maxLastSentPosition != null && entry.getPosition().compareTo(maxLastSentPosition) > 0) { return false; } @@ -566,7 +567,7 @@ public void markDeletePositionMoveForward() { // from the delete operation that was completed topic.getBrokerService().getTopicOrderedExecutor().execute(() -> { synchronized (PersistentStickyKeyDispatcherMultipleConsumers.this) { - if (recentlyJoinedConsumers != null && !recentlyJoinedConsumers.isEmpty() + if (hasRecentlyJoinedConsumers() && removeConsumersFromRecentJoinedConsumers()) { // After we process acks, we need to check whether the mark-delete position was advanced and we // can finally read more messages. It's safe to call readMoreEntries() multiple times. @@ -576,6 +577,10 @@ && removeConsumersFromRecentJoinedConsumers()) { }); } + private boolean hasRecentlyJoinedConsumers() { + return !MapUtils.isEmpty(recentlyJoinedConsumers); + } + private boolean removeConsumersFromRecentJoinedConsumers() { if (MapUtils.isEmpty(recentlyJoinedConsumers)) { return false; From 423c58e8cf9ffb94586e10a5fb7eb55c63749391 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 10:36:52 +0300 Subject: [PATCH 35/58] Use hasRecentlyJoinedConsumers method --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 36d1ade54a161..98038165da1ea 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -383,7 +383,7 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis private boolean isLookAheadAllowed() { if (serviceConfig.isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist() - || (recentlyJoinedConsumers == null || recentlyJoinedConsumers.isEmpty())) { + || !hasRecentlyJoinedConsumers()) { long keySharedNumberOfReplayMessagesThresholdForLookAhead = Math.max( serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerConsumer() * consumerList.size(), From e629ce90e158b6cd7654f0053be155b826bfa055 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 11:42:55 +0300 Subject: [PATCH 36/58] cache maxLastSentPosition for replays --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 98038165da1ea..64360ea5cedd8 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -476,6 +476,8 @@ private class ReplayPositionFilter implements Predicate { // tracks the available permits for each consumer for the duration of the filter usage // the filter is stateful and shouldn't be shared or reused later private final Map availablePermitsMap = new HashMap<>(); + private final Map maxLastSentPositionCache = + hasRecentlyJoinedConsumers() ? new HashMap<>() : null; @Override public boolean test(Position position) { @@ -508,8 +510,10 @@ public boolean test(Position position) { return false; } // check if the entry position can be replayed to a recently joined consumer - Position maxLastSentPosition = - resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, ReadType.Replay); + Position maxLastSentPosition = maxLastSentPositionCache != null + ? maxLastSentPositionCache.computeIfAbsent(consumer, __ -> + resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, ReadType.Replay)) + : null; if (maxLastSentPosition != null && position.compareTo(maxLastSentPosition) > 0) { return false; } From 7a4c34e549f3453e0459f081f98276da64426609 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 11:34:15 +0300 Subject: [PATCH 37/58] Refactor the solution: always look ahead when it would be useful --- conf/broker.conf | 7 -- conf/standalone.conf | 7 -- .../pulsar/broker/ServiceConfiguration.java | 12 --- ...PersistentDispatcherMultipleConsumers.java | 4 +- ...tStickyKeyDispatcherMultipleConsumers.java | 96 +++++++++++-------- 5 files changed, 58 insertions(+), 68 deletions(-) diff --git a/conf/broker.conf b/conf/broker.conf index 03363b5799ff0..8a0eb84b5fefa 100644 --- a/conf/broker.conf +++ b/conf/broker.conf @@ -374,13 +374,6 @@ keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 # Setting this value to 0 will disable the limit calculated per subscription. keySharedLookAheadMsgInReplayThresholdPerSubscription=10000 -# For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer -# or a blocked key hash (because of ordering constraints), the broker will continue reading more -# messages from the backlog and attempt to dispatch them to consumers until the number of replay -# messages reaches the calculated threshold. -# This setting controls whether look ahead is enabled when recently joined consumers are present. -keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist=false - # Broker periodically checks if subscription is stuck and unblock if flag is enabled. (Default is disabled) unblockStuckSubscriptionEnabled=false diff --git a/conf/standalone.conf b/conf/standalone.conf index 0f35d1f90c4f1..f196f7f51b0a7 100644 --- a/conf/standalone.conf +++ b/conf/standalone.conf @@ -251,13 +251,6 @@ keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 # Setting this value to 0 will disable the limit calculated per subscription. keySharedLookAheadMsgInReplayThresholdPerSubscription=10000 -# For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer -# or a blocked key hash (because of ordering constraints), the broker will continue reading more -# messages from the backlog and attempt to dispatch them to consumers until the number of replay -# messages reaches the calculated threshold. -# This setting controls whether look ahead is enabled when recently joined consumers are present. -keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist=false - # Tick time to schedule task that checks topic publish rate limiting across all topics # Reducing to lower value can give more accuracy while throttling publish but # it uses more CPU to perform frequent check. (Disable publish throttling with value 0) diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java index 38b9c9472eb2a..135a83530ecc6 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java @@ -979,18 +979,6 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece ) private int keySharedLookAheadMsgInReplayThresholdPerSubscription = 10000; - - @FieldContext( - category = CATEGORY_POLICIES, - doc = "For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer" - + " or a blocked key hash (because of ordering constraints), the broker will continue reading more" - + " messages from the backlog and attempt to dispatch them to consumers until the number of replay" - + " messages reaches the calculated threshold.\n" - + "This setting controls whether look ahead is enabled when recently joined consumers are present.", - dynamic = true - ) - private boolean keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist = false; - @FieldContext( category = CATEGORY_POLICIES, doc = "Once broker reaches maxUnackedMessagesPerBroker limit, it blocks subscriptions which has higher " diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 79394ed856b38..1653470991005 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -135,6 +135,7 @@ public class PersistentDispatcherMultipleConsumers extends AbstractDispatcherMul protected final ExecutorService dispatchMessagesThread; private final SharedConsumerAssignor assignor; protected int lastNumberOfEntriesDispatched; + protected boolean skipNextBackoff; private final Backoff retryBackoff; protected enum ReadType { Normal, Replay @@ -699,7 +700,8 @@ private synchronized void handleSendingMessagesAndReadingMore(ReadType readType, int entriesDispatched = lastNumberOfEntriesDispatched; updatePendingBytesToDispatch(-totalBytesSize); if (triggerReadingMore) { - if (entriesDispatched > 0) { + if (entriesDispatched > 0 || skipNextBackoff) { + skipNextBackoff = false; // Reset the backoff when we successfully dispatched messages retryBackoff.reset(); // Call readMoreEntries in the same thread to trigger the next read diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 64360ea5cedd8..9c7d3bbe659d1 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -21,6 +21,7 @@ import com.google.common.annotations.VisibleForTesting; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; @@ -38,6 +39,7 @@ import org.apache.bookkeeper.mledger.impl.ManagedCursorImpl; import org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl; import org.apache.commons.collections4.MapUtils; +import org.apache.commons.lang3.mutable.MutableBoolean; import org.apache.commons.lang3.mutable.MutableInt; import org.apache.pulsar.broker.ServiceConfiguration; import org.apache.pulsar.broker.service.BrokerServiceException; @@ -249,8 +251,12 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis } } + // returns a boolean indicating whether look-ahead could be useful, when there's a consumer + // with available permits, and it's not able to make progress because of blocked hashes. + MutableBoolean lookAheadCouldBeUseful = new MutableBoolean(); + // filter and group the entries by consumer for dispatching final Map> entriesByConsumerForDispatching = - filterAndGroupEntriesForDispatching(entries, readType); + filterAndGroupEntriesForDispatching(entries, readType, lookAheadCouldBeUseful); AtomicInteger remainingConsumersToFinishSending = new AtomicInteger(entriesByConsumerForDispatching.size()); for (Map.Entry> current : entriesByConsumerForDispatching.entrySet()) { @@ -359,63 +365,58 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis // acquire message-dispatch permits for already delivered messages acquirePermitsForDeliveredMessages(topic, cursor, totalEntries, totalMessagesSent, totalBytesSent); - if (totalMessagesSent == 0 && isLookAheadAllowed()) { - // This means, that all the messages we've just read cannot be dispatched right now. - // This condition can only happen when: - // 1. We have consumers ready to accept messages (otherwise the would not haven been triggered) - // 2. All keys in the current set of messages are routing to consumers that are currently busy - // - // The solution here is to move on and read next batch of messages which might hopefully contain - // also keys meant for other consumers. - // - // We do it unless that are "recently joined consumers". In that case, we would be looking - // ahead in the stream while the new consumers are not ready to accept the new messages, - // therefore would be most likely only increase the distance between read-position and mark-delete - // position. + if (totalMessagesSent == 0 && lookAheadCouldBeUseful.booleanValue() && isLookAheadAllowed()) { + // When all messages get filtered and no messages are sent, we should read more entries, "look ahead" + // so that a possible next batch of messages might contain messages that can be dispatched. + // This is done only when there's a consumer with available permits, and it's not able to make progress + // because of blocked hashes. Without this rule we would be looking ahead in the stream while the + // new consumers are not ready to accept the new messages, + // therefore would be most likely only increase the distance between read-position and mark-delete position. skipNextReplayToTriggerLookAhead = true; + // skip backoff delay before reading ahead in the "look ahead" mode to prevent any additional latency + skipNextBackoff = true; return true; } else if (entriesByConsumerForDispatching.size() == 0) { - // There should be a backoff delay before retrying + // There will be a backoff delay before retrying since all consumers are blocked return true; } return false; } private boolean isLookAheadAllowed() { - if (serviceConfig.isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist() - || !hasRecentlyJoinedConsumers()) { - long keySharedNumberOfReplayMessagesThresholdForLookAhead = Math.max( - serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerConsumer() - * consumerList.size(), - serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerSubscription()); - if (keySharedNumberOfReplayMessagesThresholdForLookAhead == 0 - || redeliveryMessages.size() < keySharedNumberOfReplayMessagesThresholdForLookAhead) { - return true; - } - } - return false; + long keySharedNumberOfReplayMessagesThresholdForLookAhead = Math.max( + serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerConsumer() + * consumerList.size(), + serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerSubscription()); + return (keySharedNumberOfReplayMessagesThresholdForLookAhead == 0 + || redeliveryMessages.size() < keySharedNumberOfReplayMessagesThresholdForLookAhead); } // groups the entries by consumer and filters out the entries that should not be dispatched // the entries are handled in the order they are received instead of first grouping them by consumer and // then filtering them - private Map> filterAndGroupEntriesForDispatching(List entries, ReadType readType) { + private Map> filterAndGroupEntriesForDispatching(List entries, ReadType readType, + MutableBoolean lookAheadCouldBeUseful) { // entries grouped by consumer Map> entriesGroupedByConsumer = new HashMap<>(); // permits for consumer, permits are for entries/batches Map permitsForConsumer = new HashMap<>(); // maxLastSentPosition cache for consumers, used when recently joined consumers exist Map maxLastSentPositionCache = hasRecentlyJoinedConsumers() ? new HashMap<>() : null; + Set blockedByHashConsumers = readType == ReadType.Normal ? new HashSet<>() : null; for (Entry entry : entries) { int stickyKeyHash = getStickyKeyHash(entry); Consumer consumer = selector.select(stickyKeyHash); + Position maxLastSentPosition = hasRecentlyJoinedConsumers() ? maxLastSentPositionCache.computeIfAbsent( + consumer, __ -> resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)) : null; + MutableBoolean blockedByHash = readType == ReadType.Normal ? new MutableBoolean(false) : null; + MutableInt permits = + permitsForConsumer.computeIfAbsent(consumer, k -> new MutableInt(getAvailablePermits(consumer))); boolean dispatchEntry = false; // a consumer was found for the sticky key hash and the entry can be dispatched - if (consumer != null && canDispatchEntry(consumer, maxLastSentPositionCache, entry, readType, - stickyKeyHash)) { - MutableInt permits = permitsForConsumer.computeIfAbsent(consumer, - k -> new MutableInt(getAvailablePermits(consumer))); + if (consumer != null && canDispatchEntry(entry, readType, stickyKeyHash, maxLastSentPosition, + blockedByHash)) { if (permits.intValue() > 0) { // decrement the permits for the consumer permits.decrement(); @@ -429,22 +430,36 @@ private Map> filterAndGroupEntriesForDispatching(List new ArrayList<>()); consumerEntries.add(entry); } else { + if (readType == ReadType.Normal && blockedByHash.isTrue()) { + // the entry is blocked by hash, add the consumer to the blocked set + blockedByHashConsumers.add(consumer); + } // add the message to replay addMessageToReplay(entry.getLedgerId(), entry.getEntryId(), stickyKeyHash); // release the entry as it will not be dispatched entry.release(); } } + if (readType == ReadType.Normal) { + for (Consumer consumer : blockedByHashConsumers) { + // if the consumer isn't in the entriesGroupedByConsumer, it means that it won't receive any messages + // if it has available permits, then look-ahead could be useful for this particular consumer + // to make further progress + if (!entriesGroupedByConsumer.containsKey(consumer) + && permitsForConsumer.get(consumer).intValue() > 0) { + lookAheadCouldBeUseful.setTrue(); + break; + } + } + } return entriesGroupedByConsumer; } // checks if the entry can be dispatched to the consumer - private boolean canDispatchEntry(Consumer consumer, Map maxLastSentPositionCache, Entry entry, - ReadType readType, int stickyKeyHash) { + private boolean canDispatchEntry(Entry entry, + ReadType readType, int stickyKeyHash, Position maxLastSentPosition, + MutableBoolean blockedByHash) { // check if the entry can be replayed to a recently joined consumer - Position maxLastSentPosition = - hasRecentlyJoinedConsumers() ? maxLastSentPositionCache.computeIfAbsent(consumer, - __ -> resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)) : null; if (maxLastSentPosition != null && entry.getPosition().compareTo(maxLastSentPosition) > 0) { return false; } @@ -452,6 +467,7 @@ private boolean canDispatchEntry(Consumer consumer, Map maxL // If redeliveryMessages contains messages that correspond to the same hash as the entry to be dispatched // do not send those messages for order guarantee if (readType == ReadType.Normal && redeliveryMessages.containsStickyKeyHash(stickyKeyHash)) { + blockedByHash.setTrue(); return false; } @@ -626,10 +642,8 @@ private synchronized Position updateIfNeededAndGetLastSentPosition() { * limits kick in), instead of keep replaying the same old messages, since the consumer that these * messages are routing to might be busy at the moment. * - * Please see {@link ServiceConfiguration#keySharedLookAheadMsgInReplayThresholdPerConsumer}, - * {@link ServiceConfiguration#keySharedLookAheadMsgInReplayThresholdPerSubscription} and - * {@link ServiceConfiguration#keySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist} for configuring - * the limits. + * Please see {@link ServiceConfiguration#keySharedLookAheadMsgInReplayThresholdPerConsumer} and + * {@link ServiceConfiguration#keySharedLookAheadMsgInReplayThresholdPerSubscription} for configuring the limits. */ @Override protected synchronized boolean canReplayMessages() { From e4792f3c2ed922e6289df8f2e547f647f3bb38e5 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 12:15:08 +0300 Subject: [PATCH 38/58] Enforce "look ahead" limit for Key_Shared --- ...PersistentDispatcherMultipleConsumers.java | 36 +++++++++++++------ ...tStickyKeyDispatcherMultipleConsumers.java | 30 +++++++++++----- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 1653470991005..6450fe71e7323 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -372,7 +372,11 @@ public synchronized void readMoreEntries() { log.debug("[{}] Dispatcher read is blocked due to unackMessages {} reached to max {}", name, totalUnackedMessages, topic.getMaxUnackedMessagesOnSubscription()); } - } else if (!havePendingRead && isNormalReadAllowed()) { + } else if (doesntHavePendingRead()) { + if (!isNormalReadAllowed()) { + handleNormalReadNotAllowed(); + return; + } if (shouldPauseOnAckStatePersist(ReadType.Normal)) { if (log.isDebugEnabled()) { log.debug("[{}] [{}] Skipping read for the topic, Due to blocked on ack state persistent.", @@ -403,14 +407,7 @@ public synchronized void readMoreEntries() { } } else { if (log.isDebugEnabled()) { - if (!messagesToReplayNow.isEmpty()) { - log.debug("[{}] [{}] Skipping read for the topic: because all entries in replay queue were" - + " filtered out due to the mechanism of Key_Shared mode, and the left consumers have" - + " no permits now", - topic.getName(), getSubscriptionName()); - } else { - log.debug("[{}] Cannot schedule next read until previous one is done", name); - } + log.debug("[{}] Cannot schedule next read until previous one is done", name); } } } else { @@ -420,6 +417,19 @@ public synchronized void readMoreEntries() { } } + /** + * Checks if there's a pending read operation that hasn't completed yet. + * This allows to avoid scheduling a new read operation while the previous one is still in progress. + * @return true if there's a pending read operation + */ + protected boolean doesntHavePendingRead() { + return !havePendingRead; + } + + protected void handleNormalReadNotAllowed() { + // do nothing + } + /** * Controls whether replaying entries is currently enabled. * Subclasses can override this method to temporarily disable replaying entries. @@ -452,6 +462,10 @@ protected void reScheduleRead() { reScheduleReadInMs(MESSAGE_RATE_BACKOFF_MS); } + protected synchronized void reScheduleReadWithBackoff() { + reScheduleReadInMs(retryBackoff.next()); + } + protected void reScheduleReadInMs(long readAfterMs) { if (isRescheduleReadInProgress.compareAndSet(false, true)) { if (log.isDebugEnabled()) { @@ -708,7 +722,7 @@ private synchronized void handleSendingMessagesAndReadingMore(ReadType readType, readMoreEntries(); } else if (entriesDispatched == 0) { // If no messages were dispatched, we need to reschedule a new read with an increasing backoff delay - reScheduleReadInMs(retryBackoff.next()); + reScheduleReadWithBackoff(); } } } @@ -1282,7 +1296,7 @@ protected Predicate createFilterForReplay() { * See detail {@link PersistentStickyKeyDispatcherMultipleConsumers#isNormalReadAllowed}. */ protected boolean isNormalReadAllowed() { - return true; + return !havePendingRead; } protected synchronized boolean shouldPauseDeliveryForDelayTracker() { diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 9c7d3bbe659d1..1162848404358 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -680,6 +680,16 @@ private int getAvailablePermits(Consumer c) { } } + /** + * For Key_Shared subscription, the dispatcher will not read more entries while there are pending reads + * or pending replay reads. + * @return true if there are no pending reads or pending replay reads + */ + @Override + protected boolean doesntHavePendingRead() { + return !havePendingRead && !havePendingReplayRead; + } + /** * In Key_Shared mode, the consumer will not receive any entries from a normal reading if it is included in * {@link #recentlyJoinedConsumers}, they can only receive entries from replay reads. @@ -689,21 +699,14 @@ private int getAvailablePermits(Consumer c) { */ @Override protected boolean isNormalReadAllowed() { - // Always allow ordinary reads if out-of-order delivery is enabled - if (isAllowOutOfOrderDelivery()) { - return true; + if (!isLookAheadAllowed()) { + return false; } for (Consumer consumer : consumerList) { // skip blocked consumers if (consumer == null || consumer.isBlocked()) { continue; } - // skip the consumer if isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist is false - // and consumer exists in the recently joined consumer - if (!serviceConfig.isKeySharedLookAheadEnabledWhenRecentlyJoinedConsumersExist() - && recentlyJoinedConsumers.containsKey(consumer)) { - continue; - } // before reading more, check that there's at least one consumer that has permits if (getAvailablePermits(consumer) > 0) { return true; @@ -712,6 +715,15 @@ protected boolean isNormalReadAllowed() { return false; } + @Override + protected void handleNormalReadNotAllowed() { + if (log.isDebugEnabled()) { + log.debug("[{}] [{}] Skipping read for the topic since normal read isn't allowed. " + + "Rescheduling a read with a backoff.", topic.getName(), getSubscriptionName()); + } + reScheduleReadWithBackoff(); + } + @Override public SubType getType() { return SubType.Key_Shared; From 6d5b114f02afe183309792fbc38c5761a2e8b2be Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 13:24:23 +0300 Subject: [PATCH 39/58] Check permits earlier in the logic --- ...istentStickyKeyDispatcherMultipleConsumers.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 1162848404358..b1afccd06b1b4 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -415,14 +415,12 @@ private Map> filterAndGroupEntriesForDispatching(List new MutableInt(getAvailablePermits(consumer))); boolean dispatchEntry = false; // a consumer was found for the sticky key hash and the entry can be dispatched - if (consumer != null && canDispatchEntry(entry, readType, stickyKeyHash, maxLastSentPosition, - blockedByHash)) { - if (permits.intValue() > 0) { - // decrement the permits for the consumer - permits.decrement(); - // allow the entry to be dispatched - dispatchEntry = true; - } + if (consumer != null && permits.intValue() > 0 && canDispatchEntry(entry, readType, stickyKeyHash, + maxLastSentPosition, blockedByHash)) { + // decrement the permits for the consumer + permits.decrement(); + // allow the entry to be dispatched + dispatchEntry = true; } if (dispatchEntry) { // add the entry to consumer's entry list for dispatching From 950dee481cd412b35ffc371258d85f8907c34ed7 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 14:46:37 +0300 Subject: [PATCH 40/58] Fix look ahead logic in replay mode --- ...tStickyKeyDispatcherMultipleConsumers.java | 24 +++++++++++ ...ckyKeyDispatcherMultipleConsumersTest.java | 43 ++++++++----------- 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index b1afccd06b1b4..5de02d1ffed4c 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -403,11 +403,17 @@ private Map> filterAndGroupEntriesForDispatching(List permitsForConsumer = new HashMap<>(); // maxLastSentPosition cache for consumers, used when recently joined consumers exist Map maxLastSentPositionCache = hasRecentlyJoinedConsumers() ? new HashMap<>() : null; + // in normal read mode, keep track of consumers that are blocked by hash, to check if look-ahead could be useful Set blockedByHashConsumers = readType == ReadType.Normal ? new HashSet<>() : null; + // in replay read mode, keep track of consumers for entries, used for look-ahead check + Set consumersForEntriesForLookaheadCheck = readType == ReadType.Replay ? new HashSet<>() : null; for (Entry entry : entries) { int stickyKeyHash = getStickyKeyHash(entry); Consumer consumer = selector.select(stickyKeyHash); + if (readType == ReadType.Replay) { + consumersForEntriesForLookaheadCheck.add(consumer); + } Position maxLastSentPosition = hasRecentlyJoinedConsumers() ? maxLastSentPositionCache.computeIfAbsent( consumer, __ -> resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)) : null; MutableBoolean blockedByHash = readType == ReadType.Normal ? new MutableBoolean(false) : null; @@ -438,6 +444,9 @@ private Map> filterAndGroupEntriesForDispatching(List> filterAndGroupEntriesForDispatching(List 0) { + lookAheadCouldBeUseful.setTrue(); + break; + } + } + } + } } return entriesGroupedByConsumer; } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java index a58d8650d05aa..1794c1b2d0f0a 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java @@ -119,7 +119,7 @@ public void setup() throws Exception { doReturn(100).when(configMock).getDispatcherMaxReadBatchSize(); doReturn(true).when(configMock).isSubscriptionKeySharedUseConsistentHashing(); doReturn(1).when(configMock).getSubscriptionKeySharedConsistentHashingReplicaPoints(); - doReturn(true).when(configMock).isDispatcherDispatchMessagesInSubscriptionThread(); + doReturn(false).when(configMock).isDispatcherDispatchMessagesInSubscriptionThread(); doReturn(false).when(configMock).isAllowOverrideEntryFilters(); doReturn(10).when(configMock).getDispatcherRetryBackoffInitialTimeInMs(); doReturn(50).when(configMock).getDispatcherRetryBackoffMaxTimeInMs(); @@ -343,7 +343,6 @@ public void testSkipRedeliverTemporally() { // Create 2Consumers try { doReturn("consumer2").when(slowConsumerMock).consumerName(); - // Change slowConsumer availablePermits to 0 and back to normal when(slowConsumerMock.getAvailablePermits()) .thenReturn(0) .thenReturn(1); @@ -369,28 +368,24 @@ public void testSkipRedeliverTemporally() { // Change slowConsumer availablePermits to 1 // run PersistentStickyKeyDispatcherMultipleConsumers#sendMessagesToConsumers internally // and then stop to dispatch to slowConsumer - if (persistentDispatcher.sendMessagesToConsumers(PersistentStickyKeyDispatcherMultipleConsumers.ReadType.Normal, - redeliverEntries, true)) { - persistentDispatcher.readMoreEntriesAsync(); - } - - Awaitility.await().untilAsserted(() -> { - verify(consumerMock, times(1)).sendMessages( - argThat(arg -> { - assertEquals(arg.size(), 1); - Entry entry = arg.get(0); - assertEquals(entry.getLedgerId(), 1); - assertEquals(entry.getEntryId(), 3); - return true; - }), - any(EntryBatchSizes.class), - any(EntryBatchIndexesAcks.class), - anyInt(), - anyLong(), - anyLong(), - any(RedeliveryTracker.class) - ); - }); + persistentDispatcher.readEntriesComplete(redeliverEntries, + PersistentDispatcherMultipleConsumers.ReadType.Replay); + + verify(consumerMock, times(1)).sendMessages( + argThat(arg -> { + assertEquals(arg.size(), 1); + Entry entry = arg.get(0); + assertEquals(entry.getLedgerId(), 1); + assertEquals(entry.getEntryId(), 3); + return true; + }), + any(EntryBatchSizes.class), + any(EntryBatchIndexesAcks.class), + anyInt(), + anyLong(), + anyLong(), + any(RedeliveryTracker.class) + ); verify(slowConsumerMock, times(0)).sendMessages( anyList(), any(EntryBatchSizes.class), From 0d469ec82d444bc9be914eee63c2037c8e661b03 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 14:51:36 +0300 Subject: [PATCH 41/58] Fix null handling --- ...tStickyKeyDispatcherMultipleConsumers.java | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 5de02d1ffed4c..bb7fe6006e0d8 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -411,22 +411,26 @@ private Map> filterAndGroupEntriesForDispatching(List resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)) : null; - MutableBoolean blockedByHash = readType == ReadType.Normal ? new MutableBoolean(false) : null; - MutableInt permits = - permitsForConsumer.computeIfAbsent(consumer, k -> new MutableInt(getAvailablePermits(consumer))); + MutableBoolean blockedByHash = null; boolean dispatchEntry = false; - // a consumer was found for the sticky key hash and the entry can be dispatched - if (consumer != null && permits.intValue() > 0 && canDispatchEntry(entry, readType, stickyKeyHash, - maxLastSentPosition, blockedByHash)) { - // decrement the permits for the consumer - permits.decrement(); - // allow the entry to be dispatched - dispatchEntry = true; + if (consumer != null) { + if (readType == ReadType.Replay) { + consumersForEntriesForLookaheadCheck.add(consumer); + } + Position maxLastSentPosition = hasRecentlyJoinedConsumers() ? maxLastSentPositionCache.computeIfAbsent( + consumer, __ -> resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)) : null; + blockedByHash = readType == ReadType.Normal ? new MutableBoolean(false) : null; + MutableInt permits = + permitsForConsumer.computeIfAbsent(consumer, + k -> new MutableInt(getAvailablePermits(consumer))); + // a consumer was found for the sticky key hash and the entry can be dispatched + if (permits.intValue() > 0 && canDispatchEntry(entry, readType, stickyKeyHash, + maxLastSentPosition, blockedByHash)) { + // decrement the permits for the consumer + permits.decrement(); + // allow the entry to be dispatched + dispatchEntry = true; + } } if (dispatchEntry) { // add the entry to consumer's entry list for dispatching @@ -434,7 +438,7 @@ private Map> filterAndGroupEntriesForDispatching(List new ArrayList<>()); consumerEntries.add(entry); } else { - if (readType == ReadType.Normal && blockedByHash.isTrue()) { + if (blockedByHash != null && blockedByHash.isTrue()) { // the entry is blocked by hash, add the consumer to the blocked set blockedByHashConsumers.add(consumer); } From 910da2c1ec216d219023a3b6fa80c8c1e963bda4 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 15:09:36 +0300 Subject: [PATCH 42/58] Revisit logic to make as much progress as possible --- ...tStickyKeyDispatcherMultipleConsumers.java | 65 +++++++++++-------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index bb7fe6006e0d8..ca1378230feca 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -253,10 +253,10 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis // returns a boolean indicating whether look-ahead could be useful, when there's a consumer // with available permits, and it's not able to make progress because of blocked hashes. - MutableBoolean lookAheadCouldBeUseful = new MutableBoolean(); + MutableBoolean triggerLookAhead = new MutableBoolean(); // filter and group the entries by consumer for dispatching final Map> entriesByConsumerForDispatching = - filterAndGroupEntriesForDispatching(entries, readType, lookAheadCouldBeUseful); + filterAndGroupEntriesForDispatching(entries, readType, triggerLookAhead); AtomicInteger remainingConsumersToFinishSending = new AtomicInteger(entriesByConsumerForDispatching.size()); for (Map.Entry> current : entriesByConsumerForDispatching.entrySet()) { @@ -365,7 +365,8 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis // acquire message-dispatch permits for already delivered messages acquirePermitsForDeliveredMessages(topic, cursor, totalEntries, totalMessagesSent, totalBytesSent); - if (totalMessagesSent == 0 && lookAheadCouldBeUseful.booleanValue() && isLookAheadAllowed()) { + // trigger read more messages if necessary + if (triggerLookAhead.booleanValue()) { // When all messages get filtered and no messages are sent, we should read more entries, "look ahead" // so that a possible next batch of messages might contain messages that can be dispatched. // This is done only when there's a consumer with available permits, and it's not able to make progress @@ -376,10 +377,13 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis // skip backoff delay before reading ahead in the "look ahead" mode to prevent any additional latency skipNextBackoff = true; return true; - } else if (entriesByConsumerForDispatching.size() == 0) { - // There will be a backoff delay before retrying since all consumers are blocked + } + + // if no messages were sent, we should retry after a backoff delay + if (entriesByConsumerForDispatching.size() == 0) { return true; } + return false; } @@ -396,17 +400,19 @@ private boolean isLookAheadAllowed() { // the entries are handled in the order they are received instead of first grouping them by consumer and // then filtering them private Map> filterAndGroupEntriesForDispatching(List entries, ReadType readType, - MutableBoolean lookAheadCouldBeUseful) { + MutableBoolean triggerLookAhead) { // entries grouped by consumer Map> entriesGroupedByConsumer = new HashMap<>(); // permits for consumer, permits are for entries/batches Map permitsForConsumer = new HashMap<>(); // maxLastSentPosition cache for consumers, used when recently joined consumers exist - Map maxLastSentPositionCache = hasRecentlyJoinedConsumers() ? new HashMap<>() : null; + boolean hasRecentlyJoinedConsumers = hasRecentlyJoinedConsumers(); + Map maxLastSentPositionCache = hasRecentlyJoinedConsumers ? new HashMap<>() : null; + boolean lookAheadAllowed = isLookAheadAllowed(); // in normal read mode, keep track of consumers that are blocked by hash, to check if look-ahead could be useful - Set blockedByHashConsumers = readType == ReadType.Normal ? new HashSet<>() : null; + Set blockedByHashConsumers = lookAheadAllowed && readType == ReadType.Normal ? new HashSet<>() : null; // in replay read mode, keep track of consumers for entries, used for look-ahead check - Set consumersForEntriesForLookaheadCheck = readType == ReadType.Replay ? new HashSet<>() : null; + Set consumersForEntriesForLookaheadCheck = lookAheadAllowed ? new HashSet<>() : null; for (Entry entry : entries) { int stickyKeyHash = getStickyKeyHash(entry); @@ -414,12 +420,12 @@ private Map> filterAndGroupEntriesForDispatching(List resolveMaxLastSentPositionForRecentlyJoinedConsumer(consumer, readType)) : null; - blockedByHash = readType == ReadType.Normal ? new MutableBoolean(false) : null; + blockedByHash = lookAheadAllowed && readType == ReadType.Normal ? new MutableBoolean(false) : null; MutableInt permits = permitsForConsumer.computeIfAbsent(consumer, k -> new MutableInt(getAvailablePermits(consumer))); @@ -451,27 +457,30 @@ private Map> filterAndGroupEntriesForDispatching(List 0) { - lookAheadCouldBeUseful.setTrue(); - break; + if (lookAheadAllowed && entriesGroupedByConsumer.isEmpty()) { + // check if look-ahead could be useful for the consumers that are blocked by a hash that is in the replay + // queue. This check applies only to the normal read mode. + if (readType == ReadType.Normal) { + for (Consumer consumer : blockedByHashConsumers) { + // if the consumer isn't in the entriesGroupedByConsumer, it means that it won't receive any + // messages + // if it has available permits, then look-ahead could be useful for this particular consumer + // to make further progress + if (!entriesGroupedByConsumer.containsKey(consumer) + && permitsForConsumer.get(consumer).intValue() > 0) { + triggerLookAhead.setTrue(); + break; + } } } - } else if (readType == ReadType.Replay) { - // if all entries are filtered out due to the order guarantee mechanism, then look-ahead could be useful - // if there are other consumers with available permits - if (entriesGroupedByConsumer.isEmpty()) { + // check if look-ahead could be useful for other consumers + if (!triggerLookAhead.booleanValue()) { for (Consumer consumer : getConsumers()) { // filter out the consumers that are already checked when the entries were processed for entries if (!consumersForEntriesForLookaheadCheck.contains(consumer)) { // if another consumer has available permits, then look-ahead could be useful if (getAvailablePermits(consumer) > 0) { - lookAheadCouldBeUseful.setTrue(); + triggerLookAhead.setTrue(); break; } } @@ -493,7 +502,9 @@ private boolean canDispatchEntry(Entry entry, // If redeliveryMessages contains messages that correspond to the same hash as the entry to be dispatched // do not send those messages for order guarantee if (readType == ReadType.Normal && redeliveryMessages.containsStickyKeyHash(stickyKeyHash)) { - blockedByHash.setTrue(); + if (blockedByHash != null) { + blockedByHash.setTrue(); + } return false; } From 209b69ee5d338c6a538bbecf4f5d2128ab8e9c7a Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 15:41:15 +0300 Subject: [PATCH 43/58] Fix limit calculation logic and fix test that relied on previous logic --- ...ntStickyKeyDispatcherMultipleConsumers.java | 18 ++++++++++++------ .../client/api/KeySharedSubscriptionTest.java | 10 +++++++--- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index ca1378230feca..c93ff92c82512 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -388,12 +388,18 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis } private boolean isLookAheadAllowed() { - long keySharedNumberOfReplayMessagesThresholdForLookAhead = Math.max( - serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerConsumer() - * consumerList.size(), - serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerSubscription()); - return (keySharedNumberOfReplayMessagesThresholdForLookAhead == 0 - || redeliveryMessages.size() < keySharedNumberOfReplayMessagesThresholdForLookAhead); + int perConsumerLimit = serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerConsumer(); + int perSubscriptionLimit = serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerSubscription(); + long effectiveLimit; + if (perConsumerLimit <= 0) { + effectiveLimit = perSubscriptionLimit; + } else { + effectiveLimit = perConsumerLimit * consumerList.size(); + if (perSubscriptionLimit > 0 && perSubscriptionLimit < effectiveLimit) { + effectiveLimit = perSubscriptionLimit; + } + } + return (effectiveLimit <= 0 || redeliveryMessages.size() < effectiveLimit); } // groups the entries by consumer and filters out the entries that should not be dispatched diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java index d10868fdd73e9..1ec7edbcff76a 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java @@ -630,8 +630,11 @@ public void testOrderingWhenAddingConsumers() throws Exception { } @Test - public void testReadAheadWhenAddingConsumers() throws Exception { - String topic = "testReadAheadWhenAddingConsumers-" + UUID.randomUUID(); + public void testReadAheadWithConfiguredLookAheadLimit() throws Exception { + String topic = "testReadAheadWithConfiguredLookAheadLimit-" + UUID.randomUUID(); + + // Set the look ahead limit to 50 for subscriptions + conf.setKeySharedLookAheadMsgInReplayThresholdPerSubscription(50); @Cleanup Producer producer = createProducer(topic, false); @@ -679,7 +682,8 @@ public void testReadAheadWhenAddingConsumers() throws Exception { // We need to ensure that dispatcher does not keep to look ahead in the topic, Position readPosition = sub.getCursor().getReadPosition(); - assertTrue(readPosition.getEntryId() < 1000); + long entryId = readPosition.getEntryId(); + assertTrue(entryId < 100); } @Test From a48a3fb1f0b7766e6ae80d0e17b0a31e36e35c1c Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 16:22:26 +0300 Subject: [PATCH 44/58] Improve effective limit logic, add test --- ...tStickyKeyDispatcherMultipleConsumers.java | 11 ++++- .../KeySharedLookAheadConfigTest.java | 49 +++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/KeySharedLookAheadConfigTest.java diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index c93ff92c82512..101d1f255b622 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -388,18 +388,25 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis } private boolean isLookAheadAllowed() { + return redeliveryMessages.size() < getEffectiveLookAheadLimit(serviceConfig, consumerList.size()); + } + + static long getEffectiveLookAheadLimit(ServiceConfiguration serviceConfig, int consumerCount) { int perConsumerLimit = serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerConsumer(); int perSubscriptionLimit = serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerSubscription(); long effectiveLimit; if (perConsumerLimit <= 0) { effectiveLimit = perSubscriptionLimit; } else { - effectiveLimit = perConsumerLimit * consumerList.size(); + effectiveLimit = perConsumerLimit * consumerCount; if (perSubscriptionLimit > 0 && perSubscriptionLimit < effectiveLimit) { effectiveLimit = perSubscriptionLimit; } } - return (effectiveLimit <= 0 || redeliveryMessages.size() < effectiveLimit); + if (effectiveLimit <= 0) { + effectiveLimit = Integer.MAX_VALUE; + } + return effectiveLimit; } // groups the entries by consumer and filters out the entries that should not be dispatched diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/KeySharedLookAheadConfigTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/KeySharedLookAheadConfigTest.java new file mode 100644 index 0000000000000..466e492df718e --- /dev/null +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/KeySharedLookAheadConfigTest.java @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.service.persistent; + +import static org.apache.pulsar.broker.service.persistent.PersistentStickyKeyDispatcherMultipleConsumers.getEffectiveLookAheadLimit; +import static org.testng.Assert.assertEquals; +import org.apache.pulsar.broker.ServiceConfiguration; +import org.testng.annotations.Test; + +public class KeySharedLookAheadConfigTest { + + @Test + public void testGetEffectiveLookAheadLimit() { + ServiceConfiguration config = new ServiceConfiguration(); + + config.setKeySharedLookAheadMsgInReplayThresholdPerConsumer(5); + config.setKeySharedLookAheadMsgInReplayThresholdPerSubscription(100); + assertEquals(getEffectiveLookAheadLimit(config, 5), 25); + assertEquals(getEffectiveLookAheadLimit(config, 100), 100); + + config.setKeySharedLookAheadMsgInReplayThresholdPerConsumer(5); + config.setKeySharedLookAheadMsgInReplayThresholdPerSubscription(0); + assertEquals(getEffectiveLookAheadLimit(config, 100), 500); + + config.setKeySharedLookAheadMsgInReplayThresholdPerConsumer(0); + config.setKeySharedLookAheadMsgInReplayThresholdPerSubscription(6000); + assertEquals(getEffectiveLookAheadLimit(config, 100), 6000); + + config.setKeySharedLookAheadMsgInReplayThresholdPerConsumer(0); + config.setKeySharedLookAheadMsgInReplayThresholdPerSubscription(0); + assertEquals(getEffectiveLookAheadLimit(config, 100), Integer.MAX_VALUE); + } +} \ No newline at end of file From f78f192453acf01c1ad36ed6db6a01d398bee346 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 18:48:20 +0300 Subject: [PATCH 45/58] Rename method for more clarity --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 101d1f255b622..be478f337f2f0 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -387,7 +387,7 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis return false; } - private boolean isLookAheadAllowed() { + private boolean isReplayQueueSizeBelowLimit() { return redeliveryMessages.size() < getEffectiveLookAheadLimit(serviceConfig, consumerList.size()); } @@ -421,7 +421,7 @@ private Map> filterAndGroupEntriesForDispatching(List maxLastSentPositionCache = hasRecentlyJoinedConsumers ? new HashMap<>() : null; - boolean lookAheadAllowed = isLookAheadAllowed(); + boolean lookAheadAllowed = isReplayQueueSizeBelowLimit(); // in normal read mode, keep track of consumers that are blocked by hash, to check if look-ahead could be useful Set blockedByHashConsumers = lookAheadAllowed && readType == ReadType.Normal ? new HashSet<>() : null; // in replay read mode, keep track of consumers for entries, used for look-ahead check @@ -749,7 +749,8 @@ protected boolean doesntHavePendingRead() { */ @Override protected boolean isNormalReadAllowed() { - if (!isLookAheadAllowed()) { + // don't allow reading more if the replay queue size has reached the limit + if (!isReplayQueueSizeBelowLimit()) { return false; } for (Consumer consumer : consumerList) { From 2d3e0d559b71598e3bf4a897b75a3265bc4df676 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 18:50:20 +0300 Subject: [PATCH 46/58] Update javadoc for isNormalReadAllowed --- ...PersistentStickyKeyDispatcherMultipleConsumers.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index be478f337f2f0..d9cdf522a9b9c 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -741,11 +741,8 @@ protected boolean doesntHavePendingRead() { } /** - * In Key_Shared mode, the consumer will not receive any entries from a normal reading if it is included in - * {@link #recentlyJoinedConsumers}, they can only receive entries from replay reads. - * If all entries in {@link #redeliveryMessages} have been filtered out due to the order guarantee mechanism, - * Broker need a normal read to make the consumers not included in @link #recentlyJoinedConsumers} will not be - * stuck. See https://github.com/apache/pulsar/pull/7105. + * For Key_Shared subscription, the dispatcher will not attempt to read more entries if the replay queue size + * has reached the limit or if there are no consumers with permits. */ @Override protected boolean isNormalReadAllowed() { @@ -766,6 +763,9 @@ protected boolean isNormalReadAllowed() { return false; } + /** + * When a normal read is not allowed, the dispatcher will reschedule a read with a backoff. + */ @Override protected void handleNormalReadNotAllowed() { if (log.isDebugEnabled()) { From ca47e33765722a2882d5e188c061e9686c1084c0 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 6 Sep 2024 18:52:05 +0300 Subject: [PATCH 47/58] Update isNormalReadAllowed in super class --- .../persistent/PersistentDispatcherMultipleConsumers.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 6450fe71e7323..212cd1f28716e 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -1292,11 +1292,10 @@ protected Predicate createFilterForReplay() { } /** - * This is a mode method designed for Key_Shared mode, to avoid unnecessary stuck. - * See detail {@link PersistentStickyKeyDispatcherMultipleConsumers#isNormalReadAllowed}. + * Checks if the dispatcher is allowed to read messages from the cursor. */ protected boolean isNormalReadAllowed() { - return !havePendingRead; + return true; } protected synchronized boolean shouldPauseDeliveryForDelayTracker() { From bf89d9639098d54a704a9d9b9a2e19b9cd81c9ac Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 7 Sep 2024 09:37:32 +0300 Subject: [PATCH 48/58] Fix formula mentioned in doc (max->min) --- conf/broker.conf | 4 ++-- conf/standalone.conf | 4 ++-- .../java/org/apache/pulsar/broker/ServiceConfiguration.java | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/conf/broker.conf b/conf/broker.conf index 8a0eb84b5fefa..b9955e5c43c6f 100644 --- a/conf/broker.conf +++ b/conf/broker.conf @@ -359,7 +359,7 @@ maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 # or a blocked key hash (because of ordering constraints), the broker will continue reading more # messages from the backlog and attempt to dispatch them to consumers until the number of replay # messages reaches the calculated threshold. -# Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer * +# Formula: threshold = min(keySharedLookAheadMsgInReplayThresholdPerConsumer * # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # Setting this value to 0 will disable the limit calculated per consumer. keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 @@ -368,7 +368,7 @@ keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 # or a blocked key hash (because of ordering constraints), the broker will continue reading more # messages from the backlog and attempt to dispatch them to consumers until the number of replay # messages reaches the calculated threshold. -# Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer * +# Formula: threshold = min(keySharedLookAheadMsgInReplayThresholdPerConsumer * # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. # Setting this value to 0 will disable the limit calculated per subscription. diff --git a/conf/standalone.conf b/conf/standalone.conf index f196f7f51b0a7..db7ee632835be 100644 --- a/conf/standalone.conf +++ b/conf/standalone.conf @@ -236,7 +236,7 @@ maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 # or a blocked key hash (because of ordering constraints), the broker will continue reading more # messages from the backlog and attempt to dispatch them to consumers until the number of replay # messages reaches the calculated threshold. -# Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer * +# Formula: threshold = min(keySharedLookAheadMsgInReplayThresholdPerConsumer * # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # Setting this value to 0 will disable the limit calculated per consumer. keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 @@ -245,7 +245,7 @@ keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 # or a blocked key hash (because of ordering constraints), the broker will continue reading more # messages from the backlog and attempt to dispatch them to consumers until the number of replay # messages reaches the calculated threshold. -# Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer * +# Formula: threshold = min(keySharedLookAheadMsgInReplayThresholdPerConsumer * # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. # Setting this value to 0 will disable the limit calculated per subscription. diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java index 135a83530ecc6..dc69e3de865c7 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java @@ -956,7 +956,7 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " or a blocked key hash (because of ordering constraints), the broker will continue reading more" + " messages from the backlog and attempt to dispatch them to consumers until the number of replay" + " messages reaches the calculated threshold.\n" - + "Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer *" + + "Formula: threshold = min(keySharedLookAheadMsgInReplayThresholdPerConsumer *" + " connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription)" + ".\n" + "Setting this value to 0 will disable the limit calculated per consumer.", @@ -970,7 +970,7 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " or a blocked key hash (because of ordering constraints), the broker will continue reading more" + " messages from the backlog and attempt to dispatch them to consumers until the number of replay" + " messages reaches the calculated threshold.\n" - + "Formula: threshold = max(keySharedLookAheadMsgInReplayThresholdPerConsumer *" + + "Formula: threshold = min(keySharedLookAheadMsgInReplayThresholdPerConsumer *" + " connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription)" + ".\n" + "This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist.\n" From e937a4e3b5c039977392931d8bedd49d68ee0033 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Sat, 7 Sep 2024 09:40:14 +0300 Subject: [PATCH 49/58] Fix IntelliJ warning --- .../PersistentStickyKeyDispatcherMultipleConsumers.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index d9cdf522a9b9c..adb9698755a95 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -692,8 +692,8 @@ private synchronized Position updateIfNeededAndGetLastSentPosition() { * limits kick in), instead of keep replaying the same old messages, since the consumer that these * messages are routing to might be busy at the moment. * - * Please see {@link ServiceConfiguration#keySharedLookAheadMsgInReplayThresholdPerConsumer} and - * {@link ServiceConfiguration#keySharedLookAheadMsgInReplayThresholdPerSubscription} for configuring the limits. + * Please see {@link ServiceConfiguration#getKeySharedLookAheadMsgInReplayThresholdPerConsumer} and + * {@link ServiceConfiguration#getKeySharedLookAheadMsgInReplayThresholdPerSubscription} for configuring the limits. */ @Override protected synchronized boolean canReplayMessages() { From 4c777afdf50405e78d2a53dc64db0460e9b7f0dd Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Tue, 10 Sep 2024 09:57:06 +0300 Subject: [PATCH 50/58] Implement ConcurrentBitmapSortedLongPairSet.first --- .../persistent/MessageRedeliveryController.java | 3 +-- .../utils/ConcurrentBitmapSortedLongPairSet.java | 11 +++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java index 9dd3c39bddb6b..fa6e1412151b6 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java @@ -154,8 +154,7 @@ public boolean containsStickyKeyHash(int stickyKeyHash) { } public Optional getFirstPositionInReplay() { - NavigableSet items = messagesToRedeliver.items(1, PositionFactory::create); - return items.isEmpty() ? Optional.empty() : Optional.of(items.first()); + return messagesToRedeliver.first(PositionFactory::create); } /** diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java b/pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java index 419793ba4720d..cc1eae475fa2d 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java @@ -22,10 +22,12 @@ import java.util.Map; import java.util.NavigableMap; import java.util.NavigableSet; +import java.util.Optional; import java.util.TreeMap; import java.util.TreeSet; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import org.apache.commons.lang3.mutable.MutableObject; import org.apache.pulsar.common.util.collections.LongPairSet; import org.roaringbitmap.RoaringBitmap; @@ -93,6 +95,15 @@ public void removeUpTo(long item1, long item2) { } } + public > Optional first(LongPairSet.LongPairFunction longPairConverter) { + MutableObject> result = new MutableObject<>(Optional.empty()); + processItems(longPairConverter, item -> { + result.setValue(Optional.of(item)); + return false; + }); + return result.getValue(); + } + public > NavigableSet items(int numberOfItems, LongPairSet.LongPairFunction longPairConverter) { NavigableSet items = new TreeSet<>(); From 0cdd5f7157d04e5cb7ca40268f5fdb833f481449 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 13 Sep 2024 20:14:30 +0300 Subject: [PATCH 51/58] Add testing utilities --- .../apache/pulsar/broker/BrokerTestUtil.java | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java index 79b7368b6b90c..a248ee62492e8 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java @@ -23,9 +23,22 @@ import java.io.IOException; import java.io.StringWriter; import java.io.UncheckedIOException; +import java.time.Duration; +import java.util.Arrays; import java.util.UUID; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.BiConsumer; +import java.util.function.BiFunction; import org.apache.pulsar.client.admin.PulsarAdmin; import org.apache.pulsar.client.admin.PulsarAdminException; +import org.apache.pulsar.client.api.Consumer; +import org.apache.pulsar.client.api.Message; +import org.apache.pulsar.client.api.PulsarClientException; +import org.apache.pulsar.common.util.FutureUtil; import org.apache.pulsar.common.util.ObjectMapperFactory; import org.mockito.Mockito; import org.slf4j.Logger; @@ -119,4 +132,105 @@ public static void logTopicStats(Logger logger, PulsarAdmin pulsarAdmin, String } } + /** + * Receive messages concurrently from multiple consumers and handles them using the provided message handler. + * The message handler should return true if it wants to continue receiving more messages, false otherwise. + * + * @param messageHandler the message handler + * @param quietTimeout the duration of quiet time after which the method will stop waiting for more messages + * @param consumers the consumers to receive messages from + * @param the message value type + */ + public static void receiveMessages(BiFunction, Message, Boolean> messageHandler, + Duration quietTimeout, + Consumer... consumers) { + FutureUtil.waitForAll(Arrays.stream(consumers) + .map(consumer -> receiveMessagesAsync(consumer, quietTimeout, messageHandler)).toList()).join(); + } + + // asynchronously receive messages from a consumer and handle them using the provided message handler + // the benefit is that multiple consumers can be concurrently consumed without the need to have multiple threads + // this is useful in tests where multiple consumers are needed to test the functionality + private static CompletableFuture receiveMessagesAsync(Consumer consumer, Duration quietTimeout, + BiFunction, Message, Boolean> + messageHandler) { + CompletableFuture> receiveFuture = consumer.receiveAsync(); + return receiveFuture + .orTimeout(quietTimeout.toMillis(), TimeUnit.MILLISECONDS) + .handle((msg, t) -> { + if (t != null) { + if (t instanceof TimeoutException) { + // cancel the receive future so that Pulsar client can clean up the resources + receiveFuture.cancel(false); + return false; + } else { + throw FutureUtil.wrapToCompletionException(t); + } + } + return messageHandler.apply(consumer, msg); + }).thenComposeAsync(receiveMore -> { + if (receiveMore) { + return receiveMessagesAsync(consumer, quietTimeout, messageHandler); + } else { + return CompletableFuture.completedFuture(null); + } + }); + } + + /** + * Receive messages concurrently from multiple consumers and handles them using the provided message handler. + * The messages are received until the quiet timeout is reached or the maximum number of messages is received. + * + * @param messageHandler the message handler + * @param quietTimeout the duration of quiet time after which the method will stop waiting for more messages + * @param maxMessages the maximum number of messages to receive + * @param consumers the consumers to receive messages from + * @param the message value type + */ + public static void receiveMessagesN(BiConsumer, Message> messageHandler, + Duration quietTimeout, + int maxMessages, + Consumer... consumers) + throws ExecutionException, InterruptedException { + AtomicInteger messagesReceived = new AtomicInteger(); + receiveMessages( + (consumer, message) -> { + messageHandler.accept(consumer, message); + return messagesReceived.incrementAndGet() < maxMessages; + }, quietTimeout, consumers); + } + + /** + * Receive messages concurrently from multiple consumers and handles them using the provided message handler. + * + * @param messageHandler the message handler + * @param quietTimeout the duration of quiet time after which the method will stop waiting for more messages + * @param consumers the consumers to receive messages from + * @param the message value type + */ + static void receiveMessagesInThreads(BiFunction, Message, Boolean> messageHandler, + final Duration quietTimeout, + Consumer... consumers) { + FutureUtil.waitForAll(Arrays.stream(consumers).sequential().map(consumer -> { + return CompletableFuture.runAsync(() -> { + try { + while (!Thread.currentThread().isInterrupted()) { + Message msg = consumer.receive((int) quietTimeout.toMillis(), TimeUnit.MILLISECONDS); + if (msg != null) { + if (!messageHandler.apply(consumer, msg)) { + break; + } + } else { + break; + } + } + } catch (PulsarClientException e) { + throw new RuntimeException(e); + } + }, runnable -> { + Thread thread = new Thread(runnable, "Consumer-" + consumer.getConsumerName()); + thread.start(); + }); + }).toList()).join(); + } } From b0d9fa0371526fd96163c1b9de36c7892c4e3d87 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 13 Sep 2024 21:03:26 +0300 Subject: [PATCH 52/58] Add test for read limit and fix the detected issue --- ...PersistentDispatcherMultipleConsumers.java | 15 ++ ...tStickyKeyDispatcherMultipleConsumers.java | 17 ++- .../client/api/KeySharedSubscriptionTest.java | 130 ++++++++++++++++++ 3 files changed, 159 insertions(+), 3 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java index 5b7151569ac54..450a446c85a78 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java @@ -391,6 +391,8 @@ public synchronized void readMoreEntries() { havePendingRead = true; updateMinReplayedPosition(); + messagesToRead = Math.min(messagesToRead, getMaxEntriesReadLimit()); + // Filter out and skip read delayed messages exist in DelayedDeliveryTracker if (delayedDeliveryTracker.isPresent()) { Predicate skipCondition = null; @@ -417,6 +419,17 @@ public synchronized void readMoreEntries() { } } + /** + * Sets a hard limit on the number of entries to read from the Managed Ledger. + * Subclasses can override this method to set a different limit. + * By default, this method does not impose an additional limit. + * + * @return the maximum number of entries to read from the Managed Ledger + */ + protected int getMaxEntriesReadLimit() { + return Integer.MAX_VALUE; + } + /** * Checks if there's a pending read operation that hasn't completed yet. * This allows to avoid scheduling a new read operation while the previous one is still in progress. @@ -1301,6 +1314,8 @@ protected boolean isNormalReadAllowed() { return true; } + + protected synchronized boolean shouldPauseDeliveryForDelayTracker() { return delayedDeliveryTracker.isPresent() && delayedDeliveryTracker.get().shouldPauseAllDeliveries(); } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 9901387890e6b..4ac02208d73f5 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -388,13 +388,17 @@ protected synchronized boolean trySendMessagesToConsumers(ReadType readType, Lis } private boolean isReplayQueueSizeBelowLimit() { - return redeliveryMessages.size() < getEffectiveLookAheadLimit(serviceConfig, consumerList.size()); + return redeliveryMessages.size() < getEffectiveLookAheadLimit(); } - static long getEffectiveLookAheadLimit(ServiceConfiguration serviceConfig, int consumerCount) { + private int getEffectiveLookAheadLimit() { + return getEffectiveLookAheadLimit(serviceConfig, consumerList.size()); + } + + static int getEffectiveLookAheadLimit(ServiceConfiguration serviceConfig, int consumerCount) { int perConsumerLimit = serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerConsumer(); int perSubscriptionLimit = serviceConfig.getKeySharedLookAheadMsgInReplayThresholdPerSubscription(); - long effectiveLimit; + int effectiveLimit; if (perConsumerLimit <= 0) { effectiveLimit = perSubscriptionLimit; } else { @@ -763,6 +767,13 @@ protected boolean isNormalReadAllowed() { return false; } + @Override + protected int getMaxEntriesReadLimit() { + // prevent the redelivery queue from growing over the limit by limiting the number of entries to read + // to the maximum number of entries that can be added to the redelivery queue + return Math.max(getEffectiveLookAheadLimit() - redeliveryMessages.size(), 1); + } + /** * When a normal read is not allowed, the dispatcher will reschedule a read with a backoff. */ diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java index 1ec7edbcff76a..31ec288dba315 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java @@ -18,6 +18,9 @@ */ package org.apache.pulsar.client.api; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.pulsar.broker.BrokerTestUtil.receiveMessages; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.spy; @@ -32,6 +35,7 @@ import java.lang.reflect.Field; import java.nio.file.Files; import java.nio.file.Paths; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -2306,4 +2310,130 @@ public void testRecentJoinedPosWillNotStuckOtherConsumer(boolean allowKeySharedO producer.close(); admin.topics().delete(topic, false); } + + @Test + public void testReadAheadLimit() throws Exception { + String topic = "testReadAheadLimit-" + UUID.randomUUID(); + int numberOfKeys = 1000; + long pauseTime = 100L; + int readAheadLimit = 20; + pulsar.getConfig().setKeySharedLookAheadMsgInReplayThresholdPerSubscription(readAheadLimit); + + @Cleanup + Producer producer = createProducer(topic, false); + + // create a consumer and close it to create a subscription + String subscriptionName = "key_shared"; + pulsarClient.newConsumer(Schema.INT32) + .topic(topic) + .subscriptionName(subscriptionName) + .subscriptionType(SubscriptionType.Key_Shared) + .subscribe() + .close(); + + Topic t = pulsar.getBrokerService().getTopicIfExists(topic).get().get(); + PersistentSubscription sub = (PersistentSubscription) t.getSubscription(subscriptionName); + // get the dispatcher reference + PersistentStickyKeyDispatcherMultipleConsumers dispatcher = + (PersistentStickyKeyDispatcherMultipleConsumers) sub.getDispatcher(); + + // create a function to use for checking the number of messages in replay + Runnable checkLimit = () -> { + assertThat(dispatcher.getNumberOfMessagesInReplay()).isLessThanOrEqualTo(readAheadLimit); + }; + + // Adding a new consumer. + @Cleanup + Consumer c1 = pulsarClient.newConsumer(Schema.INT32) + .topic(topic) + .consumerName("c1") + .subscriptionName(subscriptionName) + .subscriptionType(SubscriptionType.Key_Shared) + .receiverQueueSize(10) + .startPaused(true) // start paused + .subscribe(); + + @Cleanup + Consumer c2 = pulsarClient.newConsumer(Schema.INT32) + .topic(topic) + .consumerName("c2") + .subscriptionName(subscriptionName) + .subscriptionType(SubscriptionType.Key_Shared) + .receiverQueueSize(500) // use large receiver queue size + .subscribe(); + + @Cleanup + Consumer c3 = pulsarClient.newConsumer(Schema.INT32) + .topic(topic) + .consumerName("c3") + .subscriptionName(subscriptionName) + .subscriptionType(SubscriptionType.Key_Shared) + .receiverQueueSize(10) + .startPaused(true) // start paused + .subscribe(); + + // find keys that will be assigned to c2 + List keysForC2 = new ArrayList<>(); + for (int i = 0; i < numberOfKeys; i++) { + String key = String.valueOf(i); + byte[] keyBytes = key.getBytes(UTF_8); + int hash = StickyKeyConsumerSelector.makeStickyKeyHash(keyBytes); + if (dispatcher.getSelector().select(hash).consumerName().equals("c2")) { + keysForC2.add(key); + } + } + + Set remainingMessageValues = new HashSet<>(); + // produce messages with keys that all get assigned to c2 + for (int i = 0; i < 1000; i++) { + String key = keysForC2.get(random.nextInt(keysForC2.size())); + //log.info("Producing message with key: {} value: {}", key, i); + producer.newMessage() + .key(key) + .value(i) + .send(); + remainingMessageValues.add(i); + } + + checkLimit.run(); + + Thread.sleep(pauseTime); + checkLimit.run(); + + Thread.sleep(pauseTime); + checkLimit.run(); + + // resume c1 and c3 + c1.resume(); + c3.resume(); + + Thread.sleep(pauseTime); + checkLimit.run(); + + // produce more messages + for (int i = 1000; i < 2000; i++) { + String key = String.valueOf(random.nextInt(numberOfKeys)); + producer.newMessage() + .key(key) + .value(i) + .send(); + remainingMessageValues.add(i); + checkLimit.run(); + } + + // consume the messages + receiveMessages((consumer, msg) -> { + synchronized (this) { + try { + consumer.acknowledge(msg); + } catch (PulsarClientException e) { + throw new RuntimeException(e); + } + remainingMessageValues.remove(msg.getValue()); + checkLimit.run(); + return true; + } + }, Duration.ofSeconds(2), c1, c2, c3); + assertEquals(remainingMessageValues, Collections.emptySet()); + } } From 757013612b68a8ab7b606a7e257b0543b08b7768 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 18 Sep 2024 14:25:36 +0300 Subject: [PATCH 53/58] Reset read ahead limits to defaults after test method --- .../pulsar/client/api/KeySharedSubscriptionTest.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java index 31ec288dba315..ddf7b0f1d5ee2 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java @@ -66,6 +66,7 @@ import org.apache.bookkeeper.mledger.impl.ManagedCursorImpl; import org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl; import org.apache.pulsar.broker.BrokerTestUtil; +import org.apache.pulsar.broker.ServiceConfiguration; import org.apache.pulsar.broker.service.StickyKeyConsumerSelector; import org.apache.pulsar.broker.service.Topic; import org.apache.pulsar.broker.service.nonpersistent.NonPersistentStickyKeyDispatcherMultipleConsumers; @@ -159,6 +160,12 @@ public void resetDefaultNamespace() throws Exception { admin.topics().delete(topicName, false); } } + // reset read ahead limits to defaults + ServiceConfiguration defaultConf = new ServiceConfiguration(); + conf.setKeySharedLookAheadMsgInReplayThresholdPerSubscription( + defaultConf.getKeySharedLookAheadMsgInReplayThresholdPerSubscription()); + conf.setKeySharedLookAheadMsgInReplayThresholdPerConsumer( + defaultConf.getKeySharedLookAheadMsgInReplayThresholdPerConsumer()); } private static final Random random = new Random(System.nanoTime()); From 8804bd00e8131bdcfeda439207e17158af37ddf2 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 18 Sep 2024 14:31:36 +0300 Subject: [PATCH 54/58] Speed up tests using receiveAndAckMessages by consuming in multiple threads --- .../apache/pulsar/broker/BrokerTestUtil.java | 5 ++- .../client/api/ProducerConsumerBase.java | 44 ++++++------------- 2 files changed, 17 insertions(+), 32 deletions(-) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java index a248ee62492e8..5641816ee0b80 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java @@ -27,6 +27,7 @@ import java.util.Arrays; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -208,7 +209,7 @@ public static void receiveMessagesN(BiConsumer, Message> mess * @param consumers the consumers to receive messages from * @param the message value type */ - static void receiveMessagesInThreads(BiFunction, Message, Boolean> messageHandler, + public static void receiveMessagesInThreads(BiFunction, Message, Boolean> messageHandler, final Duration quietTimeout, Consumer... consumers) { FutureUtil.waitForAll(Arrays.stream(consumers).sequential().map(consumer -> { @@ -225,7 +226,7 @@ static void receiveMessagesInThreads(BiFunction, Message, Boo } } } catch (PulsarClientException e) { - throw new RuntimeException(e); + throw new CompletionException(e); } }, runnable -> { Thread thread = new Thread(runnable, "Consumer-" + consumer.getConsumerName()); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/ProducerConsumerBase.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/ProducerConsumerBase.java index ef070250ca1aa..0cf2e49d35bee 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/ProducerConsumerBase.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/ProducerConsumerBase.java @@ -18,15 +18,15 @@ */ package org.apache.pulsar.client.api; +import static org.apache.pulsar.broker.BrokerTestUtil.receiveMessagesInThreads; import com.google.common.collect.Sets; - import java.lang.reflect.Method; +import java.time.Duration; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Random; import java.util.Set; - -import java.util.concurrent.TimeUnit; import java.util.function.BiFunction; import org.apache.commons.lang3.tuple.Pair; import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest; @@ -78,32 +78,16 @@ protected ReceivedMessages receiveAndAckMessages( BiFunction ackPredicate, Consumer...consumers) throws Exception { ReceivedMessages receivedMessages = new ReceivedMessages(); - while (true) { - int receivedMsgCount = 0; - for (int i = 0; i < consumers.length; i++) { - Consumer consumer = consumers[i]; - while (true) { - Message msg = consumer.receive(2, TimeUnit.SECONDS); - if (msg != null) { - receivedMsgCount++; - T v = msg.getValue(); - MessageId messageId = msg.getMessageId(); - receivedMessages.messagesReceived.add(Pair.of(msg.getMessageId(), v)); - if (ackPredicate.apply(messageId, v)) { - consumer.acknowledge(msg); - receivedMessages.messagesAcked.add(Pair.of(msg.getMessageId(), v)); - } - } else { - break; - } - } + receiveMessagesInThreads((consumer, msg) -> { + T v = msg.getValue(); + MessageId messageId = msg.getMessageId(); + receivedMessages.messagesReceived.add(Pair.of(msg.getMessageId(), v)); + if (ackPredicate.apply(messageId, v)) { + consumer.acknowledgeAsync(msg); + receivedMessages.messagesAcked.add(Pair.of(msg.getMessageId(), v)); } - // Because of the possibility of consumers getting stuck with each other, only jump out of the loop if all - // consumers could not receive messages. - if (receivedMsgCount == 0) { - break; - } - } + return true; + }, Duration.ofSeconds(2), consumers); return receivedMessages; } @@ -113,9 +97,9 @@ protected ReceivedMessages ackAllMessages(Consumer...consumers) throws protected static class ReceivedMessages { - List> messagesReceived = new ArrayList<>(); + List> messagesReceived = Collections.synchronizedList(new ArrayList<>()); - List> messagesAcked = new ArrayList<>(); + List> messagesAcked = Collections.synchronizedList(new ArrayList<>()); public boolean hasReceivedMessage(T v) { for (Pair pair : messagesReceived) { From ba596a97f14548d6122e4127cc79664bb6e8ad32 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 18 Sep 2024 14:56:17 +0300 Subject: [PATCH 55/58] Increase the default read ahead limit to 2000/50000 --- conf/broker.conf | 4 ++-- conf/standalone.conf | 4 ++-- .../java/org/apache/pulsar/broker/ServiceConfiguration.java | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/conf/broker.conf b/conf/broker.conf index b9955e5c43c6f..5d997ec0cbb4d 100644 --- a/conf/broker.conf +++ b/conf/broker.conf @@ -362,7 +362,7 @@ maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 # Formula: threshold = min(keySharedLookAheadMsgInReplayThresholdPerConsumer * # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # Setting this value to 0 will disable the limit calculated per consumer. -keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 +keySharedLookAheadMsgInReplayThresholdPerConsumer=2000 # For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer # or a blocked key hash (because of ordering constraints), the broker will continue reading more @@ -372,7 +372,7 @@ keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. # Setting this value to 0 will disable the limit calculated per subscription. -keySharedLookAheadMsgInReplayThresholdPerSubscription=10000 +keySharedLookAheadMsgInReplayThresholdPerSubscription=50000 # Broker periodically checks if subscription is stuck and unblock if flag is enabled. (Default is disabled) unblockStuckSubscriptionEnabled=false diff --git a/conf/standalone.conf b/conf/standalone.conf index db7ee632835be..f421215c7b129 100644 --- a/conf/standalone.conf +++ b/conf/standalone.conf @@ -239,7 +239,7 @@ maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 # Formula: threshold = min(keySharedLookAheadMsgInReplayThresholdPerConsumer * # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # Setting this value to 0 will disable the limit calculated per consumer. -keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 +keySharedLookAheadMsgInReplayThresholdPerConsumer=2000 # For Key_Shared subscriptions, if messages cannot be dispatched to consumers due to a slow consumer # or a blocked key hash (because of ordering constraints), the broker will continue reading more @@ -249,7 +249,7 @@ keySharedLookAheadMsgInReplayThresholdPerConsumer=1000 # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. # Setting this value to 0 will disable the limit calculated per subscription. -keySharedLookAheadMsgInReplayThresholdPerSubscription=10000 +keySharedLookAheadMsgInReplayThresholdPerSubscription=50000 # Tick time to schedule task that checks topic publish rate limiting across all topics # Reducing to lower value can give more accuracy while throttling publish but diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java index dc69e3de865c7..bb68bb3ce2b17 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java @@ -962,7 +962,7 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + "Setting this value to 0 will disable the limit calculated per consumer.", dynamic = true ) - private int keySharedLookAheadMsgInReplayThresholdPerConsumer = 1000; + private int keySharedLookAheadMsgInReplayThresholdPerConsumer = 2000; @FieldContext( category = CATEGORY_POLICIES, @@ -977,7 +977,7 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + "Setting this value to 0 will disable the limit calculated per subscription.\n", dynamic = true ) - private int keySharedLookAheadMsgInReplayThresholdPerSubscription = 10000; + private int keySharedLookAheadMsgInReplayThresholdPerSubscription = 50000; @FieldContext( category = CATEGORY_POLICIES, From 48ff1af7b3ccbc59c2108c1d40c07c3c719b9372 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 18 Sep 2024 15:01:39 +0300 Subject: [PATCH 56/58] Follow the advice of setting to <= 2 * managedLedgerMaxUnackedRangesToPersist --- conf/broker.conf | 2 +- conf/standalone.conf | 2 +- .../java/org/apache/pulsar/broker/ServiceConfiguration.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/conf/broker.conf b/conf/broker.conf index 5d997ec0cbb4d..74130d709cdd2 100644 --- a/conf/broker.conf +++ b/conf/broker.conf @@ -372,7 +372,7 @@ keySharedLookAheadMsgInReplayThresholdPerConsumer=2000 # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. # Setting this value to 0 will disable the limit calculated per subscription. -keySharedLookAheadMsgInReplayThresholdPerSubscription=50000 +keySharedLookAheadMsgInReplayThresholdPerSubscription=20000 # Broker periodically checks if subscription is stuck and unblock if flag is enabled. (Default is disabled) unblockStuckSubscriptionEnabled=false diff --git a/conf/standalone.conf b/conf/standalone.conf index f421215c7b129..622949bf6c325 100644 --- a/conf/standalone.conf +++ b/conf/standalone.conf @@ -249,7 +249,7 @@ keySharedLookAheadMsgInReplayThresholdPerConsumer=2000 # connected consumer count, keySharedLookAheadMsgInReplayThresholdPerSubscription). # This value should be set to a value less than 2 * managedLedgerMaxUnackedRangesToPersist. # Setting this value to 0 will disable the limit calculated per subscription. -keySharedLookAheadMsgInReplayThresholdPerSubscription=50000 +keySharedLookAheadMsgInReplayThresholdPerSubscription=20000 # Tick time to schedule task that checks topic publish rate limiting across all topics # Reducing to lower value can give more accuracy while throttling publish but diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java index bb68bb3ce2b17..42dc959426692 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java @@ -977,7 +977,7 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + "Setting this value to 0 will disable the limit calculated per subscription.\n", dynamic = true ) - private int keySharedLookAheadMsgInReplayThresholdPerSubscription = 50000; + private int keySharedLookAheadMsgInReplayThresholdPerSubscription = 20000; @FieldContext( category = CATEGORY_POLICIES, From 57eaa77f4b64bb0fe2602c417f539617ab31f79d Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 18 Sep 2024 15:36:57 +0300 Subject: [PATCH 57/58] Use max unacked messages limits when look ahead limits are disabled --- ...istentStickyKeyDispatcherMultipleConsumers.java | 11 ++++++++++- .../persistent/KeySharedLookAheadConfigTest.java | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java index 4ac02208d73f5..d45b9394dd744 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java @@ -408,7 +408,16 @@ static int getEffectiveLookAheadLimit(ServiceConfiguration serviceConfig, int co } } if (effectiveLimit <= 0) { - effectiveLimit = Integer.MAX_VALUE; + // use max unacked messages limits if key shared look-ahead limits are disabled + int maxUnackedMessagesPerSubscription = serviceConfig.getMaxUnackedMessagesPerSubscription(); + if (maxUnackedMessagesPerSubscription <= 0) { + maxUnackedMessagesPerSubscription = Integer.MAX_VALUE; + } + int maxUnackedMessagesByConsumers = consumerCount * serviceConfig.getMaxUnackedMessagesPerConsumer(); + if (maxUnackedMessagesByConsumers <= 0) { + maxUnackedMessagesByConsumers = Integer.MAX_VALUE; + } + effectiveLimit = Math.min(maxUnackedMessagesPerSubscription, maxUnackedMessagesByConsumers); } return effectiveLimit; } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/KeySharedLookAheadConfigTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/KeySharedLookAheadConfigTest.java index 466e492df718e..cf028cf369d7b 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/KeySharedLookAheadConfigTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/KeySharedLookAheadConfigTest.java @@ -44,6 +44,20 @@ public void testGetEffectiveLookAheadLimit() { config.setKeySharedLookAheadMsgInReplayThresholdPerConsumer(0); config.setKeySharedLookAheadMsgInReplayThresholdPerSubscription(0); + config.setMaxUnackedMessagesPerConsumer(0); + config.setMaxUnackedMessagesPerSubscription(0); assertEquals(getEffectiveLookAheadLimit(config, 100), Integer.MAX_VALUE); + + config.setKeySharedLookAheadMsgInReplayThresholdPerConsumer(0); + config.setKeySharedLookAheadMsgInReplayThresholdPerSubscription(0); + config.setMaxUnackedMessagesPerConsumer(1); + config.setMaxUnackedMessagesPerSubscription(10); + assertEquals(getEffectiveLookAheadLimit(config, 100), 10); + + config.setKeySharedLookAheadMsgInReplayThresholdPerConsumer(0); + config.setKeySharedLookAheadMsgInReplayThresholdPerSubscription(0); + config.setMaxUnackedMessagesPerConsumer(22); + config.setMaxUnackedMessagesPerSubscription(0); + assertEquals(getEffectiveLookAheadLimit(config, 100), 2200); } } \ No newline at end of file From c9e72c45bdc00525e83979b17433fa1f8e65ef39 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Wed, 18 Sep 2024 19:48:45 +0300 Subject: [PATCH 58/58] Fix flaky test --- .../admin/v3/AdminApiTransactionTest.java | 79 ++++++++++--------- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java index 1cc20b04c2137..e32af29c7e962 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java @@ -626,21 +626,23 @@ public void testGetTransactionBufferInternalStats() throws Exception { producer.newMessage(transaction).send(); transaction.abort().get(); - // Get transaction buffer internal stats and verify single snapshot stats - TransactionBufferInternalStats stats = admin.transactions() - .getTransactionBufferInternalStatsAsync(topic2, true).get(); - assertEquals(stats.snapshotType, AbortedTxnProcessor.SnapshotType.Single.toString()); - assertNotNull(stats.singleSnapshotSystemTopicInternalStats); - - // Get managed ledger internal stats for the transaction buffer snapshot topic - PersistentTopicInternalStats internalStats = admin.topics().getInternalStats( - TopicName.get(topic2).getNamespace() + "/" + SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT); - verifyManagedLedgerInternalStats(stats.singleSnapshotSystemTopicInternalStats.managedLedgerInternalStats, - internalStats); - assertTrue(stats.singleSnapshotSystemTopicInternalStats.managedLedgerName - .contains(SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT)); - assertNull(stats.segmentInternalStats); - assertNull(stats.segmentIndexInternalStats); + Awaitility.await().untilAsserted(() -> { + // Get transaction buffer internal stats and verify single snapshot stats + TransactionBufferInternalStats stats = admin.transactions() + .getTransactionBufferInternalStatsAsync(topic2, true).get(); + assertEquals(stats.snapshotType, AbortedTxnProcessor.SnapshotType.Single.toString()); + assertNotNull(stats.singleSnapshotSystemTopicInternalStats); + + // Get managed ledger internal stats for the transaction buffer snapshot topic + PersistentTopicInternalStats internalStats = admin.topics().getInternalStats( + TopicName.get(topic2).getNamespace() + "/" + SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT); + verifyManagedLedgerInternalStats(stats.singleSnapshotSystemTopicInternalStats.managedLedgerInternalStats, + internalStats); + assertTrue(stats.singleSnapshotSystemTopicInternalStats.managedLedgerName + .contains(SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT)); + assertNull(stats.segmentInternalStats); + assertNull(stats.segmentIndexInternalStats); + }); // Configure segmented snapshot and set segment size pulsar.getConfig().setTransactionBufferSnapshotSegmentSize(9); @@ -652,28 +654,31 @@ public void testGetTransactionBufferInternalStats() throws Exception { producer.newMessage(transaction).send(); transaction.abort().get(); - // Get transaction buffer internal stats and verify segmented snapshot stats - stats = admin.transactions().getTransactionBufferInternalStatsAsync(topic3, true).get(); - assertEquals(stats.snapshotType, AbortedTxnProcessor.SnapshotType.Segment.toString()); - assertNull(stats.singleSnapshotSystemTopicInternalStats); - assertNotNull(stats.segmentInternalStats); - - // Get managed ledger internal stats for the transaction buffer segments topic - internalStats = admin.topics().getInternalStats( - TopicName.get(topic2).getNamespace() + "/" + - SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT_SEGMENTS); - verifyManagedLedgerInternalStats(stats.segmentInternalStats.managedLedgerInternalStats, internalStats); - assertTrue(stats.segmentInternalStats.managedLedgerName - .contains(SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT_SEGMENTS)); - - // Get managed ledger internal stats for the transaction buffer indexes topic - assertNotNull(stats.segmentIndexInternalStats); - internalStats = admin.topics().getInternalStats( - TopicName.get(topic2).getNamespace() + "/" + - SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT_INDEXES); - verifyManagedLedgerInternalStats(stats.segmentIndexInternalStats.managedLedgerInternalStats, internalStats); - assertTrue(stats.segmentIndexInternalStats.managedLedgerName - .contains(SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT_INDEXES)); + Awaitility.await().untilAsserted(() -> { + // Get transaction buffer internal stats and verify segmented snapshot stats + TransactionBufferInternalStats stats = + admin.transactions().getTransactionBufferInternalStatsAsync(topic3, true).get(); + assertEquals(stats.snapshotType, AbortedTxnProcessor.SnapshotType.Segment.toString()); + assertNull(stats.singleSnapshotSystemTopicInternalStats); + assertNotNull(stats.segmentInternalStats); + + // Get managed ledger internal stats for the transaction buffer segments topic + PersistentTopicInternalStats internalStats = admin.topics().getInternalStats( + TopicName.get(topic2).getNamespace() + "/" + + SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT_SEGMENTS); + verifyManagedLedgerInternalStats(stats.segmentInternalStats.managedLedgerInternalStats, internalStats); + assertTrue(stats.segmentInternalStats.managedLedgerName + .contains(SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT_SEGMENTS)); + + // Get managed ledger internal stats for the transaction buffer indexes topic + assertNotNull(stats.segmentIndexInternalStats); + internalStats = admin.topics().getInternalStats( + TopicName.get(topic2).getNamespace() + "/" + + SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT_INDEXES); + verifyManagedLedgerInternalStats(stats.segmentIndexInternalStats.managedLedgerInternalStats, internalStats); + assertTrue(stats.segmentIndexInternalStats.managedLedgerName + .contains(SystemTopicNames.TRANSACTION_BUFFER_SNAPSHOT_INDEXES)); + }); }