From 646d6ba6f1be44ad6a8f3934bf69075899324d7b Mon Sep 17 00:00:00 2001 From: qianye Date: Tue, 16 Jun 2026 20:53:22 +0800 Subject: [PATCH 01/10] [ISSUE #10489] Support BatchChangeInvisibleTime to reduce network round-trips and RocksDB flushes --- .../rocketmq/broker/BrokerController.java | 2 + .../rocketmq/broker/pop/PopConsumerCache.java | 146 +++++++-- .../broker/pop/PopConsumerKVStore.java | 7 + .../broker/pop/PopConsumerRocksdbStore.java | 18 ++ .../broker/pop/PopConsumerService.java | 114 +++++++ .../ChangeInvisibleTimeProcessor.java | 265 +++++++++++++++- .../broker/pop/PopConsumerCacheTest.java | 220 ++++++++++++- .../pop/PopConsumerRocksdbStoreTest.java | 65 +++- .../broker/pop/PopConsumerServiceTest.java | 40 ++- .../ChangeInvisibleTimeProcessorTest.java | 145 +++++++++ .../rocketmq/client/impl/MQClientAPIImpl.java | 88 ++++++ .../client/impl/mqclient/MQClientAPIExt.java | 11 + .../client/impl/MQClientAPIImplTest.java | 141 +++++++++ .../apache/rocketmq/common/BrokerConfig.java | 9 + .../rocketmq/common/PopAckConstants.java | 1 + .../rocketmq/proxy/common/RenewEvent.java | 26 ++ .../rocketmq/proxy/config/ProxyConfig.java | 19 ++ .../ReceiveMessageResponseStreamWriter.java | 76 ++++- .../BatchChangeInvisibleTimeResult.java | 50 +++ .../proxy/processor/ConsumerProcessor.java | 194 +++++++++++- .../processor/DefaultMessagingProcessor.java | 8 + .../proxy/processor/MessagingProcessor.java | 33 ++ .../processor/ReceiptHandleProcessor.java | 87 +++++- .../message/ClusterMessageService.java | 30 ++ .../service/message/LocalMessageService.java | 81 +++++ .../proxy/service/message/MessageService.java | 16 + .../service/message/ReceiptHandleMessage.java | 10 + .../receipt/DefaultReceiptHandleManager.java | 170 +++++++++- ...eceiveMessageResponseStreamWriterTest.java | 113 ++++++- .../processor/ConsumerProcessorTest.java | 294 ++++++++++++++++++ .../processor/ReceiptHandleProcessorTest.java | 167 ++++++++++ .../remoting/protocol/RequestCode.java | 1 + .../BatchChangeInvisibleTimeRequestBody.java | 33 ++ .../BatchChangeInvisibleTimeResponseBody.java | 33 ++ .../body/ChangeInvisibleTimeRequestEntry.java | 93 ++++++ .../ChangeInvisibleTimeResponseEntry.java | 57 ++++ ...BatchChangeInvisibleTimeRequestHeader.java | 68 ++++ .../body/BatchChangeInvisibleTimeTest.java | 94 ++++++ .../test/client/rmq/RMQPopClient.java | 30 ++ .../pop/BatchChangeInvisibleTimeIT.java | 171 ++++++++++ 40 files changed, 3159 insertions(+), 67 deletions(-) create mode 100644 proxy/src/main/java/org/apache/rocketmq/proxy/processor/BatchChangeInvisibleTimeResult.java create mode 100644 remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeRequestBody.java create mode 100644 remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeResponseBody.java create mode 100644 remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeRequestEntry.java create mode 100644 remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeResponseEntry.java create mode 100644 remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/BatchChangeInvisibleTimeRequestHeader.java create mode 100644 remoting/src/test/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeTest.java create mode 100644 test/src/test/java/org/apache/rocketmq/test/client/consumer/pop/BatchChangeInvisibleTimeIT.java diff --git a/broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java b/broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java index 8e2954d8ff0..b89d2db88e2 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java @@ -1207,6 +1207,8 @@ public void registerProcessor() { */ remotingServer.registerProcessor(RequestCode.CHANGE_MESSAGE_INVISIBLETIME, this.changeInvisibleTimeProcessor, this.ackMessageExecutor); fastRemotingServer.registerProcessor(RequestCode.CHANGE_MESSAGE_INVISIBLETIME, this.changeInvisibleTimeProcessor, this.ackMessageExecutor); + remotingServer.registerProcessor(RequestCode.BATCH_CHANGE_MESSAGE_INVISIBLETIME, this.changeInvisibleTimeProcessor, this.ackMessageExecutor); + fastRemotingServer.registerProcessor(RequestCode.BATCH_CHANGE_MESSAGE_INVISIBLETIME, this.changeInvisibleTimeProcessor, this.ackMessageExecutor); /** * notificationProcessor */ diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java index c74c5793a5c..238524e83ca 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java @@ -29,6 +29,7 @@ import org.apache.rocketmq.broker.BrokerController; import org.apache.rocketmq.broker.offset.ConsumerOffsetManager; import org.apache.rocketmq.common.BrokerConfig; +import org.apache.rocketmq.common.KeyBuilder; import org.apache.rocketmq.common.ServiceThread; import org.apache.rocketmq.common.constant.LoggerName; import org.apache.rocketmq.common.utils.ConcurrentHashMapUtils; @@ -116,6 +117,68 @@ public List deleteRecords(List consumerRec return remain; } + public void writeAndDeleteRecords(List writeRecordList, + List deleteRecordList) { + if (deleteRecordList.isEmpty()) { + consumerRecordStore.writeAndDeleteRecords(writeRecordList, deleteRecordList); + return; + } + + PopConsumerRecord lockRecord = validateAndGetLockRecord(writeRecordList, deleteRecordList); + String lockTopic = KeyBuilder.parseNormalTopic(lockRecord.getTopicId(), lockRecord.getGroupId()); + while (!consumerLockService.tryLock(lockRecord.getGroupId(), lockTopic)) { + Thread.yield(); + } + try { + List storeDeleteRecords = new ArrayList<>(deleteRecordList.size()); + List bufferDeleteRecords = new ArrayList<>(deleteRecordList.size()); + deleteRecordList.forEach(consumerRecord -> { + ConsumerRecords consumerRecords = consumerRecordTable.get(this.getKey(consumerRecord)); + if (consumerRecords != null && consumerRecords.contains(consumerRecord)) { + bufferDeleteRecords.add(consumerRecord); + } else { + storeDeleteRecords.add(consumerRecord); + } + }); + + consumerRecordStore.writeAndDeleteRecords(writeRecordList, storeDeleteRecords); + + int deleted = 0; + for (PopConsumerRecord consumerRecord : bufferDeleteRecords) { + ConsumerRecords consumerRecords = consumerRecordTable.get(this.getKey(consumerRecord)); + if (consumerRecords != null && consumerRecords.delete(consumerRecord)) { + deleted++; + } + } + this.estimateCacheSize.addAndGet(-deleted); + } finally { + consumerLockService.unlock(lockRecord.getGroupId(), lockTopic); + } + } + + private PopConsumerRecord validateAndGetLockRecord(List writeRecordList, + List deleteRecordList) { + PopConsumerRecord lockRecord = null; + lockRecord = validateAndGetLockRecord(lockRecord, deleteRecordList); + return validateAndGetLockRecord(lockRecord, writeRecordList); + } + + private PopConsumerRecord validateAndGetLockRecord(PopConsumerRecord expectedRecord, + List consumerRecordList) { + PopConsumerRecord lockRecord = expectedRecord; + for (PopConsumerRecord consumerRecord : consumerRecordList) { + if (lockRecord == null) { + lockRecord = consumerRecord; + continue; + } + if (!lockRecord.getGroupId().equals(consumerRecord.getGroupId()) || + !lockRecord.getTopicId().equals(consumerRecord.getTopicId())) { + throw new IllegalArgumentException("batch write and delete records must use the same consumer"); + } + } + return lockRecord; + } + public int cleanupRecords(Consumer consumer) { int remain = 0; Iterator> iterator = consumerRecordTable.entrySet().iterator(); @@ -124,44 +187,59 @@ public int cleanupRecords(Consumer consumer) { ConsumerRecords records = iterator.next().getValue(); boolean timeout = consumerLockService.isLockTimeout( records.getGroupId(), records.getTopicId()); - - if (timeout) { - records.stageExpiredRecords(Long.MAX_VALUE); - List writeConsumerRecords = - new ArrayList<>(records.getRemoveTreeMap().values()); - if (!writeConsumerRecords.isEmpty()) { - consumerRecordStore.writeRecords(writeConsumerRecords); - } - records.clearStagedRecords(); - log.info("PopConsumerOffline, so clean expire records, groupId={}, topic={}, queueId={}, records={}", - records.getGroupId(), records.getTopicId(), records.getQueueId(), records.getInFlightRecordCount()); - iterator.remove(); + String lockTopic = KeyBuilder.parseNormalTopic(records.getTopicId(), records.getGroupId()); + if (!consumerLockService.tryLock(records.getGroupId(), lockTopic)) { continue; } - - long currentTime = System.currentTimeMillis(); - records.stageExpiredRecords(currentTime); - List writeConsumerRecords = new ArrayList<>(); - records.getRemoveTreeMap().values().forEach(record -> { - if (record.getVisibilityTimeout() <= currentTime) { - consumer.accept(record); - } else { - writeConsumerRecords.add(record); + try { + if (timeout) { + records.stageExpiredRecords(Long.MAX_VALUE); + List writeConsumerRecords = + new ArrayList<>(records.getRemoveTreeMap().values()); + if (!writeConsumerRecords.isEmpty()) { + consumerRecordStore.writeRecords(writeConsumerRecords); + } + records.clearStagedRecords(); + log.info("PopConsumerOffline, so clean expire records, groupId={}, topic={}, queueId={}, records={}", + records.getGroupId(), records.getTopicId(), records.getQueueId(), records.getInFlightRecordCount()); + iterator.remove(); + continue; } - }); - // write to store and handle it later - consumerRecordStore.writeRecords(writeConsumerRecords); - records.clearStagedRecords(); + long currentTime = System.currentTimeMillis(); + records.stageExpiredRecords(currentTime); + List writeConsumerRecords = new ArrayList<>(); + records.getRemoveTreeMap().values().forEach(record -> { + if (record.getVisibilityTimeout() <= currentTime) { + consumer.accept(record); + } else { + writeConsumerRecords.add(record); + } + }); + + // write to store and handle it later + consumerRecordStore.writeRecords(writeConsumerRecords); + records.clearStagedRecords(); - // commit min offset in buffer to offset store - long offset = records.getMinOffsetInBuffer(); - if (offset > OFFSET_NOT_EXIST) { - this.commitOffset("PopConsumerCache", - records.getGroupId(), records.getTopicId(), records.getQueueId(), offset); - } + // commit min offset in buffer to offset store + long offset = records.getMinOffsetInBuffer(); + if (offset > OFFSET_NOT_EXIST) { + ConsumerOffsetManager consumerOffsetManager = brokerController.getConsumerOffsetManager(); + long commit = consumerOffsetManager.queryOffset(records.getGroupId(), records.getTopicId(), + records.getQueueId()); + if (commit != OFFSET_NOT_EXIST && offset < commit) { + log.info("PopConsumerCache, consumer offset less than store, " + + "groupId={}, topicId={}, queueId={}, offset={}", records.getGroupId(), + records.getTopicId(), records.getQueueId(), offset); + } + consumerOffsetManager.commitOffset("PopConsumerCache", records.getGroupId(), + records.getTopicId(), records.getQueueId(), offset); + } - remain += records.getInFlightRecordCount(); + remain += records.getInFlightRecordCount(); + } finally { + consumerLockService.unlock(records.getGroupId(), lockTopic); + } } return remain; } @@ -231,6 +309,10 @@ public boolean delete(PopConsumerRecord record) { return recordTreeMap.remove(record.getOffset()) != null; } + public boolean contains(PopConsumerRecord record) { + return recordTreeMap.containsKey(record.getOffset()); + } + public long getMinOffsetInBuffer() { Map.Entry entry = removeTreeMap.firstEntry(); if (entry != null) { diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerKVStore.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerKVStore.java index 33072d699b5..208b8ea85c6 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerKVStore.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerKVStore.java @@ -48,6 +48,13 @@ public interface PopConsumerKVStore { */ void deleteRecords(List consumerRecordList); + /** + * Writes and deletes consumer records from the storage in a single batch. + * @param writeRecordList The list of consumer records to be written. + * @param deleteRecordList The list of consumer records to be deleted. + */ + void writeAndDeleteRecords(List writeRecordList, List deleteRecordList); + /** * Scans and returns a list of expired consumer records within the specified time range. * @param lowerTime The start time (inclusive) of the time range to search, in milliseconds. diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerRocksdbStore.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerRocksdbStore.java index dc68f9d9fe5..da9067fd51d 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerRocksdbStore.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerRocksdbStore.java @@ -139,6 +139,24 @@ public void deleteRecords(List consumerRecordList) { } } + @Override + public void writeAndDeleteRecords(List writeRecordList, + List deleteRecordList) { + if (!writeRecordList.isEmpty() || !deleteRecordList.isEmpty()) { + try (WriteBatch writeBatch = new WriteBatch()) { + for (PopConsumerRecord record : deleteRecordList) { + writeBatch.delete(columnFamilyHandle, record.getKeyBytes()); + } + for (PopConsumerRecord record : writeRecordList) { + writeBatch.put(columnFamilyHandle, record.getKeyBytes(), record.getValueBytes()); + } + this.db.write(writeOptions, writeBatch); + } catch (RocksDBException e) { + throw new RuntimeException("Write and delete record error", e); + } + } + } + @Override // https://github.com/facebook/rocksdb/issues/10300 public List scanExpiredRecords(long lower, long upper, int maxCount) { diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java index 9ab5eb651be..36c3033f61e 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java @@ -535,6 +535,120 @@ public void changeInvisibilityDuration(long popTime, long invisibleTime, long ch } } + public void batchChangeInvisibilityDuration(List changeRecords) { + if (changeRecords == null || changeRecords.isEmpty()) { + return; + } + + List ckRecords = new ArrayList<>(changeRecords.size()); + List ackRecords = new ArrayList<>(changeRecords.size()); + List storeAckRecords = new ArrayList<>(changeRecords.size()); + + for (ChangeInvisibilityRecord changeRecord : changeRecords) { + if (brokerConfig.isPopConsumerKVServiceLog()) { + log.info("PopConsumerService batch change, time={}, invisible={}, " + + "groupId={}, topic={}, queueId={}, offset={}, new time={}, new invisible={}", + changeRecord.getPopTime(), changeRecord.getInvisibleTime(), changeRecord.getGroupId(), + changeRecord.getTopicId(), changeRecord.getQueueId(), changeRecord.getOffset(), + changeRecord.getChangedPopTime(), changeRecord.getChangedInvisibleTime()); + } + + PopConsumerRecord ckRecord = new PopConsumerRecord( + changeRecord.getChangedPopTime(), changeRecord.getGroupId(), changeRecord.getTopicId(), + changeRecord.getQueueId(), 0, changeRecord.getChangedInvisibleTime(), changeRecord.getOffset(), + null, changeRecord.isSuspend()); + + PopConsumerRecord ackRecord = new PopConsumerRecord( + changeRecord.getPopTime(), changeRecord.getGroupId(), changeRecord.getTopicId(), + changeRecord.getQueueId(), 0, changeRecord.getInvisibleTime(), changeRecord.getOffset(), + null, changeRecord.isSuspend()); + + boolean skipWrite = brokerConfig.isPopReviveSkipIfGroupAbsent() && + !brokerController.getSubscriptionGroupManager().containsSubscriptionGroup(changeRecord.getGroupId()); + + if (skipWrite) { + log.info("PopConsumerService batch change invisibility skip, time={}, " + + "groupId={}, topicId={}, queueId={}, offset={}", changeRecord.getPopTime(), + changeRecord.getGroupId(), changeRecord.getTopicId(), changeRecord.getQueueId(), + changeRecord.getOffset()); + } else { + ckRecords.add(ckRecord); + } + + ackRecords.add(ackRecord); + if (skipWrite || ckRecord.getVisibilityTimeout() != ackRecord.getVisibilityTimeout()) { + storeAckRecords.add(ackRecord); + } + } + + if (brokerConfig.isEnablePopBufferMerge() && popConsumerCache != null) { + popConsumerCache.writeAndDeleteRecords(ckRecords, ackRecords); + } else { + this.popConsumerStore.writeAndDeleteRecords(ckRecords, storeAckRecords); + } + } + + public static class ChangeInvisibilityRecord { + private final long popTime; + private final long invisibleTime; + private final long changedPopTime; + private final long changedInvisibleTime; + private final String groupId; + private final String topicId; + private final int queueId; + private final long offset; + private final boolean suspend; + + public ChangeInvisibilityRecord(long popTime, long invisibleTime, long changedPopTime, + long changedInvisibleTime, String groupId, String topicId, int queueId, long offset, boolean suspend) { + this.popTime = popTime; + this.invisibleTime = invisibleTime; + this.changedPopTime = changedPopTime; + this.changedInvisibleTime = changedInvisibleTime; + this.groupId = groupId; + this.topicId = topicId; + this.queueId = queueId; + this.offset = offset; + this.suspend = suspend; + } + + public long getPopTime() { + return popTime; + } + + public long getInvisibleTime() { + return invisibleTime; + } + + public long getChangedPopTime() { + return changedPopTime; + } + + public long getChangedInvisibleTime() { + return changedInvisibleTime; + } + + public String getGroupId() { + return groupId; + } + + public String getTopicId() { + return topicId; + } + + public int getQueueId() { + return queueId; + } + + public long getOffset() { + return offset; + } + + public boolean isSuspend() { + return suspend; + } + } + // Use broker escape bridge to support remote read public CompletableFuture> getMessageAsync(PopConsumerRecord consumerRecord) { return this.brokerController.getEscapeBridge().getMessageAsync(consumerRecord.getTopicId(), diff --git a/broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java b/broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java index 5ff132ca237..9075d05a575 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java @@ -20,12 +20,17 @@ import io.netty.channel.Channel; import io.netty.channel.ChannelHandlerContext; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import org.apache.commons.lang3.StringUtils; import org.apache.rocketmq.broker.BrokerController; import org.apache.rocketmq.broker.offset.ConsumerOffsetManager; import org.apache.rocketmq.broker.offset.MemoryConsumerOrderInfoManager; +import org.apache.rocketmq.broker.pop.PopConsumerService.ChangeInvisibilityRecord; import org.apache.rocketmq.broker.pop.PopConsumerLockService; import org.apache.rocketmq.broker.pop.orderly.ConsumerOrderInfoManager; import org.apache.rocketmq.common.PopAckConstants; @@ -43,7 +48,13 @@ import org.apache.rocketmq.remoting.netty.NettyRemotingAbstract; import org.apache.rocketmq.remoting.netty.NettyRequestProcessor; import org.apache.rocketmq.remoting.protocol.RemotingCommand; +import org.apache.rocketmq.remoting.protocol.RequestCode; import org.apache.rocketmq.remoting.protocol.ResponseCode; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeRequestBody; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeResponseBody; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeResponseEntry; +import org.apache.rocketmq.remoting.protocol.header.BatchChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeResponseHeader; import org.apache.rocketmq.remoting.protocol.header.ExtraInfoUtil; @@ -104,10 +115,19 @@ private RemotingCommand processRequest(final Channel channel, RemotingCommand re public CompletableFuture processRequestAsync(final Channel channel, RemotingCommand request, boolean brokerAllowSuspend) throws RemotingCommandException { - final ChangeInvisibleTimeRequestHeader requestHeader = (ChangeInvisibleTimeRequestHeader) request.decodeCommandCustomHeader(ChangeInvisibleTimeRequestHeader.class); + if (request.getCode() == RequestCode.BATCH_CHANGE_MESSAGE_INVISIBLETIME) { + return processBatchRequestAsync(channel, request, brokerAllowSuspend); + } + final ChangeInvisibleTimeRequestHeader requestHeader = + (ChangeInvisibleTimeRequestHeader) request.decodeCommandCustomHeader(ChangeInvisibleTimeRequestHeader.class); + return processSingleRequestAsync(channel, requestHeader, request.getOpaque(), brokerAllowSuspend); + } + + protected CompletableFuture processSingleRequestAsync(final Channel channel, + ChangeInvisibleTimeRequestHeader requestHeader, int opaque, boolean brokerAllowSuspend) throws RemotingCommandException { RemotingCommand response = RemotingCommand.createResponseCommand(ChangeInvisibleTimeResponseHeader.class); response.setCode(ResponseCode.SUCCESS); - response.setOpaque(request.getOpaque()); + response.setOpaque(opaque); final ChangeInvisibleTimeResponseHeader responseHeader = (ChangeInvisibleTimeResponseHeader) response.readCustomHeader(); TopicConfig topicConfig = this.brokerController.getTopicConfigManager().selectTopicConfig(requestHeader.getTopic()); if (null == topicConfig) { @@ -186,6 +206,247 @@ public CompletableFuture processRequestAsync(final Channel chan }); } + protected CompletableFuture processBatchRequestAsync(final Channel channel, + RemotingCommand request, boolean brokerAllowSuspend) { + RemotingCommand response = RemotingCommand.createResponseCommand(null); + response.setCode(ResponseCode.SUCCESS); + response.setOpaque(request.getOpaque()); + + BatchChangeInvisibleTimeRequestHeader batchRequestHeader; + try { + batchRequestHeader = (BatchChangeInvisibleTimeRequestHeader) + request.decodeCommandCustomHeader(BatchChangeInvisibleTimeRequestHeader.class); + } catch (Throwable t) { + response.setCode(ResponseCode.MESSAGE_ILLEGAL); + response.setRemark("batch change invisible time request header is invalid"); + return CompletableFuture.completedFuture(response); + } + + BatchChangeInvisibleTimeRequestBody requestBody = + BatchChangeInvisibleTimeRequestBody.decode(request.getBody(), BatchChangeInvisibleTimeRequestBody.class); + List requestEntries = requestBody == null || requestBody.getEntries() == null ? + Collections.emptyList() : requestBody.getEntries(); + int batchMaxNum = Math.max(1, brokerController.getBrokerConfig().getBatchChangeInvisibleTimeMaxNum()); + if (requestEntries.size() > batchMaxNum) { + response.setCode(ResponseCode.MESSAGE_ILLEGAL); + response.setRemark(String.format("batch change invisible time entries exceed limit: %d", + batchMaxNum)); + return CompletableFuture.completedFuture(response); + } + + ChangeInvisibleTimeResponseEntry[] responseEntries = new ChangeInvisibleTimeResponseEntry[requestEntries.size()]; + for (int i = 0; i < requestEntries.size(); i++) { + responseEntries[i] = buildFailedResponseEntry(ResponseCode.SYSTEM_ERROR); + } + List> futures = new ArrayList<>(); + + if (!prepareBatchRequestEntries(batchRequestHeader, requestEntries)) { + response.setCode(ResponseCode.MESSAGE_ILLEGAL); + response.setRemark("batch change invisible time entries must use the same topic and consumerGroup as request header"); + return CompletableFuture.completedFuture(response); + } + + if (brokerController.getBrokerConfig().isPopConsumerKVServiceEnable()) { + List kvChangeRecords = new ArrayList<>(); + List kvIndexes = new ArrayList<>(); + List kvSuccessEntries = new ArrayList<>(); + for (int i = 0; i < requestEntries.size(); i++) { + ChangeInvisibleTimeRequestEntry requestEntry = requestEntries.get(i); + if (tryAppendBatchKvChange(channel, requestEntry, i, responseEntries, kvChangeRecords, + kvIndexes, kvSuccessEntries)) { + continue; + } + appendSingleBatchEntry(channel, requestEntry, request.getOpaque(), brokerAllowSuspend, + responseEntries, futures, i); + } + + if (!kvChangeRecords.isEmpty()) { + try { + brokerController.getPopConsumerService().batchChangeInvisibilityDuration(kvChangeRecords); + for (int i = 0; i < kvIndexes.size(); i++) { + responseEntries[kvIndexes.get(i)] = kvSuccessEntries.get(i); + } + } catch (Throwable t) { + POP_LOGGER.error("batch change invisibility duration failed", t); + for (Integer index : kvIndexes) { + responseEntries[index] = buildFailedResponseEntry(ResponseCode.SYSTEM_ERROR); + } + } + } + } else { + for (int i = 0; i < requestEntries.size(); i++) { + appendSingleBatchEntry(channel, requestEntries.get(i), request.getOpaque(), brokerAllowSuspend, + responseEntries, futures, i); + } + } + + return CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).thenApply(ignored -> { + BatchChangeInvisibleTimeResponseBody responseBody = new BatchChangeInvisibleTimeResponseBody(); + responseBody.setEntries(Arrays.asList(responseEntries)); + response.setBody(responseBody.encode()); + return response; + }); + } + + protected boolean prepareBatchRequestEntries(BatchChangeInvisibleTimeRequestHeader batchRequestHeader, + List requestEntries) { + if (batchRequestHeader == null || + StringUtils.isBlank(batchRequestHeader.getConsumerGroup()) || + StringUtils.isBlank(batchRequestHeader.getTopic())) { + return false; + } + + for (ChangeInvisibleTimeRequestEntry requestEntry : requestEntries) { + if (requestEntry == null) { + continue; + } + if (StringUtils.isBlank(requestEntry.getConsumerGroup())) { + requestEntry.setConsumerGroup(batchRequestHeader.getConsumerGroup()); + } + if (StringUtils.isBlank(requestEntry.getTopic())) { + requestEntry.setTopic(batchRequestHeader.getTopic()); + } + if (!StringUtils.equals(batchRequestHeader.getConsumerGroup(), requestEntry.getConsumerGroup()) || + !StringUtils.equals(batchRequestHeader.getTopic(), requestEntry.getTopic())) { + return false; + } + } + return true; + } + + protected boolean tryAppendBatchKvChange(final Channel channel, ChangeInvisibleTimeRequestEntry requestEntry, int index, + ChangeInvisibleTimeResponseEntry[] responseEntries, List kvChangeRecords, + List kvIndexes, List kvSuccessEntries) { + if (requestEntry == null) { + responseEntries[index] = buildFailedResponseEntry(ResponseCode.SYSTEM_ERROR); + return true; + } + if (StringUtils.isNotBlank(requestEntry.getLiteTopic())) { + return false; + } + + try { + String[] extraInfo = ExtraInfoUtil.split(requestEntry.getExtraInfo()); + if (ExtraInfoUtil.isOrder(extraInfo)) { + return false; + } + + ChangeInvisibleTimeResponseEntry failedEntry = validateBatchKvEntry(channel, requestEntry); + if (failedEntry != null) { + responseEntries[index] = failedEntry; + return true; + } + + long current = System.currentTimeMillis(); + kvChangeRecords.add(new ChangeInvisibilityRecord( + ExtraInfoUtil.getPopTime(extraInfo), ExtraInfoUtil.getInvisibleTime(extraInfo), current, + requestEntry.getInvisibleTime(), requestEntry.getConsumerGroup(), requestEntry.getTopic(), + requestEntry.getQueueId(), requestEntry.getOffset(), requestEntry.isSuspend())); + ChangeInvisibleTimeResponseEntry successEntry = new ChangeInvisibleTimeResponseEntry(); + successEntry.setCode(ResponseCode.SUCCESS); + successEntry.setPopTime(current); + successEntry.setInvisibleTime(requestEntry.getInvisibleTime()); + successEntry.setReviveQid(ExtraInfoUtil.getReviveQid(extraInfo)); + kvIndexes.add(index); + kvSuccessEntries.add(successEntry); + return true; + } catch (Throwable t) { + responseEntries[index] = buildFailedResponseEntry(ResponseCode.SYSTEM_ERROR); + return true; + } + } + + protected void appendSingleBatchEntry(final Channel channel, ChangeInvisibleTimeRequestEntry requestEntry, int opaque, + boolean brokerAllowSuspend, ChangeInvisibleTimeResponseEntry[] responseEntries, + List> futures, int index) { + try { + ChangeInvisibleTimeRequestHeader requestHeader = buildRequestHeader(requestEntry); + futures.add(processSingleBatchEntry(channel, requestHeader, opaque, brokerAllowSuspend) + .thenAccept(entry -> responseEntries[index] = entry)); + } catch (Throwable t) { + responseEntries[index] = buildFailedResponseEntry(ResponseCode.SYSTEM_ERROR); + } + } + + protected CompletableFuture processSingleBatchEntry(final Channel channel, + ChangeInvisibleTimeRequestHeader requestHeader, int opaque, boolean brokerAllowSuspend) { + try { + return processSingleRequestAsync(channel, requestHeader, opaque, brokerAllowSuspend) + .thenApply(this::buildResponseEntry) + .exceptionally(throwable -> { + return buildFailedResponseEntry(ResponseCode.SYSTEM_ERROR); + }); + } catch (Throwable t) { + return CompletableFuture.completedFuture(buildFailedResponseEntry(ResponseCode.SYSTEM_ERROR)); + } + } + + protected ChangeInvisibleTimeResponseEntry buildFailedResponseEntry(int responseCode) { + ChangeInvisibleTimeResponseEntry failedEntry = new ChangeInvisibleTimeResponseEntry(); + failedEntry.setCode(responseCode); + return failedEntry; + } + + protected ChangeInvisibleTimeResponseEntry validateBatchKvEntry(final Channel channel, + ChangeInvisibleTimeRequestEntry requestEntry) throws RemotingCommandException { + TopicConfig topicConfig = this.brokerController.getTopicConfigManager().selectTopicConfig(requestEntry.getTopic()); + if (null == topicConfig) { + POP_LOGGER.error("The topic {} not exist, consumer: {} ", requestEntry.getTopic(), RemotingHelper.parseChannelRemoteAddr(channel)); + ChangeInvisibleTimeResponseEntry responseEntry = new ChangeInvisibleTimeResponseEntry(); + responseEntry.setCode(ResponseCode.TOPIC_NOT_EXIST); + return responseEntry; + } + + if (requestEntry.getQueueId() >= topicConfig.getReadQueueNums() || requestEntry.getQueueId() < 0) { + String errorInfo = String.format("queueId[%d] is illegal, topic:[%s] topicConfig.readQueueNums:[%d] consumer:[%s]", + requestEntry.getQueueId(), requestEntry.getTopic(), topicConfig.getReadQueueNums(), channel.remoteAddress()); + POP_LOGGER.warn(errorInfo); + ChangeInvisibleTimeResponseEntry responseEntry = new ChangeInvisibleTimeResponseEntry(); + responseEntry.setCode(ResponseCode.MESSAGE_ILLEGAL); + return responseEntry; + } + + long minOffset = this.brokerController.getMessageStore().getMinOffsetInQueue(requestEntry.getTopic(), requestEntry.getQueueId()); + long maxOffset; + try { + maxOffset = this.brokerController.getMessageStore().getMaxOffsetInQueue(requestEntry.getTopic(), requestEntry.getQueueId()); + } catch (ConsumeQueueException e) { + throw new RemotingCommandException("Failed to get max consume offset", e); + } + if (requestEntry.getOffset() < minOffset || requestEntry.getOffset() >= maxOffset) { + ChangeInvisibleTimeResponseEntry responseEntry = new ChangeInvisibleTimeResponseEntry(); + responseEntry.setCode(ResponseCode.NO_MESSAGE); + return responseEntry; + } + return null; + } + + protected ChangeInvisibleTimeRequestHeader buildRequestHeader(ChangeInvisibleTimeRequestEntry entry) { + ChangeInvisibleTimeRequestHeader requestHeader = new ChangeInvisibleTimeRequestHeader(); + requestHeader.setConsumerGroup(entry.getConsumerGroup()); + requestHeader.setTopic(entry.getTopic()); + requestHeader.setQueueId(entry.getQueueId()); + requestHeader.setExtraInfo(entry.getExtraInfo()); + requestHeader.setOffset(entry.getOffset()); + requestHeader.setInvisibleTime(entry.getInvisibleTime()); + requestHeader.setLiteTopic(entry.getLiteTopic()); + requestHeader.setSuspend(entry.isSuspend()); + return requestHeader; + } + + protected ChangeInvisibleTimeResponseEntry buildResponseEntry(RemotingCommand singleResponse) { + ChangeInvisibleTimeResponseEntry responseEntry = new ChangeInvisibleTimeResponseEntry(); + responseEntry.setCode(singleResponse.getCode()); + ChangeInvisibleTimeResponseHeader responseHeader = + (ChangeInvisibleTimeResponseHeader) singleResponse.readCustomHeader(); + if (responseHeader != null) { + responseEntry.setPopTime(responseHeader.getPopTime()); + responseEntry.setInvisibleTime(responseHeader.getInvisibleTime()); + responseEntry.setReviveQid(responseHeader.getReviveQid()); + } + return responseEntry; + } + @SuppressWarnings({"StatementWithEmptyBody", "DuplicatedCode"}) public CompletableFuture processChangeInvisibleTimeForOrderNew( ChangeInvisibleTimeRequestHeader requestHeader, String[] extraInfo, diff --git a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java index 28045ca26e7..688d6466635 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java @@ -16,7 +16,9 @@ */ package org.apache.rocketmq.broker.pop; +import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.Queue; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.atomic.AtomicInteger; @@ -24,12 +26,15 @@ import org.apache.rocketmq.broker.BrokerController; import org.apache.rocketmq.broker.offset.ConsumerOffsetManager; import org.apache.rocketmq.common.BrokerConfig; +import org.apache.rocketmq.common.KeyBuilder; import org.awaitility.Awaitility; import org.junit.Assert; import org.junit.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mockito; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; public class PopConsumerCacheTest { @@ -98,6 +103,7 @@ public void consumerCacheTest() { PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); + Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); PopConsumerCache consumerCache = new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); @@ -143,4 +149,216 @@ record = new PopConsumerRecord(2L, groupId, topicId, queueId, consumerCache.cleanupRecords(consumerRecordList::add); Assert.assertEquals(0, consumerRecordList.size()); } -} \ No newline at end of file + + @Test + public void cleanupRecordsShouldCommitOffsetWhileHoldingConsumerLock() { + BrokerConfig brokerConfig = new BrokerConfig(); + brokerConfig.setPopCkStayBufferTime(60000); + BrokerController brokerController = Mockito.mock(BrokerController.class); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); + PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); + ConsumerOffsetManager consumerOffsetManager = Mockito.mock(ConsumerOffsetManager.class); + Mockito.when(brokerController.getBrokerConfig()).thenReturn(brokerConfig); + Mockito.when(brokerController.getConsumerOffsetManager()).thenReturn(consumerOffsetManager); + Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); + + PopConsumerCache consumerCache = + new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); + PopConsumerRecord record = new PopConsumerRecord(System.currentTimeMillis(), groupId, topicId, queueId, + 0, 20000, 100, attemptId); + consumerCache.writeRecords(Collections.singletonList(record)); + + int remain = consumerCache.cleanupRecords(ignored -> Assert.fail("Record should remain in cache")); + + Assert.assertEquals(1, remain); + Mockito.verify(consumerOffsetManager).commitOffset("PopConsumerCache", groupId, topicId, queueId, 100L); + } + + @Test + public void writeAndDeleteRecordsShouldSkipStoreDeleteForBufferedRecords() { + BrokerController brokerController = Mockito.mock(BrokerController.class); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); + PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); + Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); + Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); + + PopConsumerCache consumerCache = + new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); + PopConsumerRecord bufferedOldRecord = new PopConsumerRecord(2L, groupId, topicId, queueId, + 0, 20000, 100, attemptId); + PopConsumerRecord storeOldRecord = new PopConsumerRecord(3L, groupId, topicId, queueId, + 0, 20000, 101, attemptId); + PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, topicId, queueId, + 0, 30000, 100, attemptId); + consumerCache.writeRecords(Collections.singletonList(bufferedOldRecord)); + + consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), + Arrays.asList(bufferedOldRecord, storeOldRecord)); + + ArgumentCaptor> writeCaptor = ArgumentCaptor.forClass(List.class); + ArgumentCaptor> deleteCaptor = ArgumentCaptor.forClass(List.class); + Mockito.verify(consumerKVStore).writeAndDeleteRecords(writeCaptor.capture(), deleteCaptor.capture()); + Assert.assertEquals(Collections.singletonList(newRecord), writeCaptor.getValue()); + Assert.assertEquals(Collections.singletonList(storeOldRecord), deleteCaptor.getValue()); + Assert.assertEquals(0, consumerCache.getCacheSize()); + Assert.assertEquals(0, consumerCache.getPopInFlightMessageCount(groupId, topicId, queueId)); + } + + @Test + public void writeAndDeleteRecordsShouldUseSingleConsumerLock() { + BrokerController brokerController = Mockito.mock(BrokerController.class); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); + PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); + Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); + Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); + + PopConsumerCache consumerCache = + new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); + PopConsumerRecord oldRecord1 = new PopConsumerRecord(2L, groupId, topicId, queueId, + 0, 20000, 100, attemptId); + PopConsumerRecord oldRecord2 = new PopConsumerRecord(3L, groupId, topicId, queueId + 1, + 0, 20000, 101, attemptId); + PopConsumerRecord newRecord1 = new PopConsumerRecord(4L, groupId, topicId, queueId, + 0, 30000, 100, attemptId); + PopConsumerRecord newRecord2 = new PopConsumerRecord(5L, groupId, topicId, queueId + 1, + 0, 30000, 101, attemptId); + + consumerCache.writeAndDeleteRecords(Arrays.asList(newRecord1, newRecord2), + Arrays.asList(oldRecord1, oldRecord2)); + + Mockito.verify(consumerLockService, Mockito.times(1)).tryLock(groupId, topicId); + Mockito.verify(consumerLockService, Mockito.times(1)).unlock(groupId, topicId); + } + + @Test + public void writeAndDeleteRecordsShouldLockNormalTopicForRetryTopic() { + BrokerController brokerController = Mockito.mock(BrokerController.class); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); + PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); + Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); + Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); + + String retryTopic = KeyBuilder.buildPopRetryTopicV2(topicId, groupId); + PopConsumerCache consumerCache = + new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); + PopConsumerRecord oldRecord = new PopConsumerRecord(2L, groupId, retryTopic, queueId, + 0, 20000, 100, attemptId); + PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, retryTopic, queueId, + 0, 30000, 100, attemptId); + + consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), + Collections.singletonList(oldRecord)); + + Mockito.verify(consumerLockService, Mockito.times(1)).tryLock(groupId, topicId); + Mockito.verify(consumerLockService, Mockito.times(1)).unlock(groupId, topicId); + } + + @Test + public void writeAndDeleteRecordsShouldRejectMixedConsumerLockKey() { + BrokerController brokerController = Mockito.mock(BrokerController.class); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); + PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); + Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); + + PopConsumerCache consumerCache = + new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); + PopConsumerRecord oldRecord = new PopConsumerRecord(2L, groupId, topicId, queueId, + 0, 20000, 100, attemptId); + PopConsumerRecord mixedOldRecord = new PopConsumerRecord(3L, groupId, topicId + "_other", queueId, + 0, 20000, 101, attemptId); + PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, topicId, queueId, + 0, 30000, 100, attemptId); + + try { + consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), + Arrays.asList(oldRecord, mixedOldRecord)); + Assert.fail("Should reject mixed consumer lock key"); + } catch (IllegalArgumentException e) { + Assert.assertTrue(e.getMessage().contains("same consumer")); + } + + Mockito.verify(consumerLockService, Mockito.never()).tryLock(anyString(), anyString()); + Mockito.verify(consumerKVStore, Mockito.never()).writeAndDeleteRecords(any(), any()); + } + + @Test + public void writeAndDeleteRecordsShouldDeleteBufferOnlyWhenStoreKeyMatchesNewRecord() { + BrokerController brokerController = Mockito.mock(BrokerController.class); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); + PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); + Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); + Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); + + PopConsumerCache consumerCache = + new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); + PopConsumerRecord bufferedOldRecord = new PopConsumerRecord(2L, groupId, topicId, queueId, + 0, 20000, 100, attemptId); + PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, topicId, queueId, + 0, 19998, 100, attemptId); + consumerCache.writeRecords(Collections.singletonList(bufferedOldRecord)); + + consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), + Collections.singletonList(bufferedOldRecord)); + + ArgumentCaptor> deleteCaptor = ArgumentCaptor.forClass(List.class); + Mockito.verify(consumerKVStore).writeAndDeleteRecords(any(), deleteCaptor.capture()); + Assert.assertEquals(Collections.emptyList(), deleteCaptor.getValue()); + Assert.assertEquals(0, consumerCache.getCacheSize()); + Assert.assertEquals(0, consumerCache.getPopInFlightMessageCount(groupId, topicId, queueId)); + } + + @Test + public void writeAndDeleteRecordsShouldKeepBufferWhenStoreFails() { + BrokerController brokerController = Mockito.mock(BrokerController.class); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); + PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); + Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); + Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); + Mockito.doThrow(new RuntimeException("store failure")) + .when(consumerKVStore).writeAndDeleteRecords(any(), any()); + + PopConsumerCache consumerCache = + new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); + PopConsumerRecord bufferedOldRecord = new PopConsumerRecord(2L, groupId, topicId, queueId, + 0, 20000, 100, attemptId); + PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, topicId, queueId, + 0, 30000, 100, attemptId); + consumerCache.writeRecords(Collections.singletonList(bufferedOldRecord)); + + try { + consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), + Collections.singletonList(bufferedOldRecord)); + Assert.fail("Should throw store failure"); + } catch (RuntimeException e) { + Assert.assertEquals("store failure", e.getMessage()); + } + + Assert.assertEquals(1, consumerCache.getCacheSize()); + Assert.assertEquals(1, consumerCache.getPopInFlightMessageCount(groupId, topicId, queueId)); + } + + @Test + public void writeAndDeleteRecordsShouldSendSameStoreKeyDeleteToStore() { + BrokerController brokerController = Mockito.mock(BrokerController.class); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); + PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); + Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); + Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); + + PopConsumerCache consumerCache = + new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); + PopConsumerRecord oldRecord = new PopConsumerRecord(2L, groupId, topicId, queueId, + 0, 20000, 100, attemptId); + PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, topicId, queueId, + 0, 19998, 100, attemptId); + + consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), + Collections.singletonList(oldRecord)); + + ArgumentCaptor> deleteCaptor = ArgumentCaptor.forClass(List.class); + Mockito.verify(consumerKVStore).writeAndDeleteRecords(any(), deleteCaptor.capture()); + Assert.assertEquals(Collections.singletonList(oldRecord), deleteCaptor.getValue()); + Assert.assertEquals(0, consumerCache.getCacheSize()); + Assert.assertEquals(0, consumerCache.getPopInFlightMessageCount(groupId, topicId, queueId)); + } +} diff --git a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerRocksdbStoreTest.java b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerRocksdbStoreTest.java index 1d44c0109f0..7bd186609ff 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerRocksdbStoreTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerRocksdbStoreTest.java @@ -22,6 +22,7 @@ import java.lang.reflect.Field; import java.nio.file.Paths; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.UUID; import java.util.concurrent.TimeUnit; @@ -110,6 +111,68 @@ public void rocksdbStoreWriteDeleteTest() { deleteStoreDirectory(filePath); } + @Test + public void rocksdbStoreWriteAndDeleteTest() { + String filePath = getRandomStorePath(); + PopConsumerKVStore consumerStore = new PopConsumerRocksdbStore( + filePath, 256 * SizeUnit.MB, 32 * SizeUnit.MB); + consumerStore.start(); + + PopConsumerRecord oldRecord = getConsumerRecord(); + oldRecord.setOffset(100L); + PopConsumerRecord keptRecord = getConsumerRecord(); + keptRecord.setOffset(101L); + consumerStore.writeRecords(IntStream.of(0, 1).mapToObj(i -> i == 0 ? oldRecord : keptRecord) + .collect(Collectors.toList())); + + PopConsumerRecord newRecord = getConsumerRecord(); + newRecord.setPopTime(oldRecord.getPopTime() + 1000); + newRecord.setInvisibleTime(oldRecord.getInvisibleTime() + 1000); + newRecord.setOffset(oldRecord.getOffset()); + + consumerStore.writeAndDeleteRecords(Collections.singletonList(newRecord), Collections.singletonList(oldRecord)); + + List consumerRecords = consumerStore.scanExpiredRecords(0, Long.MAX_VALUE, 10); + Assert.assertEquals(2, consumerRecords.size()); + Assert.assertTrue(consumerRecords.stream().anyMatch(record -> + record.getOffset() == newRecord.getOffset() && + record.getVisibilityTimeout() == newRecord.getVisibilityTimeout())); + Assert.assertTrue(consumerRecords.stream().anyMatch(record -> record.getOffset() == keptRecord.getOffset())); + Assert.assertFalse(consumerRecords.stream().anyMatch(record -> + record.getOffset() == oldRecord.getOffset() && + record.getVisibilityTimeout() == oldRecord.getVisibilityTimeout())); + + consumerStore.shutdown(); + deleteStoreDirectory(filePath); + } + + @Test + public void rocksdbStoreWriteAndDeleteShouldKeepWrittenRecordWhenKeyIsSame() { + String filePath = getRandomStorePath(); + PopConsumerKVStore consumerStore = new PopConsumerRocksdbStore( + filePath, 256 * SizeUnit.MB, 32 * SizeUnit.MB); + consumerStore.start(); + + PopConsumerRecord oldRecord = getConsumerRecord(); + oldRecord.setPopTime(2L); + oldRecord.setInvisibleTime(20000L); + PopConsumerRecord newRecord = getConsumerRecord(); + newRecord.setPopTime(4L); + newRecord.setInvisibleTime(19998L); + Assert.assertEquals(oldRecord.getVisibilityTimeout(), newRecord.getVisibilityTimeout()); + + consumerStore.writeRecords(Collections.singletonList(oldRecord)); + consumerStore.writeAndDeleteRecords(Collections.singletonList(newRecord), Collections.singletonList(oldRecord)); + + List consumerRecords = consumerStore.scanExpiredRecords(0, Long.MAX_VALUE, 10); + Assert.assertEquals(1, consumerRecords.size()); + Assert.assertEquals(newRecord.getPopTime(), consumerRecords.get(0).getPopTime()); + Assert.assertEquals(newRecord.getInvisibleTime(), consumerRecords.get(0).getInvisibleTime()); + + consumerStore.shutdown(); + deleteStoreDirectory(filePath); + } + private long getDirectorySizeRecursive(File directory) { long size = 0; File[] files = directory.listFiles(); @@ -193,4 +256,4 @@ public void tombstoneDeletionTest() throws IllegalAccessException, NoSuchFieldEx rocksdbStore.shutdown(); deleteStoreDirectory(rocksdbStore.getFilePath()); } -} \ No newline at end of file +} diff --git a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java index 089d2c1f22b..a847bbdd594 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java @@ -538,6 +538,44 @@ public void testChangeInvisibilityDurationWithSuspendFalse() { consumerService.shutdown(); } + @Test + public void testBatchChangeInvisibilityDurationDeletesSameVisibilityOldRecordFromCache() + throws IllegalAccessException { + long current = System.currentTimeMillis(); + long popTime = current - 1000; + long invisibleTime = 10000; + long changedPopTime = current; + long changedInvisibleTime = 9000; + long offset = 300L; + + consumerService.getPopConsumerStore().start(); + Mockito.when(brokerController.getSubscriptionGroupManager().containsSubscriptionGroup(groupId)).thenReturn(true); + + PopConsumerCache consumerCache = (PopConsumerCache) FieldUtils.readField( + consumerService, "popConsumerCache", true); + PopConsumerRecord oldRecord = new PopConsumerRecord(popTime, groupId, topicId, queueId, + 0, invisibleTime, offset, null, false); + consumerCache.writeRecords(Collections.singletonList(oldRecord)); + Assert.assertEquals(1, consumerCache.getPopInFlightMessageCount(groupId, topicId, queueId)); + + PopConsumerService.ChangeInvisibilityRecord changeRecord = + new PopConsumerService.ChangeInvisibilityRecord(popTime, invisibleTime, changedPopTime, + changedInvisibleTime, groupId, topicId, queueId, offset, false); + consumerService.batchChangeInvisibilityDuration(Collections.singletonList(changeRecord)); + + Assert.assertEquals(0, consumerCache.getPopInFlightMessageCount(groupId, topicId, queueId)); + List records = consumerService.getPopConsumerStore() + .scanExpiredRecords(0, changedPopTime + changedInvisibleTime + 1000, 10); + PopConsumerRecord ckRecord = records.stream() + .filter(r -> r.getOffset() == offset && r.getPopTime() == changedPopTime) + .findFirst() + .orElse(null); + Assert.assertNotNull("Should find the changed checkpoint record", ckRecord); + Assert.assertEquals(changedInvisibleTime, ckRecord.getInvisibleTime()); + + consumerService.shutdown(); + } + @Test public void testReviveRetryWithSuspendTrue() { Mockito.when(brokerController.getTopicConfigManager().selectTopicConfig(topicId)).thenReturn(null); @@ -743,4 +781,4 @@ public void testReviveRetryWithSuspendFalseMultipleTimes() { messageExt.setReconsumeTimes(capturedMessage.getReconsumeTimes()); } } -} \ No newline at end of file +} diff --git a/broker/src/test/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessorTest.java b/broker/src/test/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessorTest.java index 75ce68f4bdf..504008f1263 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessorTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessorTest.java @@ -24,6 +24,8 @@ import org.apache.rocketmq.broker.failover.EscapeBridge; import org.apache.rocketmq.broker.metrics.BrokerMetricsManager; import org.apache.rocketmq.broker.metrics.PopMetricsManager; +import org.apache.rocketmq.broker.pop.PopConsumerService; +import org.apache.rocketmq.broker.pop.PopConsumerService.ChangeInvisibilityRecord; import org.apache.rocketmq.broker.topic.TopicConfigManager; import com.alibaba.fastjson2.JSON; import org.apache.rocketmq.common.BrokerConfig; @@ -38,7 +40,11 @@ import org.apache.rocketmq.remoting.protocol.RemotingCommand; import org.apache.rocketmq.remoting.protocol.RequestCode; import org.apache.rocketmq.remoting.protocol.ResponseCode; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeRequestBody; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeResponseBody; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeRequestHeader; +import org.apache.rocketmq.remoting.protocol.header.BatchChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ExtraInfoUtil; import org.apache.rocketmq.remoting.protocol.heartbeat.ConsumerData; import org.apache.rocketmq.store.AppendMessageResult; @@ -56,10 +62,12 @@ import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; import java.lang.reflect.Field; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; @@ -202,6 +210,143 @@ public void testProcessRequest_NoMessage() throws RemotingCommandException, Cons assertThat(responseToReturn.getOpaque()).isEqualTo(request.getOpaque()); } + @Test + public void testProcessBatchRequestRejectsOversizedBody() throws Exception { + assertThat(brokerController.getBrokerConfig().getBatchChangeInvisibleTimeMaxNum()).isEqualTo(1024); + int batchMaxNum = brokerController.getBrokerConfig().getBatchChangeInvisibleTimeMaxNum(); + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + List entries = new ArrayList<>(); + for (int i = 0; i <= batchMaxNum; i++) { + ChangeInvisibleTimeRequestEntry entry = new ChangeInvisibleTimeRequestEntry(); + entry.setConsumerGroup(group); + entry.setTopic(topic); + entry.setQueueId(0); + entry.setExtraInfo("0 10000 10000 0 test_broker 0"); + entry.setOffset(i); + entry.setInvisibleTime(30000); + entries.add(entry); + } + requestBody.setEntries(entries); + + RemotingCommand request = buildBatchRequest(requestBody); + request.setBody(requestBody.encode()); + RemotingCommand response = changeInvisibleTimeProcessor.processRequestAsync(channel, request, true).get(); + + assertThat(response.getCode()).isEqualTo(ResponseCode.MESSAGE_ILLEGAL); + assertThat(response.getRemark()).contains(String.valueOf(batchMaxNum)); + } + + @Test + public void testProcessBatchRequestConvertsBadEntryToPerEntryFailure() throws Exception { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + List entries = new ArrayList<>(); + entries.add(null); + requestBody.setEntries(entries); + + RemotingCommand request = buildBatchRequest(requestBody); + request.setBody(requestBody.encode()); + RemotingCommand response = changeInvisibleTimeProcessor.processRequestAsync(channel, request, true).get(); + + BatchChangeInvisibleTimeResponseBody responseBody = + BatchChangeInvisibleTimeResponseBody.decode(response.getBody(), BatchChangeInvisibleTimeResponseBody.class); + assertThat(response.getCode()).isEqualTo(ResponseCode.SUCCESS); + assertThat(responseBody.getEntries()).hasSize(1); + assertThat(responseBody.getEntries().get(0).getCode()).isEqualTo(ResponseCode.SYSTEM_ERROR); + } + + @Test + @SuppressWarnings("unchecked") + public void testProcessBatchKvRequestDoesNotBuildSingleHeader() throws Exception { + brokerController.getBrokerConfig().setPopConsumerKVServiceEnable(true); + when(messageStore.getMinOffsetInQueue(anyString(), anyInt())).thenReturn(0L); + when(messageStore.getMaxOffsetInQueue(anyString(), anyInt())).thenReturn(10L); + PopConsumerService popConsumerService = mock(PopConsumerService.class); + Mockito.doReturn(popConsumerService).when(brokerController).getPopConsumerService(); + + ChangeInvisibleTimeProcessor processor = new ChangeInvisibleTimeProcessor(brokerController) { + @Override + protected ChangeInvisibleTimeRequestHeader buildRequestHeader(ChangeInvisibleTimeRequestEntry entry) { + throw new AssertionError("KV batch entry should not build single request header"); + } + }; + + long popTime = System.currentTimeMillis() - 1_000; + long oldInvisibleTime = 30_000; + long newInvisibleTime = 60_000; + long queueOffset = 1L; + int reviveQid = 0; + ChangeInvisibleTimeRequestEntry entry = new ChangeInvisibleTimeRequestEntry(); + entry.setConsumerGroup(group); + entry.setTopic(topic); + entry.setQueueId(0); + entry.setExtraInfo(ExtraInfoUtil.buildExtraInfo(queueOffset, popTime, oldInvisibleTime, reviveQid, + topic, "test_broker", 0)); + entry.setOffset(queueOffset); + entry.setInvisibleTime(newInvisibleTime); + + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + List entries = new ArrayList<>(); + entries.add(entry); + requestBody.setEntries(entries); + + RemotingCommand request = buildBatchRequest(requestBody); + request.setBody(requestBody.encode()); + RemotingCommand response = processor.processRequestAsync(channel, request, true).get(); + + BatchChangeInvisibleTimeResponseBody responseBody = + BatchChangeInvisibleTimeResponseBody.decode(response.getBody(), BatchChangeInvisibleTimeResponseBody.class); + assertThat(response.getCode()).isEqualTo(ResponseCode.SUCCESS); + assertThat(responseBody.getEntries()).hasSize(1); + assertThat(responseBody.getEntries().get(0).getCode()).isEqualTo(ResponseCode.SUCCESS); + assertThat(responseBody.getEntries().get(0).getInvisibleTime()).isEqualTo(newInvisibleTime); + assertThat(responseBody.getEntries().get(0).getReviveQid()).isEqualTo(reviveQid); + + ArgumentCaptor changeRecordsCaptor = ArgumentCaptor.forClass(List.class); + Mockito.verify(popConsumerService).batchChangeInvisibilityDuration(changeRecordsCaptor.capture()); + List changeRecords = changeRecordsCaptor.getValue(); + assertThat(changeRecords).hasSize(1); + assertThat(changeRecords.get(0).getPopTime()).isEqualTo(popTime); + assertThat(changeRecords.get(0).getInvisibleTime()).isEqualTo(oldInvisibleTime); + assertThat(changeRecords.get(0).getChangedInvisibleTime()).isEqualTo(newInvisibleTime); + assertThat(changeRecords.get(0).getGroupId()).isEqualTo(group); + assertThat(changeRecords.get(0).getTopicId()).isEqualTo(topic); + assertThat(changeRecords.get(0).getQueueId()).isEqualTo(0); + assertThat(changeRecords.get(0).getOffset()).isEqualTo(queueOffset); + } + + @Test + public void testProcessBatchRequestRejectsMismatchedTopicOrGroup() throws Exception { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + ChangeInvisibleTimeRequestEntry entry = new ChangeInvisibleTimeRequestEntry(); + entry.setConsumerGroup(group + "_other"); + entry.setTopic(topic); + entry.setQueueId(0); + entry.setExtraInfo("0 10000 10000 0 test_broker 0"); + entry.setOffset(0L); + entry.setInvisibleTime(30000); + List entries = new ArrayList<>(); + entries.add(entry); + requestBody.setEntries(entries); + + RemotingCommand request = buildBatchRequest(requestBody); + request.setBody(requestBody.encode()); + RemotingCommand response = changeInvisibleTimeProcessor.processRequestAsync(channel, request, true).get(); + + assertThat(response.getCode()).isEqualTo(ResponseCode.MESSAGE_ILLEGAL); + assertThat(response.getRemark()).contains("same topic and consumerGroup"); + } + + private RemotingCommand buildBatchRequest(BatchChangeInvisibleTimeRequestBody requestBody) { + BatchChangeInvisibleTimeRequestHeader requestHeader = new BatchChangeInvisibleTimeRequestHeader(); + requestHeader.setTopic(topic); + requestHeader.setConsumerGroup(group); + RemotingCommand request = RemotingCommand.createRequestCommand( + RequestCode.BATCH_CHANGE_MESSAGE_INVISIBLETIME, requestHeader); + request.setBody(requestBody.encode()); + request.makeCustomHeaderToNet(); + return request; + } + @Test public void testProcessRequestAsync_JsonParsing() throws Exception { Channel mockChannel = mock(Channel.class); diff --git a/client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java b/client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java index 8294ffd422f..1c94576b68f 100644 --- a/client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java +++ b/client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java @@ -31,6 +31,7 @@ import java.util.Optional; import java.util.Properties; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; @@ -115,9 +116,13 @@ import org.apache.rocketmq.remoting.protocol.body.AclInfo; import org.apache.rocketmq.remoting.protocol.body.BatchAck; import org.apache.rocketmq.remoting.protocol.body.BatchAckMessageRequestBody; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeRequestBody; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeResponseBody; import org.apache.rocketmq.remoting.protocol.body.BrokerMemberGroup; import org.apache.rocketmq.remoting.protocol.body.BrokerReplicasInfo; import org.apache.rocketmq.remoting.protocol.body.BrokerStatsData; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeResponseEntry; import org.apache.rocketmq.remoting.protocol.body.CheckClientRequestBody; import org.apache.rocketmq.remoting.protocol.body.ClusterInfo; import org.apache.rocketmq.remoting.protocol.body.ConsumeMessageDirectlyResult; @@ -156,6 +161,7 @@ import org.apache.rocketmq.remoting.protocol.body.UserInfo; import org.apache.rocketmq.remoting.protocol.header.AckMessageRequestHeader; import org.apache.rocketmq.remoting.protocol.header.AddBrokerRequestHeader; +import org.apache.rocketmq.remoting.protocol.header.BatchChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeResponseHeader; import org.apache.rocketmq.remoting.protocol.header.CheckRocksdbCqWriteProgressRequestHeader; @@ -1040,6 +1046,88 @@ public void operationFail(Throwable throwable) { }); } + public CompletableFuture> batchChangeInvisibleTimeAsync( + final String addr, + final String topic, + final String consumerGroup, + final BatchChangeInvisibleTimeRequestBody requestBody, + final long timeoutMillis + ) { + CompletableFuture> future = new CompletableFuture<>(); + BatchChangeInvisibleTimeRequestHeader requestHeader = new BatchChangeInvisibleTimeRequestHeader(); + requestHeader.setTopic(topic); + requestHeader.setConsumerGroup(consumerGroup); + final RemotingCommand request = RemotingCommand.createRequestCommand( + RequestCode.BATCH_CHANGE_MESSAGE_INVISIBLETIME, requestHeader); + if (requestBody != null) { + request.setBody(requestBody.encode()); + } + try { + this.remotingClient.invokeAsync(addr, request, timeoutMillis, new InvokeCallback() { + @Override + public void operationComplete(ResponseFuture responseFuture) { + + } + + @Override + public void operationSucceed(RemotingCommand response) { + try { + if (ResponseCode.SUCCESS != response.getCode()) { + future.completeExceptionally(new MQBrokerException(response.getCode(), response.getRemark(), addr)); + return; + } + future.complete(processBatchChangeInvisibleTimeResponse(addr, requestBody, response)); + } catch (Throwable t) { + future.completeExceptionally(t); + } + } + + @Override + public void operationFail(Throwable throwable) { + future.completeExceptionally(throwable); + } + }); + } catch (Throwable t) { + future.completeExceptionally(t); + } + return future; + } + + protected List processBatchChangeInvisibleTimeResponse(String addr, + BatchChangeInvisibleTimeRequestBody requestBody, RemotingCommand response) throws MQBrokerException { + List requestEntries = requestBody == null || requestBody.getEntries() == null ? + Collections.emptyList() : requestBody.getEntries(); + BatchChangeInvisibleTimeResponseBody responseBody = + BatchChangeInvisibleTimeResponseBody.decode(response.getBody(), BatchChangeInvisibleTimeResponseBody.class); + List responseEntries = responseBody == null || responseBody.getEntries() == null ? + Collections.emptyList() : responseBody.getEntries(); + if (requestEntries.size() != responseEntries.size()) { + throw new MQBrokerException(ResponseCode.SYSTEM_ERROR, + String.format("batch change invisible time response size mismatch, request=%d, response=%d", + requestEntries.size(), responseEntries.size()), addr); + } + + List resultList = new ArrayList<>(responseEntries.size()); + for (int i = 0; i < responseEntries.size(); i++) { + ChangeInvisibleTimeRequestEntry requestEntry = requestEntries.get(i); + ChangeInvisibleTimeResponseEntry responseEntry = responseEntries.get(i); + AckResult ackResult = new AckResult(); + if (requestEntry != null && responseEntry != null && ResponseCode.SUCCESS == responseEntry.getCode()) { + ackResult.setStatus(AckStatus.OK); + ackResult.setPopTime(responseEntry.getPopTime()); + String brokerName = ExtraInfoUtil.getBrokerName(ExtraInfoUtil.split(requestEntry.getExtraInfo())); + ackResult.setExtraInfo(ExtraInfoUtil + .buildExtraInfo(requestEntry.getOffset(), responseEntry.getPopTime(), responseEntry.getInvisibleTime(), + responseEntry.getReviveQid(), requestEntry.getTopic(), brokerName, requestEntry.getQueueId()) + MessageConst.KEY_SEPARATOR + + requestEntry.getOffset()); + } else { + ackResult.setStatus(AckStatus.NO_EXIST); + } + resultList.add(ackResult); + } + return resultList; + } + private void pullMessageAsync( final String addr, final RemotingCommand request, diff --git a/client/src/main/java/org/apache/rocketmq/client/impl/mqclient/MQClientAPIExt.java b/client/src/main/java/org/apache/rocketmq/client/impl/mqclient/MQClientAPIExt.java index f577ea2043d..2344b91fa58 100644 --- a/client/src/main/java/org/apache/rocketmq/client/impl/mqclient/MQClientAPIExt.java +++ b/client/src/main/java/org/apache/rocketmq/client/impl/mqclient/MQClientAPIExt.java @@ -59,6 +59,7 @@ import org.apache.rocketmq.remoting.protocol.RemotingCommand; import org.apache.rocketmq.remoting.protocol.RequestCode; import org.apache.rocketmq.remoting.protocol.ResponseCode; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeRequestBody; import org.apache.rocketmq.remoting.protocol.body.GetLiteTopicInfoResponseBody; import org.apache.rocketmq.remoting.protocol.body.LiteSubscriptionCtlRequestBody; import org.apache.rocketmq.remoting.protocol.body.LockBatchRequestBody; @@ -361,6 +362,16 @@ public void onException(Throwable t) { return future; } + public CompletableFuture> batchChangeInvisibleTimeAsync( + String brokerAddr, + String topic, + String consumerGroup, + BatchChangeInvisibleTimeRequestBody requestBody, + long timeoutMillis + ) { + return super.batchChangeInvisibleTimeAsync(brokerAddr, topic, consumerGroup, requestBody, timeoutMillis); + } + public CompletableFuture pullMessageAsync( String brokerAddr, PullMessageRequestHeader requestHeader, diff --git a/client/src/test/java/org/apache/rocketmq/client/impl/MQClientAPIImplTest.java b/client/src/test/java/org/apache/rocketmq/client/impl/MQClientAPIImplTest.java index 27b3d685715..6f752622d56 100644 --- a/client/src/test/java/org/apache/rocketmq/client/impl/MQClientAPIImplTest.java +++ b/client/src/test/java/org/apache/rocketmq/client/impl/MQClientAPIImplTest.java @@ -41,6 +41,7 @@ import org.apache.rocketmq.common.Pair; import org.apache.rocketmq.common.TopicConfig; import org.apache.rocketmq.common.consumer.ConsumeFromWhere; +import org.apache.rocketmq.common.consumer.ReceiptHandle; import org.apache.rocketmq.common.message.Message; import org.apache.rocketmq.common.message.MessageAccessor; import org.apache.rocketmq.common.message.MessageClientIDSetter; @@ -71,10 +72,14 @@ import org.apache.rocketmq.remoting.protocol.admin.TopicOffset; import org.apache.rocketmq.remoting.protocol.admin.TopicStatsTable; import org.apache.rocketmq.remoting.protocol.body.AclInfo; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeRequestBody; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeResponseBody; import org.apache.rocketmq.remoting.protocol.body.BrokerMemberGroup; import org.apache.rocketmq.remoting.protocol.body.BrokerReplicasInfo; import org.apache.rocketmq.remoting.protocol.body.BrokerStatsData; import org.apache.rocketmq.remoting.protocol.body.BrokerStatsItem; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeResponseEntry; import org.apache.rocketmq.remoting.protocol.body.ClusterInfo; import org.apache.rocketmq.remoting.protocol.body.Connection; import org.apache.rocketmq.remoting.protocol.body.ConsumeMessageDirectlyResult; @@ -104,6 +109,7 @@ import org.apache.rocketmq.remoting.protocol.body.UnlockBatchRequestBody; import org.apache.rocketmq.remoting.protocol.body.UserInfo; import org.apache.rocketmq.remoting.protocol.header.AckMessageRequestHeader; +import org.apache.rocketmq.remoting.protocol.header.BatchChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeResponseHeader; import org.apache.rocketmq.remoting.protocol.header.EndTransactionRequestHeader; @@ -721,6 +727,141 @@ public void onException(Throwable e) { done.await(); } + @Test + public void testProcessBatchChangeInvisibleTimeResponse() throws Exception { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + ChangeInvisibleTimeRequestEntry requestEntry = new ChangeInvisibleTimeRequestEntry(); + requestEntry.setConsumerGroup(group); + requestEntry.setTopic(topic); + requestEntry.setQueueId(1); + requestEntry.setExtraInfo("10 100 1000 0 0 broker-a 1 11"); + requestEntry.setOffset(11); + requestEntry.setInvisibleTime(3000); + requestBody.setEntries(Collections.singletonList(requestEntry)); + + BatchChangeInvisibleTimeResponseBody responseBody = new BatchChangeInvisibleTimeResponseBody(); + ChangeInvisibleTimeResponseEntry responseEntry = new ChangeInvisibleTimeResponseEntry(); + responseEntry.setCode(ResponseCode.SUCCESS); + responseEntry.setPopTime(200); + responseEntry.setInvisibleTime(3000); + responseEntry.setReviveQid(2); + responseBody.setEntries(Collections.singletonList(responseEntry)); + + RemotingCommand response = RemotingCommand.createResponseCommand(null); + response.setCode(ResponseCode.SUCCESS); + response.setBody(responseBody.encode()); + + List resultList = + mqClientAPI.processBatchChangeInvisibleTimeResponse(brokerAddr, requestBody, response); + + assertThat(resultList).hasSize(1); + assertThat(resultList.get(0).getStatus()).isEqualTo(AckStatus.OK); + ReceiptHandle receiptHandle = ReceiptHandle.decode(resultList.get(0).getExtraInfo()); + assertThat(receiptHandle.getStartOffset()).isEqualTo(11); + assertThat(receiptHandle.getRetrieveTime()).isEqualTo(200); + assertThat(receiptHandle.getInvisibleTime()).isEqualTo(3000); + assertThat(receiptHandle.getReviveQueueId()).isEqualTo(2); + assertThat(receiptHandle.getBrokerName()).isEqualTo("broker-a"); + assertThat(receiptHandle.getQueueId()).isEqualTo(1); + assertThat(receiptHandle.getOffset()).isEqualTo(11); + } + + @Test + public void testProcessBatchChangeInvisibleTimeResponseWithNullEntry() throws Exception { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + ChangeInvisibleTimeRequestEntry requestEntry = new ChangeInvisibleTimeRequestEntry(); + requestEntry.setConsumerGroup(group); + requestEntry.setTopic(topic); + requestEntry.setQueueId(1); + requestEntry.setExtraInfo("10 100 1000 0 0 broker-a 1 11"); + requestEntry.setOffset(11); + requestEntry.setInvisibleTime(3000); + requestBody.setEntries(Collections.singletonList(requestEntry)); + + BatchChangeInvisibleTimeResponseBody responseBody = new BatchChangeInvisibleTimeResponseBody(); + responseBody.setEntries(Collections.singletonList(null)); + + RemotingCommand response = RemotingCommand.createResponseCommand(null); + response.setCode(ResponseCode.SUCCESS); + response.setBody(responseBody.encode()); + + List resultList = + mqClientAPI.processBatchChangeInvisibleTimeResponse(brokerAddr, requestBody, response); + + assertThat(resultList).hasSize(1); + assertThat(resultList.get(0).getStatus()).isEqualTo(AckStatus.NO_EXIST); + } + + @Test + public void testProcessBatchChangeInvisibleTimeResponseWithNullRequestEntry() throws Exception { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + List requestEntries = new ArrayList<>(); + requestEntries.add(null); + requestBody.setEntries(requestEntries); + + BatchChangeInvisibleTimeResponseBody responseBody = new BatchChangeInvisibleTimeResponseBody(); + ChangeInvisibleTimeResponseEntry responseEntry = new ChangeInvisibleTimeResponseEntry(); + responseEntry.setCode(ResponseCode.SUCCESS); + responseEntry.setPopTime(200); + responseEntry.setInvisibleTime(3000); + responseEntry.setReviveQid(2); + responseBody.setEntries(Collections.singletonList(responseEntry)); + + RemotingCommand response = RemotingCommand.createResponseCommand(null); + response.setCode(ResponseCode.SUCCESS); + response.setBody(responseBody.encode()); + + List resultList = + mqClientAPI.processBatchChangeInvisibleTimeResponse(brokerAddr, requestBody, response); + + assertThat(resultList).hasSize(1); + assertThat(resultList.get(0).getStatus()).isEqualTo(AckStatus.NO_EXIST); + } + + @Test + public void testBatchChangeInvisibleTimeAsyncSendsRequestHeader() throws Exception { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + ChangeInvisibleTimeRequestEntry requestEntry = new ChangeInvisibleTimeRequestEntry(); + requestEntry.setConsumerGroup(group); + requestEntry.setTopic(topic); + requestEntry.setQueueId(1); + requestEntry.setExtraInfo("10 100 1000 0 0 broker-a 1 11"); + requestEntry.setOffset(11); + requestEntry.setInvisibleTime(3000); + requestBody.setEntries(Collections.singletonList(requestEntry)); + + BatchChangeInvisibleTimeResponseBody responseBody = new BatchChangeInvisibleTimeResponseBody(); + ChangeInvisibleTimeResponseEntry responseEntry = new ChangeInvisibleTimeResponseEntry(); + responseEntry.setCode(ResponseCode.SUCCESS); + responseEntry.setPopTime(200); + responseEntry.setInvisibleTime(3000); + responseEntry.setReviveQid(2); + responseBody.setEntries(Collections.singletonList(responseEntry)); + + RemotingCommand[] capturedRequest = new RemotingCommand[1]; + doAnswer((Answer) invocation -> { + capturedRequest[0] = invocation.getArgument(1); + InvokeCallback callback = invocation.getArgument(3); + RemotingCommand response = RemotingCommand.createResponseCommand(null); + response.setCode(ResponseCode.SUCCESS); + response.setBody(responseBody.encode()); + callback.operationSucceed(response); + return null; + }).when(remotingClient).invokeAsync(anyString(), any(RemotingCommand.class), anyLong(), any(InvokeCallback.class)); + + List resultList = mqClientAPI.batchChangeInvisibleTimeAsync( + brokerAddr, topic, group, requestBody, 3000).get(); + + assertThat(resultList).hasSize(1); + assertThat(resultList.get(0).getStatus()).isEqualTo(AckStatus.OK); + assertThat(capturedRequest[0].getCode()).isEqualTo(RequestCode.BATCH_CHANGE_MESSAGE_INVISIBLETIME); + capturedRequest[0].makeCustomHeaderToNet(); + BatchChangeInvisibleTimeRequestHeader requestHeader = (BatchChangeInvisibleTimeRequestHeader) + capturedRequest[0].decodeCommandCustomHeader(BatchChangeInvisibleTimeRequestHeader.class); + assertThat(requestHeader.getTopic()).isEqualTo(topic); + assertThat(requestHeader.getConsumerGroup()).isEqualTo(group); + } + @Test public void testSetMessageRequestMode_Success() throws Exception { doAnswer((Answer) mock -> { diff --git a/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java b/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java index 38644659e10..0d448dd43e1 100644 --- a/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java +++ b/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java @@ -230,6 +230,7 @@ public class BrokerConfig extends BrokerIdentity { private int popCkMaxBufferSize = 200000; private int popCkOffsetMaxQueueSize = 20000; private boolean enablePopBatchAck = false; + private int batchChangeInvisibleTimeMaxNum = PopAckConstants.DEFAULT_BATCH_CHANGE_INVISIBLE_TIME_MAX_NUM; // set the interval to the maxFilterMessageSize in MessageStoreConfig divided by the cq unit size private long popLongPollingForceNotifyInterval = 800; private boolean enableNotifyBeforePopCalculateLag = true; @@ -698,6 +699,14 @@ public void setEnablePopBatchAck(boolean enablePopBatchAck) { this.enablePopBatchAck = enablePopBatchAck; } + public int getBatchChangeInvisibleTimeMaxNum() { + return batchChangeInvisibleTimeMaxNum; + } + + public void setBatchChangeInvisibleTimeMaxNum(int batchChangeInvisibleTimeMaxNum) { + this.batchChangeInvisibleTimeMaxNum = batchChangeInvisibleTimeMaxNum; + } + public boolean isEnableSkipLongAwaitingAck() { return enableSkipLongAwaitingAck; } diff --git a/common/src/main/java/org/apache/rocketmq/common/PopAckConstants.java b/common/src/main/java/org/apache/rocketmq/common/PopAckConstants.java index 2f979fa15f0..56dca9bbc1c 100644 --- a/common/src/main/java/org/apache/rocketmq/common/PopAckConstants.java +++ b/common/src/main/java/org/apache/rocketmq/common/PopAckConstants.java @@ -32,6 +32,7 @@ public class PopAckConstants { public static final String ACK_TAG = "ack"; public static final String BATCH_ACK_TAG = "bAck"; public static final String SPLIT = "@"; + public static final int DEFAULT_BATCH_CHANGE_INVISIBLE_TIME_MAX_NUM = 1024; /** * Build cluster revive topic diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/common/RenewEvent.java b/proxy/src/main/java/org/apache/rocketmq/proxy/common/RenewEvent.java index 8d591560a7d..c0b0b26ad28 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/common/RenewEvent.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/common/RenewEvent.java @@ -17,15 +17,19 @@ package org.apache.rocketmq.proxy.common; +import java.util.Collections; +import java.util.List; import java.util.concurrent.CompletableFuture; import org.apache.rocketmq.client.consumer.AckResult; public class RenewEvent { protected ReceiptHandleGroupKey key; protected MessageReceiptHandle messageReceiptHandle; + protected List messageReceiptHandleList; protected long renewTime; protected EventType eventType; protected CompletableFuture future; + protected List> futureList; public enum EventType { RENEW, @@ -37,9 +41,23 @@ public RenewEvent(ReceiptHandleGroupKey key, MessageReceiptHandle messageReceipt EventType eventType, CompletableFuture future) { this.key = key; this.messageReceiptHandle = messageReceiptHandle; + this.messageReceiptHandleList = Collections.singletonList(messageReceiptHandle); this.renewTime = renewTime; this.eventType = eventType; this.future = future; + this.futureList = Collections.singletonList(future); + } + + public RenewEvent(ReceiptHandleGroupKey key, List messageReceiptHandleList, long renewTime, + EventType eventType, List> futureList) { + this.key = key; + this.messageReceiptHandleList = messageReceiptHandleList; + this.messageReceiptHandle = messageReceiptHandleList == null || messageReceiptHandleList.isEmpty() ? + null : messageReceiptHandleList.get(0); + this.renewTime = renewTime; + this.eventType = eventType; + this.futureList = futureList; + this.future = futureList == null || futureList.isEmpty() ? null : futureList.get(0); } public ReceiptHandleGroupKey getKey() { @@ -50,6 +68,10 @@ public MessageReceiptHandle getMessageReceiptHandle() { return messageReceiptHandle; } + public List getMessageReceiptHandleList() { + return messageReceiptHandleList; + } + public long getRenewTime() { return renewTime; } @@ -61,4 +83,8 @@ public EventType getEventType() { public CompletableFuture getFuture() { return future; } + + public List> getFutureList() { + return futureList; + } } diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/config/ProxyConfig.java b/proxy/src/main/java/org/apache/rocketmq/proxy/config/ProxyConfig.java index 5a1a5859305..ecf6426f851 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/config/ProxyConfig.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/config/ProxyConfig.java @@ -29,6 +29,7 @@ import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.apache.rocketmq.common.MixAll; +import org.apache.rocketmq.common.PopAckConstants; import org.apache.rocketmq.common.constant.LoggerName; import org.apache.rocketmq.common.metrics.MetricsExporterType; import org.apache.rocketmq.common.utils.NetworkUtil; @@ -290,6 +291,8 @@ public class ProxyConfig implements ConfigFile { private long remotingWaitTimeMillsInDefaultQueue = 3 * 1000; private boolean enableBatchAck = false; + private boolean enableBatchChangeInvisibleTime = false; + private int batchChangeInvisibleTimeMaxNum = PopAckConstants.DEFAULT_BATCH_CHANGE_INVISIBLE_TIME_MAX_NUM; @Override public void initData() { @@ -1553,6 +1556,22 @@ public void setEnableBatchAck(boolean enableBatchAck) { this.enableBatchAck = enableBatchAck; } + public boolean isEnableBatchChangeInvisibleTime() { + return enableBatchChangeInvisibleTime; + } + + public void setEnableBatchChangeInvisibleTime(boolean enableBatchChangeInvisibleTime) { + this.enableBatchChangeInvisibleTime = enableBatchChangeInvisibleTime; + } + + public int getBatchChangeInvisibleTimeMaxNum() { + return batchChangeInvisibleTimeMaxNum; + } + + public void setBatchChangeInvisibleTimeMaxNum(int batchChangeInvisibleTimeMaxNum) { + this.batchChangeInvisibleTimeMaxNum = batchChangeInvisibleTimeMaxNum; + } + public boolean isEnableMessageBodyEmptyCheck() { return enableMessageBodyEmptyCheck; } diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageResponseStreamWriter.java b/proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageResponseStreamWriter.java index 69bd2a6bc4e..155d4e3e5e3 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageResponseStreamWriter.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageResponseStreamWriter.java @@ -23,6 +23,7 @@ import com.google.protobuf.util.Timestamps; import io.grpc.stub.StreamObserver; import java.time.Duration; +import java.util.ArrayList; import java.util.Iterator; import java.util.List; import org.apache.rocketmq.client.consumer.PopResult; @@ -34,10 +35,12 @@ import org.apache.rocketmq.logging.org.slf4j.Logger; import org.apache.rocketmq.logging.org.slf4j.LoggerFactory; import org.apache.rocketmq.proxy.common.ProxyContext; +import org.apache.rocketmq.proxy.config.ConfigurationManager; import org.apache.rocketmq.proxy.grpc.v2.common.GrpcConverter; import org.apache.rocketmq.proxy.grpc.v2.common.ResponseBuilder; import org.apache.rocketmq.proxy.grpc.v2.common.ResponseWriter; import org.apache.rocketmq.proxy.processor.MessagingProcessor; +import org.apache.rocketmq.proxy.service.message.ReceiptHandleMessage; public class ReceiveMessageResponseStreamWriter { private static final Logger log = LoggerFactory.getLogger(LoggerName.PROXY_LOGGER_NAME); @@ -73,8 +76,7 @@ public void writeAndComplete(ProxyContext ctx, ReceiveMessageRequest request, Po .setStatus(ResponseBuilder.getInstance().buildStatus(Code.OK, Code.OK.name())) .build()); } catch (Throwable t) { - messageFoundList.forEach(messageExt -> - this.processThrowableWhenWriteMessage(t, ctx, request, messageExt)); + this.processThrowableWhenWriteMessages(t, ctx, request, messageFoundList); throw t; } Iterator messageIterator = messageFoundList.iterator(); @@ -86,9 +88,10 @@ public void writeAndComplete(ProxyContext ctx, ReceiveMessageRequest request, Po .setMessage(curMessage) .build()); } catch (Throwable t) { - this.processThrowableWhenWriteMessage(t, ctx, request, curMessageExt); - messageIterator.forEachRemaining(messageExt -> - this.processThrowableWhenWriteMessage(t, ctx, request, messageExt)); + List toNackMessageList = new ArrayList<>(); + toNackMessageList.add(curMessageExt); + messageIterator.forEachRemaining(toNackMessageList::add); + this.processThrowableWhenWriteMessages(t, ctx, request, toNackMessageList); return; } } @@ -122,6 +125,61 @@ protected Message convertToMessage(MessageExt messageExt) { return GrpcConverter.getInstance().buildMessage(messageExt); } + protected void processThrowableWhenWriteMessages(Throwable throwable, + ProxyContext ctx, ReceiveMessageRequest request, List messageExtList) { + if (!ConfigurationManager.getProxyConfig().isEnableBatchChangeInvisibleTime()) { + messageExtList.forEach(messageExt -> this.processThrowableWhenWriteMessage(throwable, ctx, request, messageExt)); + return; + } + + List handleMessageList = new ArrayList<>(); + for (MessageExt messageExt : messageExtList) { + String handle = messageExt.getProperty(MessageConst.PROPERTY_POP_CK); + if (handle == null) { + continue; + } + handleMessageList.add(new ReceiptHandleMessage( + ReceiptHandle.decode(handle), + messageExt.getMsgId(), + messageExt.getProperty(MessageConst.PROPERTY_LITE_TOPIC))); + } + if (handleMessageList.isEmpty()) { + return; + } + if (handleMessageList.size() == 1) { + ReceiptHandleMessage handleMessage = handleMessageList.get(0); + this.messagingProcessor.changeInvisibleTime( + ctx, + handleMessage.getReceiptHandle(), + handleMessage.getMessageId(), + request.getGroup().getName(), + request.getMessageQueue().getTopic().getName(), + NACK_INVISIBLE_TIME, + handleMessage.getLiteTopic(), + MessagingProcessor.DEFAULT_TIMEOUT_MILLS, + true + ).exceptionally(t -> { + log.error("change invisible time failed when nack message after write failed. group={}, topic={}, messageId={}", + request.getGroup().getName(), request.getMessageQueue().getTopic().getName(), handleMessage.getMessageId(), t); + return null; + }); + return; + } + this.messagingProcessor.batchChangeInvisibleTime( + ctx, + handleMessageList, + request.getGroup().getName(), + request.getMessageQueue().getTopic().getName(), + NACK_INVISIBLE_TIME, + MessagingProcessor.DEFAULT_TIMEOUT_MILLS, + true + ).exceptionally(t -> { + log.error("batch change invisible time failed when nack messages after write failed. group={}, topic={}, size={}", + request.getGroup().getName(), request.getMessageQueue().getTopic().getName(), handleMessageList.size(), t); + return null; + }); + } + protected void processThrowableWhenWriteMessage(Throwable throwable, ProxyContext ctx, ReceiveMessageRequest request, MessageExt messageExt) { @@ -137,10 +195,14 @@ protected void processThrowableWhenWriteMessage(Throwable throwable, request.getGroup().getName(), request.getMessageQueue().getTopic().getName(), NACK_INVISIBLE_TIME, - null, + messageExt.getProperty(MessageConst.PROPERTY_LITE_TOPIC), MessagingProcessor.DEFAULT_TIMEOUT_MILLS, true - ); + ).exceptionally(t -> { + log.error("change invisible time failed when nack message after write failed. group={}, topic={}, messageId={}", + request.getGroup().getName(), request.getMessageQueue().getTopic().getName(), messageExt.getMsgId(), t); + return null; + }); } public void writeAndComplete(ProxyContext ctx, Code code, String message) { diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/BatchChangeInvisibleTimeResult.java b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/BatchChangeInvisibleTimeResult.java new file mode 100644 index 00000000000..7dbeb7f27a2 --- /dev/null +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/BatchChangeInvisibleTimeResult.java @@ -0,0 +1,50 @@ +/* + * 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.rocketmq.proxy.processor; + +import org.apache.rocketmq.client.consumer.AckResult; +import org.apache.rocketmq.proxy.common.ProxyException; +import org.apache.rocketmq.proxy.service.message.ReceiptHandleMessage; + +public class BatchChangeInvisibleTimeResult { + private final ReceiptHandleMessage receiptHandleMessage; + private AckResult ackResult; + private ProxyException proxyException; + + public BatchChangeInvisibleTimeResult(ReceiptHandleMessage receiptHandleMessage, AckResult ackResult) { + this.receiptHandleMessage = receiptHandleMessage; + this.ackResult = ackResult; + } + + public BatchChangeInvisibleTimeResult(ReceiptHandleMessage receiptHandleMessage, ProxyException proxyException) { + this.receiptHandleMessage = receiptHandleMessage; + this.proxyException = proxyException; + } + + public ReceiptHandleMessage getReceiptHandleMessage() { + return receiptHandleMessage; + } + + public AckResult getAckResult() { + return ackResult; + } + + public ProxyException getProxyException() { + return proxyException; + } +} diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java index b66d57c62a6..ad11221959c 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java @@ -48,6 +48,8 @@ import org.apache.rocketmq.proxy.common.ProxyException; import org.apache.rocketmq.proxy.common.ProxyExceptionCode; import org.apache.rocketmq.proxy.common.utils.ProxyUtils; +import org.apache.rocketmq.proxy.config.ConfigurationManager; +import org.apache.rocketmq.proxy.config.ProxyConfig; import org.apache.rocketmq.proxy.service.ServiceManager; import org.apache.rocketmq.proxy.service.message.ReceiptHandleMessage; import org.apache.rocketmq.proxy.service.route.AddressableMessageQueue; @@ -162,6 +164,7 @@ private PopResult filterPopResult(ProxyContext ctx, PopResult popResult, Command popMessageResultFilter != null) { List messageExtList = new ArrayList<>(); + List toReturnMessageList = new ArrayList<>(); for (MessageExt messageExt : popResult.getMsgFoundList()) { try { fillUniqIDIfNeed(messageExt); @@ -199,16 +202,21 @@ private PopResult filterPopResult(ProxyContext ctx, PopResult popResult, Command MessagingProcessor.DEFAULT_TIMEOUT_MILLS); break; case TO_RETURN: - this.messagingProcessor.changeInvisibleTime( - ctx, - ReceiptHandle.decode(handleString), - messageExt.getMsgId(), - consumerGroup, - topic, - MessagingProcessor.INVISIBLE_TIME_MS, - null, - MessagingProcessor.DEFAULT_TIMEOUT_MILLS, - true); + if (ConfigurationManager.getProxyConfig().isEnableBatchChangeInvisibleTime()) { + toReturnMessageList.add(new ReceiptHandleMessage( + ReceiptHandle.decode(handleString), messageExt.getMsgId(), liteTopic)); + } else { + this.messagingProcessor.changeInvisibleTime( + ctx, + ReceiptHandle.decode(handleString), + messageExt.getMsgId(), + consumerGroup, + topic, + MessagingProcessor.INVISIBLE_TIME_MS, + null, + MessagingProcessor.DEFAULT_TIMEOUT_MILLS, + true); + } break; case MATCH: default: @@ -220,6 +228,28 @@ private PopResult filterPopResult(ProxyContext ctx, PopResult popResult, Command messageExtList.add(messageExt); } } + if (toReturnMessageList.size() == 1) { + ReceiptHandleMessage handleMessage = toReturnMessageList.get(0); + this.messagingProcessor.changeInvisibleTime( + ctx, + handleMessage.getReceiptHandle(), + handleMessage.getMessageId(), + consumerGroup, + topic, + MessagingProcessor.INVISIBLE_TIME_MS, + handleMessage.getLiteTopic(), + MessagingProcessor.DEFAULT_TIMEOUT_MILLS, + true); + } else if (!toReturnMessageList.isEmpty()) { + this.messagingProcessor.batchChangeInvisibleTime( + ctx, + toReturnMessageList, + consumerGroup, + topic, + MessagingProcessor.INVISIBLE_TIME_MS, + MessagingProcessor.DEFAULT_TIMEOUT_MILLS, + true); + } popResult.setMsgFoundList(messageExtList); } return popResult; @@ -404,6 +434,150 @@ protected CompletableFuture> processBrokerHandle(ProxyConte }); } + public CompletableFuture> batchChangeInvisibleTime( + ProxyContext ctx, + List handleMessageList, + String consumerGroup, + String topic, + long invisibleTime, + long timeoutMillis, + boolean suspend + ) { + CompletableFuture> future = new CompletableFuture<>(); + try { + List batchResultList = new ArrayList<>(handleMessageList.size()); + Map> brokerHandleListMap = new HashMap<>(); + + for (ReceiptHandleMessage handleMessage : handleMessageList) { + if (handleMessage.getReceiptHandle().isExpired()) { + batchResultList.add(new BatchChangeInvisibleTimeResult(handleMessage, EXPIRED_HANDLE_PROXY_EXCEPTION)); + continue; + } + ReceiptHandle handle = handleMessage.getReceiptHandle(); + String realTopic = handle.getRealTopic(topic, consumerGroup); + String batchKey = handle.getBrokerName() + "@" + realTopic; + brokerHandleListMap.computeIfAbsent(batchKey, key -> new ArrayList<>()) + .add(handleMessage); + } + + if (brokerHandleListMap.isEmpty()) { + return FutureUtils.addExecutor(CompletableFuture.completedFuture(batchResultList), this.executor); + } + + CompletableFuture>[] futures = new CompletableFuture[brokerHandleListMap.size()]; + int futureIndex = 0; + for (List brokerHandleList : brokerHandleListMap.values()) { + futures[futureIndex++] = processBrokerChangeInvisibleTime( + ctx, consumerGroup, topic, brokerHandleList, invisibleTime, timeoutMillis, suspend); + } + CompletableFuture.allOf(futures).whenComplete((val, throwable) -> { + if (throwable != null) { + future.completeExceptionally(throwable); + return; + } + for (CompletableFuture> resultFuture : futures) { + batchResultList.addAll(resultFuture.join()); + } + future.complete(batchResultList); + }); + } catch (Throwable t) { + future.completeExceptionally(t); + } + return FutureUtils.addExecutor(future, this.executor); + } + + protected CompletableFuture> processBrokerChangeInvisibleTime( + ProxyContext ctx, String consumerGroup, String topic, List handleMessageList, + long invisibleTime, long timeoutMillis, boolean suspend) { + if (handleMessageList.size() == 1) { + return fallbackChangeInvisibleTime(ctx, consumerGroup, topic, handleMessageList, invisibleTime, + timeoutMillis, suspend); + } + ProxyConfig proxyConfig = ConfigurationManager.getProxyConfig(); + int batchMaxNum = Math.max(1, proxyConfig.getBatchChangeInvisibleTimeMaxNum()); + if (handleMessageList.size() > batchMaxNum) { + List>> futures = new ArrayList<>(); + for (int fromIndex = 0; fromIndex < handleMessageList.size(); fromIndex += batchMaxNum) { + int toIndex = Math.min(handleMessageList.size(), fromIndex + batchMaxNum); + futures.add(processBrokerChangeInvisibleTime(ctx, consumerGroup, topic, + handleMessageList.subList(fromIndex, toIndex), invisibleTime, timeoutMillis, suspend)); + } + return CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).thenApply(ignored -> { + List results = new ArrayList<>(handleMessageList.size()); + for (CompletableFuture> resultFuture : futures) { + results.addAll(resultFuture.join()); + } + return results; + }); + } + CompletableFuture> future = new CompletableFuture<>(); + this.serviceManager.getMessageService().batchChangeInvisibleTime( + ctx, handleMessageList, consumerGroup, topic, invisibleTime, timeoutMillis, suspend) + .whenComplete((ackResults, throwable) -> { + if (throwable == null) { + future.complete(buildBatchChangeInvisibleTimeResults(handleMessageList, ackResults)); + return; + } + log.warn("batch change invisible time failed, fallback to single requests. topic={}, group={}, size={}", + topic, consumerGroup, handleMessageList.size(), throwable); + fallbackChangeInvisibleTime(ctx, consumerGroup, topic, handleMessageList, invisibleTime, timeoutMillis, suspend) + .whenComplete((fallbackResults, fallbackThrowable) -> { + if (fallbackThrowable != null) { + future.completeExceptionally(fallbackThrowable); + } else { + future.complete(fallbackResults); + } + }); + }); + return future; + } + + protected List buildBatchChangeInvisibleTimeResults( + List handleMessageList, List ackResults) { + List results = new ArrayList<>(handleMessageList.size()); + for (int i = 0; i < handleMessageList.size(); i++) { + AckResult ackResult = ackResults != null && i < ackResults.size() ? ackResults.get(i) : null; + if (ackResult == null) { + results.add(new BatchChangeInvisibleTimeResult(handleMessageList.get(i), + new ProxyException(ProxyExceptionCode.INTERNAL_SERVER_ERROR, "batch change invisible time result missing"))); + } else { + ReceiptHandleMessage handleMessage = handleMessageList.get(i); + if (StringUtils.isNotBlank(ackResult.getExtraInfo())) { + AckResult result = new AckResult(); + result.setStatus(ackResult.getStatus()); + result.setPopTime(ackResult.getPopTime()); + result.setExtraInfo(createHandle(ackResult.getExtraInfo(), + handleMessage.getReceiptHandle().getCommitLogOffset())); + results.add(new BatchChangeInvisibleTimeResult(handleMessage, result)); + } else { + results.add(new BatchChangeInvisibleTimeResult(handleMessage, ackResult)); + } + } + } + return results; + } + + protected CompletableFuture> fallbackChangeInvisibleTime( + ProxyContext ctx, String consumerGroup, String topic, List handleMessageList, + long invisibleTime, long timeoutMillis, boolean suspend) { + CompletableFuture[] futures = new CompletableFuture[handleMessageList.size()]; + for (int i = 0; i < handleMessageList.size(); i++) { + ReceiptHandleMessage handleMessage = handleMessageList.get(i); + futures[i] = this.changeInvisibleTime(ctx, handleMessage.getReceiptHandle(), handleMessage.getMessageId(), + consumerGroup, topic, invisibleTime, handleMessage.getLiteTopic(), timeoutMillis, suspend) + .thenApply(ackResult -> new BatchChangeInvisibleTimeResult(handleMessage, ackResult)) + .exceptionally(throwable -> new BatchChangeInvisibleTimeResult(handleMessage, + new ProxyException(ProxyExceptionCode.INTERNAL_SERVER_ERROR, throwable.getMessage(), throwable))); + } + return CompletableFuture.allOf(futures).thenApply(ignored -> { + List results = new ArrayList<>(futures.length); + for (CompletableFuture resultFuture : futures) { + results.add(resultFuture.join()); + } + return results; + }); + } + public CompletableFuture changeInvisibleTime(ProxyContext ctx, ReceiptHandle handle, String messageId, String groupName, String topicName, long invisibleTime, String liteTopic, long timeoutMillis, boolean suspend) { CompletableFuture future = new CompletableFuture<>(); diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/DefaultMessagingProcessor.java b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/DefaultMessagingProcessor.java index 3e7a8894859..801b6f79ead 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/DefaultMessagingProcessor.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/DefaultMessagingProcessor.java @@ -232,6 +232,14 @@ public CompletableFuture> batchAckMessage(ProxyContext ctx, return this.consumerProcessor.batchAckMessage(ctx, handleMessageList, consumerGroup, topic, timeoutMillis); } + @Override + public CompletableFuture> batchChangeInvisibleTime(ProxyContext ctx, + List handleMessageList, String consumerGroup, String topic, long invisibleTime, + long timeoutMillis, boolean suspend) { + return this.consumerProcessor.batchChangeInvisibleTime( + ctx, handleMessageList, consumerGroup, topic, invisibleTime, timeoutMillis, suspend); + } + @Override public CompletableFuture changeInvisibleTime(ProxyContext ctx, ReceiptHandle handle, String messageId, String groupName, String topicName, long invisibleTime, String liteTopic, long timeoutMillis, boolean suspend) { diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/MessagingProcessor.java b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/MessagingProcessor.java index a1500dbdedd..8142a3aa2e1 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/MessagingProcessor.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/MessagingProcessor.java @@ -234,6 +234,39 @@ CompletableFuture> batchAckMessage( long timeoutMillis ); + /** + * Change invisible time for handles grouped by broker. + * + * @param handleMessageList non-empty handles from the same broker + * @param suspend whether the new checkpoint should be marked suspended + */ + default CompletableFuture> batchChangeInvisibleTime( + ProxyContext ctx, + List handleMessageList, + String consumerGroup, + String topic, + long invisibleTime, + boolean suspend + ) { + return batchChangeInvisibleTime(ctx, handleMessageList, consumerGroup, topic, invisibleTime, DEFAULT_TIMEOUT_MILLS, suspend); + } + + /** + * Change invisible time for handles grouped by broker. + * + * @param handleMessageList non-empty handles from the same broker + * @param suspend whether the new checkpoint should be marked suspended + */ + CompletableFuture> batchChangeInvisibleTime( + ProxyContext ctx, + List handleMessageList, + String consumerGroup, + String topic, + long invisibleTime, + long timeoutMillis, + boolean suspend + ); + default CompletableFuture changeInvisibleTime( ProxyContext ctx, ReceiptHandle handle, diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessor.java b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessor.java index bc3730aed9a..81ae3abb068 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessor.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessor.java @@ -18,6 +18,12 @@ package org.apache.rocketmq.proxy.processor; import io.netty.channel.Channel; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CompletableFuture; +import org.apache.rocketmq.client.consumer.AckResult; import org.apache.rocketmq.common.constant.LoggerName; import org.apache.rocketmq.common.consumer.ReceiptHandle; import org.apache.rocketmq.common.state.StateEventListener; @@ -27,10 +33,13 @@ import org.apache.rocketmq.proxy.common.ProxyContext; import org.apache.rocketmq.proxy.common.RenewEvent; import org.apache.rocketmq.proxy.service.ServiceManager; +import org.apache.rocketmq.proxy.service.message.ReceiptHandleMessage; import org.apache.rocketmq.proxy.service.receipt.DefaultReceiptHandleManager; public class ReceiptHandleProcessor extends AbstractProcessor { protected final static Logger log = LoggerFactory.getLogger(LoggerName.PROXY_LOGGER_NAME); + private static final String RENEW_BATCH_KEY_SEPARATOR = "\u0001"; + protected DefaultReceiptHandleManager receiptHandleManager; public ReceiptHandleProcessor(MessagingProcessor messagingProcessor, ServiceManager serviceManager) { @@ -38,7 +47,15 @@ public ReceiptHandleProcessor(MessagingProcessor messagingProcessor, ServiceMana StateEventListener eventListener = event -> { ProxyContext context = createContext(event.getEventType().name()) .setChannel(event.getKey().getChannel()); - MessageReceiptHandle messageReceiptHandle = event.getMessageReceiptHandle(); + List messageReceiptHandleList = event.getMessageReceiptHandleList(); + if (messageReceiptHandleList == null || messageReceiptHandleList.isEmpty()) { + return; + } + if (messageReceiptHandleList.size() > 1) { + batchChangeInvisibleTime(context, event); + return; + } + MessageReceiptHandle messageReceiptHandle = messageReceiptHandleList.get(0); ReceiptHandle handle = ReceiptHandle.decode(messageReceiptHandle.getReceiptHandleStr()); messagingProcessor .changeInvisibleTime(context, handle, messageReceiptHandle.getMessageId(), @@ -56,6 +73,74 @@ public ReceiptHandleProcessor(MessagingProcessor messagingProcessor, ServiceMana this.appendStartAndShutdown(receiptHandleManager); } + protected void batchChangeInvisibleTime(ProxyContext context, RenewEvent event) { + List messageReceiptHandleList = event.getMessageReceiptHandleList(); + List> futureList = event.getFutureList(); + Map> indexesByGroupAndTopic = new HashMap<>(); + for (int i = 0; i < messageReceiptHandleList.size(); i++) { + MessageReceiptHandle messageReceiptHandle = messageReceiptHandleList.get(i); + String key = messageReceiptHandle.getGroup() + RENEW_BATCH_KEY_SEPARATOR + messageReceiptHandle.getTopic(); + indexesByGroupAndTopic.computeIfAbsent(key, ignored -> new ArrayList<>()).add(i); + } + + for (List indexes : indexesByGroupAndTopic.values()) { + MessageReceiptHandle firstHandle = messageReceiptHandleList.get(indexes.get(0)); + if (indexes.size() == 1) { + int index = indexes.get(0); + CompletableFuture future = futureList.get(index); + ReceiptHandle handle = ReceiptHandle.decode(firstHandle.getReceiptHandleStr()); + messagingProcessor + .changeInvisibleTime(context, handle, firstHandle.getMessageId(), + firstHandle.getGroup(), firstHandle.getTopic(), + event.getRenewTime(), firstHandle.getLiteTopic()) + .whenComplete((ackResult, throwable) -> { + if (throwable != null) { + future.completeExceptionally(throwable); + } else { + future.complete(ackResult); + } + }); + continue; + } + + List handleMessageList = new ArrayList<>(indexes.size()); + for (Integer index : indexes) { + MessageReceiptHandle messageReceiptHandle = messageReceiptHandleList.get(index); + handleMessageList.add(new ReceiptHandleMessage( + ReceiptHandle.decode(messageReceiptHandle.getReceiptHandleStr()), + messageReceiptHandle.getMessageId(), + messageReceiptHandle.getLiteTopic())); + } + messagingProcessor.batchChangeInvisibleTime( + context, + handleMessageList, + firstHandle.getGroup(), + firstHandle.getTopic(), + event.getRenewTime(), + MessagingProcessor.DEFAULT_TIMEOUT_MILLS, + false) + .whenComplete((results, throwable) -> { + if (throwable != null) { + indexes.forEach(index -> futureList.get(index).completeExceptionally(throwable)); + return; + } + for (int i = 0; i < indexes.size(); i++) { + CompletableFuture future = futureList.get(indexes.get(i)); + if (results == null || i >= results.size()) { + future.completeExceptionally(new IllegalStateException("batch change invisible time result missing")); + continue; + } + BatchChangeInvisibleTimeResult result = results.get(i); + if (result.getProxyException() != null) { + future.completeExceptionally(result.getProxyException()); + } else { + future.complete(result.getAckResult()); + } + } + }); + } + } + protected ProxyContext createContext(String actionName) { return ProxyContext.createForInner(this.getClass().getSimpleName() + actionName); } diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/ClusterMessageService.java b/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/ClusterMessageService.java index 77c4ef60f14..2c0c289e21e 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/ClusterMessageService.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/ClusterMessageService.java @@ -36,6 +36,8 @@ import org.apache.rocketmq.proxy.service.route.AddressableMessageQueue; import org.apache.rocketmq.proxy.service.route.TopicRouteService; import org.apache.rocketmq.remoting.protocol.RemotingCommand; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeRequestBody; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; import org.apache.rocketmq.remoting.protocol.body.LockBatchRequestBody; import org.apache.rocketmq.remoting.protocol.body.UnlockBatchRequestBody; import org.apache.rocketmq.remoting.protocol.header.AckMessageRequestHeader; @@ -169,6 +171,34 @@ public CompletableFuture batchAckMessage(ProxyContext ctx, List> batchChangeInvisibleTime(ProxyContext ctx, + List handleList, String consumerGroup, String topic, long invisibleTime, + long timeoutMillis, boolean suspend) { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + String realTopic = handleList.get(0).getReceiptHandle().getRealTopic(topic, consumerGroup); + requestBody.setEntries(handleList.stream().map(message -> { + ReceiptHandle handle = message.getReceiptHandle(); + ChangeInvisibleTimeRequestEntry entry = new ChangeInvisibleTimeRequestEntry(); + entry.setConsumerGroup(consumerGroup); + entry.setTopic(handle.getRealTopic(topic, consumerGroup)); + entry.setQueueId(handle.getQueueId()); + entry.setExtraInfo(handle.getReceiptHandle()); + entry.setOffset(handle.getOffset()); + entry.setInvisibleTime(invisibleTime); + entry.setLiteTopic(message.getLiteTopic()); + entry.setSuspend(suspend); + return entry; + }).collect(Collectors.toList())); + return this.mqClientAPIFactory.getClient().batchChangeInvisibleTimeAsync( + this.resolveBrokerAddrInReceiptHandle(ctx, handleList.get(0).getReceiptHandle()), + realTopic, + consumerGroup, + requestBody, + timeoutMillis + ); + } + @Override public CompletableFuture pullMessage(ProxyContext ctx, AddressableMessageQueue messageQueue, PullMessageRequestHeader requestHeader, long timeoutMillis) { diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java b/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java index c93fa93983c..9738443ca25 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java @@ -57,9 +57,14 @@ import org.apache.rocketmq.remoting.protocol.ResponseCode; import org.apache.rocketmq.remoting.protocol.body.BatchAck; import org.apache.rocketmq.remoting.protocol.body.BatchAckMessageRequestBody; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeRequestBody; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeResponseBody; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeResponseEntry; import org.apache.rocketmq.remoting.protocol.body.LockBatchRequestBody; import org.apache.rocketmq.remoting.protocol.body.UnlockBatchRequestBody; import org.apache.rocketmq.remoting.protocol.header.AckMessageRequestHeader; +import org.apache.rocketmq.remoting.protocol.header.BatchChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeResponseHeader; import org.apache.rocketmq.remoting.protocol.header.ConsumerSendMsgBackRequestHeader; @@ -431,6 +436,82 @@ public CompletableFuture batchAckMessage(ProxyContext ctx, List> batchChangeInvisibleTime(ProxyContext ctx, + List handleList, String consumerGroup, String topic, long invisibleTime, + long timeoutMillis, boolean suspend) { + SimpleChannel channel = channelManager.createChannel(ctx); + ChannelHandlerContext channelHandlerContext = channel.getChannelHandlerContext(); + BatchChangeInvisibleTimeRequestHeader requestHeader = new BatchChangeInvisibleTimeRequestHeader(); + requestHeader.setConsumerGroup(consumerGroup); + requestHeader.setTopic(handleList.get(0).getReceiptHandle().getRealTopic(topic, consumerGroup)); + RemotingCommand command = LocalRemotingCommand.createRequestCommand( + RequestCode.BATCH_CHANGE_MESSAGE_INVISIBLETIME, requestHeader); + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + List entries = new ArrayList<>(handleList.size()); + for (ReceiptHandleMessage receiptHandleMessage : handleList) { + ReceiptHandle handle = receiptHandleMessage.getReceiptHandle(); + ChangeInvisibleTimeRequestEntry entry = new ChangeInvisibleTimeRequestEntry(); + entry.setConsumerGroup(consumerGroup); + entry.setTopic(handle.getRealTopic(topic, consumerGroup)); + entry.setQueueId(handle.getQueueId()); + entry.setExtraInfo(handle.getReceiptHandle()); + entry.setOffset(handle.getOffset()); + entry.setInvisibleTime(invisibleTime); + entry.setLiteTopic(receiptHandleMessage.getLiteTopic()); + entry.setSuspend(suspend); + entries.add(entry); + } + requestBody.setEntries(entries); + command.setBody(requestBody.encode()); + + CompletableFuture future = new CompletableFuture<>(); + try { + future = brokerController.getChangeInvisibleTimeProcessor() + .processRequestAsync(channelHandlerContext.channel(), command, true); + } catch (Exception e) { + log.error("Fail to process batchChangeInvisibleTime command", e); + future.completeExceptionally(e); + } + return future.thenApply(r -> { + BatchChangeInvisibleTimeResponseBody responseBody = + BatchChangeInvisibleTimeResponseBody.decode(r.getBody(), BatchChangeInvisibleTimeResponseBody.class); + List responseEntries = responseBody == null || responseBody.getEntries() == null ? + Collections.emptyList() : responseBody.getEntries(); + if (responseEntries.size() != handleList.size()) { + throw new IllegalStateException(String.format( + "batchChangeInvisibleTime response size mismatch, request=%d, response=%d", + handleList.size(), responseEntries.size())); + } + + List resultList = new ArrayList<>(responseEntries.size()); + for (int i = 0; i < responseEntries.size(); i++) { + ChangeInvisibleTimeResponseEntry responseEntry = responseEntries.get(i); + ReceiptHandle handle = handleList.get(i).getReceiptHandle(); + AckResult ackResult = new AckResult(); + if (responseEntry != null && ResponseCode.SUCCESS == responseEntry.getCode()) { + ackResult.setStatus(AckStatus.OK); + ackResult.setPopTime(responseEntry.getPopTime()); + ackResult.setExtraInfo(ReceiptHandle.builder() + .startOffset(handle.getStartOffset()) + .retrieveTime(responseEntry.getPopTime()) + .invisibleTime(responseEntry.getInvisibleTime()) + .reviveQueueId(responseEntry.getReviveQid()) + .topicType(handle.getTopicType()) + .brokerName(handle.getBrokerName()) + .queueId(handle.getQueueId()) + .offset(handle.getOffset()) + .build() + .encode()); + } else { + ackResult.setStatus(AckStatus.NO_EXIST); + } + resultList.add(ackResult); + } + return resultList; + }); + } + @Override public CompletableFuture pullMessage(ProxyContext ctx, AddressableMessageQueue messageQueue, PullMessageRequestHeader requestHeader, long timeoutMillis) { diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/MessageService.java b/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/MessageService.java index 1e828c36fd9..2550621659c 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/MessageService.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/MessageService.java @@ -108,6 +108,22 @@ CompletableFuture batchAckMessage( long timeoutMillis ); + /** + * Change invisible time for handles that have already been grouped by broker. + * + * @param handleList non-empty handles from the same broker + * @param suspend whether the new checkpoint should be marked suspended + */ + CompletableFuture> batchChangeInvisibleTime( + ProxyContext ctx, + List handleList, + String consumerGroup, + String topic, + long invisibleTime, + long timeoutMillis, + boolean suspend + ); + CompletableFuture pullMessage( ProxyContext ctx, AddressableMessageQueue messageQueue, diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/ReceiptHandleMessage.java b/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/ReceiptHandleMessage.java index ae63fed491e..d57e0b8897e 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/ReceiptHandleMessage.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/service/message/ReceiptHandleMessage.java @@ -23,10 +23,16 @@ public class ReceiptHandleMessage { private final ReceiptHandle receiptHandle; private final String messageId; + private final String liteTopic; public ReceiptHandleMessage(ReceiptHandle receiptHandle, String messageId) { + this(receiptHandle, messageId, null); + } + + public ReceiptHandleMessage(ReceiptHandle receiptHandle, String messageId, String liteTopic) { this.receiptHandle = receiptHandle; this.messageId = messageId; + this.liteTopic = liteTopic; } public ReceiptHandle getReceiptHandle() { @@ -36,4 +42,8 @@ public ReceiptHandle getReceiptHandle() { public String getMessageId() { return messageId; } + + public String getLiteTopic() { + return liteTopic; + } } diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/service/receipt/DefaultReceiptHandleManager.java b/proxy/src/main/java/org/apache/rocketmq/proxy/service/receipt/DefaultReceiptHandleManager.java index f9dfd825337..a92a26766a8 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/service/receipt/DefaultReceiptHandleManager.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/service/receipt/DefaultReceiptHandleManager.java @@ -19,6 +19,9 @@ import com.google.common.base.Stopwatch; import io.netty.channel.Channel; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -166,6 +169,21 @@ protected void scheduleRenewTask() { } ReceiptHandleGroup group = entry.getValue(); + if (proxyConfig.isEnableBatchChangeInvisibleTime()) { + List renewMessageList = new ArrayList<>(); + group.scan((msgID, handleStr, v) -> { + long current = System.currentTimeMillis(); + ReceiptHandle handle = ReceiptHandle.decode(v.getReceiptHandleStr()); + if (handle.getNextVisibleTime() - current <= proxyConfig.getRenewAheadTimeMillis()) { + renewMessageList.add(new RenewMessage(msgID, handleStr)); + } + }); + if (!renewMessageList.isEmpty()) { + renewalWorkerService.submit(() -> renewMessageBatch(createContext("RenewMessage"), key, group, + renewMessageList)); + } + continue; + } group.scan((msgID, handleStr, v) -> { long current = System.currentTimeMillis(); ReceiptHandle handle = ReceiptHandle.decode(v.getReceiptHandleStr()); @@ -192,17 +210,56 @@ protected void renewMessage(ProxyContext context, ReceiptHandleGroupKey key, Rec } protected CompletableFuture startRenewMessage(ProxyContext context, ReceiptHandleGroupKey key, MessageReceiptHandle messageReceiptHandle) { + RenewEventData renewEventData = prepareRenewMessage(context, key, messageReceiptHandle); + if (renewEventData.getEventType() != null) { + eventListener.fireEvent(new RenewEvent(key, messageReceiptHandle, renewEventData.getRenewTime(), + renewEventData.getEventType(), renewEventData.getAckFuture())); + } + return renewEventData.getResultFuture(); + } + + protected void renewMessageBatch(ProxyContext context, ReceiptHandleGroupKey key, ReceiptHandleGroup group, + List renewMessageList) { + Map> eventDataMap = new HashMap<>(); + for (RenewMessage renewMessage : renewMessageList) { + try { + group.computeIfPresent(renewMessage.getMsgID(), renewMessage.getHandleStr(), messageReceiptHandle -> { + RenewEventData renewEventData = prepareRenewMessage(context, key, messageReceiptHandle); + if (renewEventData.getEventType() != null) { + String batchKey = renewEventData.getEventType().name() + "@" + renewEventData.getRenewTime(); + eventDataMap.computeIfAbsent(batchKey, ignore -> new ArrayList<>()).add(renewEventData); + } + return renewEventData.getResultFuture(); + }, 0); + } catch (Exception e) { + log.error("error when renew message. msgID:{}, handleStr:{}", renewMessage.getMsgID(), renewMessage.getHandleStr(), e); + } + } + + for (List renewEventDataList : eventDataMap.values()) { + List handleList = new ArrayList<>(renewEventDataList.size()); + List> futureList = new ArrayList<>(renewEventDataList.size()); + for (RenewEventData renewEventData : renewEventDataList) { + handleList.add(renewEventData.getMessageReceiptHandle()); + futureList.add(renewEventData.getAckFuture()); + } + RenewEventData first = renewEventDataList.get(0); + eventListener.fireEvent(new RenewEvent(key, handleList, first.getRenewTime(), first.getEventType(), futureList)); + } + } + + protected RenewEventData prepareRenewMessage(ProxyContext context, ReceiptHandleGroupKey key, + MessageReceiptHandle messageReceiptHandle) { CompletableFuture resFuture = new CompletableFuture<>(); ProxyConfig proxyConfig = ConfigurationManager.getProxyConfig(); long current = System.currentTimeMillis(); try { if (messageReceiptHandle.getRenewRetryTimes() >= proxyConfig.getMaxRenewRetryTimes()) { log.warn("handle has exceed max renewRetryTimes. handle:{}", messageReceiptHandle); - return CompletableFuture.completedFuture(null); + return RenewEventData.completed(messageReceiptHandle, resFuture, null); } if (current - messageReceiptHandle.getConsumeTimestamp() < proxyConfig.getRenewMaxTimeMillis()) { CompletableFuture future = new CompletableFuture<>(); - eventListener.fireEvent(new RenewEvent(key, messageReceiptHandle, RENEW_POLICY.nextDelayDuration(messageReceiptHandle.getRenewTimes()), RenewEvent.EventType.RENEW, future)); future.whenComplete((ackResult, throwable) -> { if (throwable != null) { log.error("error when renew. handle:{}", messageReceiptHandle, throwable); @@ -222,28 +279,31 @@ protected CompletableFuture startRenewMessage(ProxyContext resFuture.complete(null); } }); + return new RenewEventData(messageReceiptHandle, RENEW_POLICY.nextDelayDuration(messageReceiptHandle.getRenewTimes()), + RenewEvent.EventType.RENEW, future, resFuture); } else { SubscriptionGroupConfig subscriptionGroupConfig = metadataService.getSubscriptionGroupConfig(context, messageReceiptHandle.getGroup()); if (subscriptionGroupConfig == null) { log.error("group's subscriptionGroupConfig is null when renew. handle: {}", messageReceiptHandle); - return CompletableFuture.completedFuture(null); + return RenewEventData.completed(messageReceiptHandle, resFuture, null); } RetryPolicy retryPolicy = subscriptionGroupConfig.getGroupRetryPolicy().getRetryPolicy(); CompletableFuture future = new CompletableFuture<>(); - eventListener.fireEvent(new RenewEvent(key, messageReceiptHandle, retryPolicy.nextDelayDuration(messageReceiptHandle.getReconsumeTimes()), RenewEvent.EventType.STOP_RENEW, future)); future.whenComplete((ackResult, throwable) -> { if (throwable != null) { log.error("error when nack in renew. handle:{}", messageReceiptHandle, throwable); } resFuture.complete(null); }); + return new RenewEventData(messageReceiptHandle, retryPolicy.nextDelayDuration(messageReceiptHandle.getReconsumeTimes()), + RenewEvent.EventType.STOP_RENEW, future, resFuture); } } catch (Throwable t) { log.error("unexpect error when renew message, stop to renew it. handle:{}", messageReceiptHandle, t); resFuture.complete(null); } - return resFuture; + return RenewEventData.completed(messageReceiptHandle, resFuture, null); } protected void clearGroup(ReceiptHandleGroupKey key) { @@ -261,21 +321,47 @@ private void returnHandleGroup(ReceiptHandleGroupKey key, ReceiptHandleGroup han return; } ProxyConfig proxyConfig = ConfigurationManager.getProxyConfig(); + if (proxyConfig.isEnableBatchChangeInvisibleTime()) { + fireClearGroupEventBatch(key, handleGroup, proxyConfig); + } else { + handleGroup.scan((msgID, handle, v) -> { + try { + handleGroup.computeIfPresent(msgID, handle, messageReceiptHandle -> { + CompletableFuture future = new CompletableFuture<>(); + eventListener.fireEvent(new RenewEvent(key, messageReceiptHandle, + proxyConfig.getInvisibleTimeMillisWhenClear(), RenewEvent.EventType.CLEAR_GROUP, future)); + return CompletableFuture.completedFuture(null); + }, 0); + } catch (Exception e) { + log.error("error when clear handle for group. key:{}", key, e); + } + }); + } + // scheduleRenewTask will trigger cleanup again + if (!handleGroup.isEmpty()) { + log.warn("The handle cannot be completely cleared, the remaining quantity is {}, key:{}", handleGroup.getHandleNum(), key); + receiptHandleGroupMap.putIfAbsent(key, handleGroup); + } + } + + protected void fireClearGroupEventBatch(ReceiptHandleGroupKey key, ReceiptHandleGroup handleGroup, + ProxyConfig proxyConfig) { + List handleList = new ArrayList<>(); + List> futureList = new ArrayList<>(); handleGroup.scan((msgID, handle, v) -> { try { handleGroup.computeIfPresent(msgID, handle, messageReceiptHandle -> { - CompletableFuture future = new CompletableFuture<>(); - eventListener.fireEvent(new RenewEvent(key, messageReceiptHandle, proxyConfig.getInvisibleTimeMillisWhenClear(), RenewEvent.EventType.CLEAR_GROUP, future)); + handleList.add(messageReceiptHandle); + futureList.add(new CompletableFuture<>()); return CompletableFuture.completedFuture(null); }, 0); } catch (Exception e) { log.error("error when clear handle for group. key:{}", key, e); } }); - // scheduleRenewTask will trigger cleanup again - if (!handleGroup.isEmpty()) { - log.warn("The handle cannot be completely cleared, the remaining quantity is {}, key:{}", handleGroup.getHandleNum(), key); - receiptHandleGroupMap.putIfAbsent(key, handleGroup); + if (!handleList.isEmpty()) { + eventListener.fireEvent(new RenewEvent(key, handleList, proxyConfig.getInvisibleTimeMillisWhenClear(), + RenewEvent.EventType.CLEAR_GROUP, futureList)); } } @@ -303,4 +389,66 @@ protected boolean renewExceptionNeedRetry(Throwable t) { protected ProxyContext createContext(String actionName) { return ProxyContext.createForInner(this.getClass().getSimpleName() + actionName); } + + protected static class RenewMessage { + private final String msgID; + private final String handleStr; + + public RenewMessage(String msgID, String handleStr) { + this.msgID = msgID; + this.handleStr = handleStr; + } + + public String getMsgID() { + return msgID; + } + + public String getHandleStr() { + return handleStr; + } + } + + protected static class RenewEventData { + private final MessageReceiptHandle messageReceiptHandle; + private final long renewTime; + private final RenewEvent.EventType eventType; + private final CompletableFuture ackFuture; + private final CompletableFuture resultFuture; + + public RenewEventData(MessageReceiptHandle messageReceiptHandle, long renewTime, + RenewEvent.EventType eventType, CompletableFuture ackFuture, + CompletableFuture resultFuture) { + this.messageReceiptHandle = messageReceiptHandle; + this.renewTime = renewTime; + this.eventType = eventType; + this.ackFuture = ackFuture; + this.resultFuture = resultFuture; + } + + public static RenewEventData completed(MessageReceiptHandle messageReceiptHandle, + CompletableFuture resultFuture, MessageReceiptHandle result) { + resultFuture.complete(result); + return new RenewEventData(messageReceiptHandle, 0, null, null, resultFuture); + } + + public MessageReceiptHandle getMessageReceiptHandle() { + return messageReceiptHandle; + } + + public long getRenewTime() { + return renewTime; + } + + public RenewEvent.EventType getEventType() { + return eventType; + } + + public CompletableFuture getAckFuture() { + return ackFuture; + } + + public CompletableFuture getResultFuture() { + return resultFuture; + } + } } diff --git a/proxy/src/test/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageResponseStreamWriterTest.java b/proxy/src/test/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageResponseStreamWriterTest.java index 2bc281376ee..f3887442793 100644 --- a/proxy/src/test/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageResponseStreamWriterTest.java +++ b/proxy/src/test/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageResponseStreamWriterTest.java @@ -25,6 +25,7 @@ import apache.rocketmq.v2.ReceiveMessageResponse; import apache.rocketmq.v2.Resource; import io.grpc.stub.StreamObserver; +import java.lang.reflect.Method; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; @@ -38,9 +39,11 @@ import org.apache.rocketmq.common.message.MessageClientIDSetter; import org.apache.rocketmq.common.message.MessageConst; import org.apache.rocketmq.common.message.MessageExt; -import java.lang.reflect.Method; import org.apache.rocketmq.proxy.common.ProxyContext; +import org.apache.rocketmq.proxy.config.ConfigurationManager; import org.apache.rocketmq.proxy.grpc.v2.BaseActivityTest; +import org.apache.rocketmq.proxy.processor.BatchChangeInvisibleTimeResult; +import org.apache.rocketmq.proxy.service.message.ReceiptHandleMessage; import org.apache.rocketmq.remoting.protocol.header.ExtraInfoUtil; import org.junit.Before; import org.junit.Test; @@ -50,6 +53,7 @@ import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -58,6 +62,7 @@ import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -206,6 +211,112 @@ public void testNackMessageWithSuspendTrue() { assertTrue("Suspend should be true for nack", changeInvisibleTimeSuspendCaptor.getValue()); } + @Test + public void testBatchNackWhenWriteStatusFailed() { + ConfigurationManager.getProxyConfig().setEnableBatchChangeInvisibleTime(true); + ArgumentCaptor handleMessageListCaptor = ArgumentCaptor.forClass(List.class); + doReturn(CompletableFuture.completedFuture(new ArrayList())) + .when(this.messagingProcessor).batchChangeInvisibleTime( + any(), handleMessageListCaptor.capture(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + doThrow(new RuntimeException("client cancelled")).when(streamObserver).onNext(any()); + + List messageExtList = new ArrayList<>(); + messageExtList.add(createMessageExt(TOPIC, "tag")); + messageExtList.add(createMessageExt(TOPIC, "tag")); + writer.writeAndComplete( + ProxyContext.create(), + createReceiveMessageRequest(), + new PopResult(PopStatus.FOUND, messageExtList) + ); + + verify(this.messagingProcessor, times(1)).batchChangeInvisibleTime( + any(), anyList(), eq(CONSUMER_GROUP), eq(TOPIC), + eq(ReceiveMessageResponseStreamWriter.NACK_INVISIBLE_TIME), + eq(org.apache.rocketmq.proxy.processor.MessagingProcessor.DEFAULT_TIMEOUT_MILLS), + eq(true)); + verify(this.messagingProcessor, never()).changeInvisibleTime( + any(), any(), anyString(), anyString(), anyString(), anyLong(), any(), anyLong(), anyBoolean()); + assertEquals(2, handleMessageListCaptor.getValue().size()); + assertEquals(messageExtList.get(0).getMsgId(), + ((ReceiptHandleMessage) handleMessageListCaptor.getValue().get(0)).getMessageId()); + assertEquals(messageExtList.get(1).getMsgId(), + ((ReceiptHandleMessage) handleMessageListCaptor.getValue().get(1)).getMessageId()); + } + + @Test + public void testSingleNackWhenWriteStatusFailedUseSingleChangeInvisibleTime() { + ConfigurationManager.getProxyConfig().setEnableBatchChangeInvisibleTime(true); + doReturn(CompletableFuture.completedFuture(mock(AckResult.class))) + .when(this.messagingProcessor).changeInvisibleTime( + any(), any(), anyString(), anyString(), anyString(), anyLong(), any(), anyLong(), anyBoolean()); + doThrow(new RuntimeException("client cancelled")).when(streamObserver).onNext(any()); + + List messageExtList = new ArrayList<>(); + messageExtList.add(createMessageExt(TOPIC, "tag")); + writer.writeAndComplete( + ProxyContext.create(), + createReceiveMessageRequest(), + new PopResult(PopStatus.FOUND, messageExtList) + ); + + verify(this.messagingProcessor, times(1)).changeInvisibleTime( + any(), any(), eq(messageExtList.get(0).getMsgId()), eq(CONSUMER_GROUP), eq(TOPIC), + eq(ReceiveMessageResponseStreamWriter.NACK_INVISIBLE_TIME), eq(null), + eq(org.apache.rocketmq.proxy.processor.MessagingProcessor.DEFAULT_TIMEOUT_MILLS), eq(true)); + verify(this.messagingProcessor, never()).batchChangeInvisibleTime( + any(), anyList(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + } + + @Test + public void testBatchNackRemainingMessagesWhenClientCancelMidStream() { + ConfigurationManager.getProxyConfig().setEnableBatchChangeInvisibleTime(true); + ArgumentCaptor handleMessageListCaptor = ArgumentCaptor.forClass(List.class); + doReturn(CompletableFuture.completedFuture(new ArrayList())) + .when(this.messagingProcessor).batchChangeInvisibleTime( + any(), handleMessageListCaptor.capture(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + + AtomicInteger onNextCallNum = new AtomicInteger(0); + doAnswer(mock -> { + if (onNextCallNum.incrementAndGet() == 3) { + throw new RuntimeException("client cancelled"); + } + return null; + }).when(streamObserver).onNext(any()); + + List messageExtList = new ArrayList<>(); + messageExtList.add(createMessageExt(TOPIC, "tag")); + messageExtList.add(createMessageExt(TOPIC, "tag")); + messageExtList.add(createMessageExt(TOPIC, "tag")); + writer.writeAndComplete( + ProxyContext.create(), + createReceiveMessageRequest(), + new PopResult(PopStatus.FOUND, messageExtList) + ); + + verify(this.messagingProcessor, times(1)).batchChangeInvisibleTime( + any(), anyList(), eq(CONSUMER_GROUP), eq(TOPIC), + eq(ReceiveMessageResponseStreamWriter.NACK_INVISIBLE_TIME), + eq(org.apache.rocketmq.proxy.processor.MessagingProcessor.DEFAULT_TIMEOUT_MILLS), + eq(true)); + verify(this.messagingProcessor, never()).changeInvisibleTime( + any(), any(), anyString(), anyString(), anyString(), anyLong(), any(), anyLong(), anyBoolean()); + assertEquals(2, handleMessageListCaptor.getValue().size()); + assertEquals(messageExtList.get(1).getMsgId(), + ((ReceiptHandleMessage) handleMessageListCaptor.getValue().get(0)).getMessageId()); + assertEquals(messageExtList.get(2).getMsgId(), + ((ReceiptHandleMessage) handleMessageListCaptor.getValue().get(1)).getMessageId()); + } + + private static ReceiveMessageRequest createReceiveMessageRequest() { + return ReceiveMessageRequest.newBuilder() + .setGroup(Resource.newBuilder().setName(CONSUMER_GROUP).build()) + .setMessageQueue(MessageQueue.newBuilder().setTopic(Resource.newBuilder().setName(TOPIC).build()).build()) + .setFilterExpression(FilterExpression.newBuilder() + .setType(FilterType.TAG) + .setExpression("*") + .build()) + .build(); + } private static MessageExt createMessageExt(String topic, String tags) { String msgId = MessageClientIDSetter.createUniqID(); diff --git a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java index 4df343c409e..755a7ed3d18 100644 --- a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java +++ b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java @@ -46,6 +46,7 @@ import org.apache.rocketmq.proxy.common.ProxyContext; import org.apache.rocketmq.proxy.common.ProxyExceptionCode; import org.apache.rocketmq.common.utils.FutureUtils; +import org.apache.rocketmq.proxy.config.ConfigurationManager; import org.apache.rocketmq.proxy.common.utils.ProxyUtils; import org.apache.rocketmq.proxy.service.message.ReceiptHandleMessage; import org.apache.rocketmq.proxy.service.route.AddressableMessageQueue; @@ -68,6 +69,7 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; @@ -75,6 +77,7 @@ import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -281,6 +284,196 @@ public void testBatchAckMessage() throws Throwable { assertNull(msgBatchAckResult.get(errThrowMessage.getMsgId()).getAckResult()); } + @Test + public void testBatchChangeInvisibleTime() throws Throwable { + String brokerName1 = "brokerName1"; + String brokerName2 = "brokerName2"; + MessageExt expireMessage = createMessageExt(TOPIC, "", 0, 3000, System.currentTimeMillis() - 10000, + 0, 0, 0, 0, brokerName1); + ReceiptHandle expireHandle = create(expireMessage); + + List receiptHandleMessageList = new ArrayList<>(); + receiptHandleMessageList.add(new ReceiptHandleMessage(expireHandle, expireMessage.getMsgId())); + List broker1Msg = new ArrayList<>(); + List broker2Msg = new ArrayList<>(); + + long now = System.currentTimeMillis(); + int msgNum = 3; + for (int i = 0; i < msgNum; i++) { + MessageExt brokerMessage = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, i + 1, brokerName1); + ReceiptHandle brokerHandle = create(brokerMessage); + receiptHandleMessageList.add(new ReceiptHandleMessage(brokerHandle, brokerMessage.getMsgId())); + broker1Msg.add(brokerMessage.getMsgId()); + } + for (int i = 0; i < msgNum; i++) { + MessageExt brokerMessage = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, i + 1, brokerName2); + ReceiptHandle brokerHandle = create(brokerMessage); + receiptHandleMessageList.add(new ReceiptHandleMessage(brokerHandle, brokerMessage.getMsgId())); + broker2Msg.add(brokerMessage.getMsgId()); + } + + String newExtraInfo = "newExtraInfo"; + long popTime = 12345L; + doAnswer((Answer>>) invocation -> { + List handleMessageList = invocation.getArgument(1, List.class); + List ackResultList = new ArrayList<>(); + String brokerName = handleMessageList.get(0).getReceiptHandle().getBrokerName(); + for (ReceiptHandleMessage ignored : handleMessageList) { + AckResult ackResult = new AckResult(); + if (brokerName.equals(brokerName1)) { + ackResult.setStatus(AckStatus.OK); + ackResult.setPopTime(popTime); + ackResult.setExtraInfo(newExtraInfo); + } else { + ackResult.setStatus(AckStatus.NO_EXIST); + } + ackResultList.add(ackResult); + } + return CompletableFuture.completedFuture(ackResultList); + }).when(this.messageService).batchChangeInvisibleTime( + any(), anyList(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + + List resultList = this.consumerProcessor.batchChangeInvisibleTime( + createContext(), receiptHandleMessageList, CONSUMER_GROUP, TOPIC, 3000, 3000, true).get(); + + assertEquals(receiptHandleMessageList.size(), resultList.size()); + Map msgResult = new HashMap<>(); + for (BatchChangeInvisibleTimeResult result : resultList) { + msgResult.put(result.getReceiptHandleMessage().getMessageId(), result); + } + for (String msgId : broker1Msg) { + BatchChangeInvisibleTimeResult result = msgResult.get(msgId); + assertEquals(AckStatus.OK, result.getAckResult().getStatus()); + assertEquals(popTime, result.getAckResult().getPopTime()); + assertEquals(newExtraInfo + MessageConst.KEY_SEPARATOR + + result.getReceiptHandleMessage().getReceiptHandle().getCommitLogOffset(), + result.getAckResult().getExtraInfo()); + assertNull(result.getProxyException()); + } + for (String msgId : broker2Msg) { + assertEquals(AckStatus.NO_EXIST, msgResult.get(msgId).getAckResult().getStatus()); + assertNull(msgResult.get(msgId).getProxyException()); + } + assertNotNull(msgResult.get(expireMessage.getMsgId()).getProxyException()); + assertEquals(ProxyExceptionCode.INVALID_RECEIPT_HANDLE, + msgResult.get(expireMessage.getMsgId()).getProxyException().getCode()); + assertNull(msgResult.get(expireMessage.getMsgId()).getAckResult()); + } + + @Test + public void testBatchChangeInvisibleTimeSplitByRealTopic() throws Throwable { + String brokerName = "brokerName1"; + String retryTopic = KeyBuilder.buildPopRetryTopic(TOPIC, CONSUMER_GROUP, + new BrokerConfig().isEnableRetryTopicV2()); + List receiptHandleMessageList = new ArrayList<>(); + long now = System.currentTimeMillis(); + for (int i = 0; i < 2; i++) { + MessageExt normalMessage = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, i + 1, brokerName); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(normalMessage), normalMessage.getMsgId())); + + MessageExt retryMessage = createMessageExt(retryTopic, "", 0, 3000, now, + 0, 0, 0, i + 10, brokerName); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(retryMessage), retryMessage.getMsgId())); + } + + ArgumentCaptor batchHandleListCaptor = ArgumentCaptor.forClass(List.class); + doAnswer((Answer>>) invocation -> { + List handleMessageList = invocation.getArgument(1, List.class); + List ackResultList = new ArrayList<>(); + for (ReceiptHandleMessage ignored : handleMessageList) { + AckResult ackResult = new AckResult(); + ackResult.setStatus(AckStatus.OK); + ackResultList.add(ackResult); + } + return CompletableFuture.completedFuture(ackResultList); + }).when(this.messageService).batchChangeInvisibleTime( + any(), batchHandleListCaptor.capture(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + + List resultList = this.consumerProcessor.batchChangeInvisibleTime( + createContext(), receiptHandleMessageList, CONSUMER_GROUP, TOPIC, 3000, 3000, true).get(); + + assertEquals(receiptHandleMessageList.size(), resultList.size()); + verify(this.messageService, times(2)).batchChangeInvisibleTime( + any(), anyList(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + assertEquals(2, batchHandleListCaptor.getAllValues().size()); + assertEquals(2, batchHandleListCaptor.getAllValues().get(0).size()); + assertEquals(2, batchHandleListCaptor.getAllValues().get(1).size()); + verify(this.messageService, never()).changeInvisibleTime(any(), any(), anyString(), any(), anyLong()); + } + + @Test + public void testBatchChangeInvisibleTimeWithSingleHandleUseSingleChange() throws Throwable { + MessageExt messageExt = createMessageExt(TOPIC, "", 0, 3000); + ReceiptHandle handle = create(messageExt); + List receiptHandleMessageList = Collections.singletonList( + new ReceiptHandleMessage(handle, messageExt.getMsgId())); + + String newExtraInfo = "newExtraInfo"; + AckResult innerAckResult = new AckResult(); + innerAckResult.setStatus(AckStatus.OK); + innerAckResult.setPopTime(12345L); + innerAckResult.setExtraInfo(newExtraInfo); + when(this.messageService.changeInvisibleTime(any(), any(), anyString(), any(), anyLong())) + .thenReturn(CompletableFuture.completedFuture(innerAckResult)); + + List resultList = this.consumerProcessor.batchChangeInvisibleTime( + createContext(), receiptHandleMessageList, CONSUMER_GROUP, TOPIC, 3000, 3000, true).get(); + + assertEquals(1, resultList.size()); + assertSame(receiptHandleMessageList.get(0), resultList.get(0).getReceiptHandleMessage()); + assertEquals(AckStatus.OK, resultList.get(0).getAckResult().getStatus()); + assertEquals(newExtraInfo + MessageConst.KEY_SEPARATOR + handle.getCommitLogOffset(), + resultList.get(0).getAckResult().getExtraInfo()); + assertNull(resultList.get(0).getProxyException()); + verify(this.messageService).changeInvisibleTime(any(), any(), eq(messageExt.getMsgId()), any(), anyLong()); + verify(this.messageService, never()).batchChangeInvisibleTime( + any(), anyList(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + } + + @Test + public void testBatchChangeInvisibleTimeSplitOversizedBrokerGroup() throws Throwable { + String brokerName = "brokerName1"; + assertEquals(1024, ConfigurationManager.getProxyConfig().getBatchChangeInvisibleTimeMaxNum()); + int batchMaxNum = ConfigurationManager.getProxyConfig().getBatchChangeInvisibleTimeMaxNum(); + List receiptHandleMessageList = new ArrayList<>(); + long now = System.currentTimeMillis(); + for (int i = 0; i <= batchMaxNum; i++) { + MessageExt brokerMessage = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, i + 1, brokerName); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(brokerMessage), brokerMessage.getMsgId())); + } + + ArgumentCaptor batchHandleListCaptor = ArgumentCaptor.forClass(List.class); + doAnswer((Answer>>) invocation -> { + List handleMessageList = invocation.getArgument(1, List.class); + List ackResultList = new ArrayList<>(); + for (ReceiptHandleMessage ignored : handleMessageList) { + AckResult ackResult = new AckResult(); + ackResult.setStatus(AckStatus.OK); + ackResultList.add(ackResult); + } + return CompletableFuture.completedFuture(ackResultList); + }).when(this.messageService).batchChangeInvisibleTime( + any(), batchHandleListCaptor.capture(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + + AckResult singleAckResult = new AckResult(); + singleAckResult.setStatus(AckStatus.OK); + when(this.messageService.changeInvisibleTime(any(), any(), anyString(), any(), anyLong())) + .thenReturn(CompletableFuture.completedFuture(singleAckResult)); + + List resultList = this.consumerProcessor.batchChangeInvisibleTime( + createContext(), receiptHandleMessageList, CONSUMER_GROUP, TOPIC, 3000, 3000, true).get(); + + assertEquals(receiptHandleMessageList.size(), resultList.size()); + assertEquals(batchMaxNum, batchHandleListCaptor.getValue().size()); + verify(this.messageService).batchChangeInvisibleTime( + any(), anyList(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + verify(this.messageService).changeInvisibleTime(any(), any(), anyString(), any(), anyLong()); + } + @Test public void testChangeInvisibleTime() throws Throwable { ReceiptHandle handle = create(createMessageExt(MixAll.RETRY_GROUP_TOPIC_PREFIX + TOPIC, "", 0, 3000)); @@ -441,6 +634,107 @@ public void testPopMessageWithToReturnFilter() throws Throwable { assertEquals(0, popResult.getMsgFoundList().size()); } + @Test + public void testPopMessageWithToReturnFilterUseBatchChangeInvisibleTime() throws Throwable { + ConfigurationManager.getProxyConfig().setEnableBatchChangeInvisibleTime(true); + final String tag = "tag"; + final long invisibleTime = Duration.ofSeconds(15).toMillis(); + ArgumentCaptor messageQueueArgumentCaptor = ArgumentCaptor.forClass(AddressableMessageQueue.class); + ArgumentCaptor requestHeaderArgumentCaptor = ArgumentCaptor.forClass(PopMessageRequestHeader.class); + + List messageExtList = new ArrayList<>(); + messageExtList.add(createMessageExt(TOPIC, tag, 0, invisibleTime)); + messageExtList.add(createMessageExt(TOPIC, tag, 0, invisibleTime)); + PopResult innerPopResult = new PopResult(PopStatus.FOUND, messageExtList); + when(this.messageService.popMessage(any(), messageQueueArgumentCaptor.capture(), requestHeaderArgumentCaptor.capture(), anyLong())) + .thenReturn(CompletableFuture.completedFuture(innerPopResult)); + + when(this.topicRouteService.getCurrentMessageQueueView(any(), anyString())) + .thenReturn(mock(MessageQueueView.class)); + + ArgumentCaptor handleMessageListCaptor = ArgumentCaptor.forClass(List.class); + when(this.messagingProcessor.batchChangeInvisibleTime(any(), handleMessageListCaptor.capture(), + anyString(), anyString(), anyLong(), anyLong(), anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(Collections.emptyList())); + + AddressableMessageQueue messageQueue = mock(AddressableMessageQueue.class); + PopResult popResult = this.consumerProcessor.popMessage( + createContext(), + (ctx, messageQueueView) -> messageQueue, + CONSUMER_GROUP, + TOPIC, + 60, + invisibleTime, + Duration.ofSeconds(3).toMillis(), + ConsumeInitMode.MAX, + FilterAPI.build(TOPIC, tag, ExpressionType.TAG), + false, + (ctx, consumerGroup, subscriptionData, messageExt) -> PopMessageResultFilter.FilterResult.TO_RETURN, + null, + Duration.ofSeconds(3).toMillis() + ).get(); + + verify(this.messagingProcessor).batchChangeInvisibleTime(any(), anyList(), + eq(CONSUMER_GROUP), eq(TOPIC), eq(Duration.ofSeconds(1).toMillis()), + eq(MessagingProcessor.DEFAULT_TIMEOUT_MILLS), eq(true)); + verify(this.messagingProcessor, never()).changeInvisibleTime(any(), any(), anyString(), + anyString(), anyString(), anyLong(), any(), anyLong(), anyBoolean()); + assertEquals(2, handleMessageListCaptor.getValue().size()); + assertEquals(messageExtList.get(0).getMsgId(), + ((ReceiptHandleMessage) handleMessageListCaptor.getValue().get(0)).getMessageId()); + assertEquals(messageExtList.get(1).getMsgId(), + ((ReceiptHandleMessage) handleMessageListCaptor.getValue().get(1)).getMessageId()); + assertEquals(PopStatus.FOUND, popResult.getPopStatus()); + assertEquals(0, popResult.getMsgFoundList().size()); + } + + @Test + public void testPopMessageWithSingleToReturnFilterUseSingleChangeInvisibleTime() throws Throwable { + ConfigurationManager.getProxyConfig().setEnableBatchChangeInvisibleTime(true); + final String tag = "tag"; + final long invisibleTime = Duration.ofSeconds(15).toMillis(); + ArgumentCaptor messageQueueArgumentCaptor = ArgumentCaptor.forClass(AddressableMessageQueue.class); + ArgumentCaptor requestHeaderArgumentCaptor = ArgumentCaptor.forClass(PopMessageRequestHeader.class); + + List messageExtList = new ArrayList<>(); + messageExtList.add(createMessageExt(TOPIC, tag, 0, invisibleTime)); + PopResult innerPopResult = new PopResult(PopStatus.FOUND, messageExtList); + when(this.messageService.popMessage(any(), messageQueueArgumentCaptor.capture(), requestHeaderArgumentCaptor.capture(), anyLong())) + .thenReturn(CompletableFuture.completedFuture(innerPopResult)); + + when(this.topicRouteService.getCurrentMessageQueueView(any(), anyString())) + .thenReturn(mock(MessageQueueView.class)); + + when(this.messagingProcessor.changeInvisibleTime(any(), any(), anyString(), + anyString(), anyString(), anyLong(), any(), anyLong(), anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(mock(AckResult.class))); + + AddressableMessageQueue messageQueue = mock(AddressableMessageQueue.class); + PopResult popResult = this.consumerProcessor.popMessage( + createContext(), + (ctx, messageQueueView) -> messageQueue, + CONSUMER_GROUP, + TOPIC, + 60, + invisibleTime, + Duration.ofSeconds(3).toMillis(), + ConsumeInitMode.MAX, + FilterAPI.build(TOPIC, tag, ExpressionType.TAG), + false, + (ctx, consumerGroup, subscriptionData, messageExt) -> PopMessageResultFilter.FilterResult.TO_RETURN, + null, + Duration.ofSeconds(3).toMillis() + ).get(); + + verify(this.messagingProcessor).changeInvisibleTime(any(), any(), eq(messageExtList.get(0).getMsgId()), + eq(CONSUMER_GROUP), eq(TOPIC), eq(Duration.ofSeconds(1).toMillis()), eq(null), + eq(MessagingProcessor.DEFAULT_TIMEOUT_MILLS), eq(true)); + verify(this.messagingProcessor, never()).batchChangeInvisibleTime(any(), anyList(), + anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + assertEquals(PopStatus.FOUND, popResult.getPopStatus()); + assertEquals(0, popResult.getMsgFoundList().size()); + } + @Test public void testChangeInvisibleTimeWithSuspendFalse() throws Throwable { ReceiptHandle handle = create(createMessageExt(MixAll.RETRY_GROUP_TOPIC_PREFIX + TOPIC, "", 0, 3000)); diff --git a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessorTest.java b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessorTest.java index 62e5e64eb42..c771515a70c 100644 --- a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessorTest.java +++ b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessorTest.java @@ -17,8 +17,16 @@ package org.apache.rocketmq.proxy.processor; import io.netty.channel.local.LocalChannel; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; import org.apache.rocketmq.broker.client.ClientChannelInfo; +import org.apache.rocketmq.broker.client.ConsumerGroupEvent; +import org.apache.rocketmq.broker.client.ConsumerIdsChangeListener; import org.apache.rocketmq.broker.client.ConsumerManager; +import org.apache.rocketmq.client.consumer.AckResult; +import org.apache.rocketmq.client.consumer.AckStatus; import org.apache.rocketmq.common.consumer.ReceiptHandle; import org.apache.rocketmq.common.message.MessageClientIDSetter; import org.apache.rocketmq.proxy.common.ContextVariable; @@ -28,14 +36,20 @@ import org.apache.rocketmq.proxy.config.InitConfigTest; import org.apache.rocketmq.proxy.config.ProxyConfig; import org.apache.rocketmq.proxy.service.ServiceManager; +import org.apache.rocketmq.proxy.service.message.ReceiptHandleMessage; import org.apache.rocketmq.proxy.service.metadata.MetadataService; +import org.apache.rocketmq.proxy.service.receipt.DefaultReceiptHandleManager; +import org.apache.rocketmq.remoting.protocol.LanguageCode; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; +import static org.junit.Assert.assertEquals; import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) @@ -98,4 +112,157 @@ public void testStart() throws Exception { Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), Mockito.eq(ConfigurationManager.getProxyConfig().getDefaultInvisibleTimeMills()), Mockito.eq(null)); } + @Test + public void testRenewWithBatchChangeInvisibleTime() throws Exception { + ConfigurationManager.getProxyConfig().setEnableBatchChangeInvisibleTime(true); + Mockito.when(consumerManager.findChannel(Mockito.eq(CONSUMER_GROUP), Mockito.eq(PROXY_CONTEXT.getChannel()))) + .thenReturn(Mockito.mock(ClientChannelInfo.class)); + + String anotherMsgId = MessageClientIDSetter.createUniqID(); + MessageReceiptHandle anotherHandle = new MessageReceiptHandle( + CONSUMER_GROUP, + TOPIC, + QUEUE_ID, + ReceiptHandle.builder() + .startOffset(0L) + .retrieveTime(System.currentTimeMillis() - INVISIBLE_TIME + + ConfigurationManager.getProxyConfig().getRenewAheadTimeMillis() - 5) + .invisibleTime(INVISIBLE_TIME) + .reviveQueueId(1) + .topicType(ReceiptHandle.NORMAL_TOPIC) + .brokerName(BROKER_NAME) + .queueId(QUEUE_ID) + .offset(OFFSET + 1) + .commitLogOffset(0L) + .build().encode(), + anotherMsgId, + OFFSET + 1, + RECONSUME_TIMES); + + ArgumentCaptor handleMessageListCaptor = ArgumentCaptor.forClass(List.class); + Mockito.doAnswer((Answer>>) invocation -> { + List handleMessageList = invocation.getArgument(1, List.class); + List results = new ArrayList<>(handleMessageList.size()); + for (ReceiptHandleMessage handleMessage : handleMessageList) { + AckResult ackResult = new AckResult(); + ackResult.setStatus(AckStatus.OK); + ackResult.setExtraInfo(handleMessage.getReceiptHandle().encode()); + results.add(new BatchChangeInvisibleTimeResult(handleMessage, ackResult)); + } + return CompletableFuture.completedFuture(results); + }).when(messagingProcessor).batchChangeInvisibleTime( + Mockito.any(), handleMessageListCaptor.capture(), Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), + Mockito.eq(ConfigurationManager.getProxyConfig().getDefaultInvisibleTimeMills()), + Mockito.eq(MessagingProcessor.DEFAULT_TIMEOUT_MILLS), Mockito.eq(false)); + + receiptHandleProcessor.addReceiptHandle( + PROXY_CONTEXT, PROXY_CONTEXT.getChannel(), CONSUMER_GROUP, MSG_ID, messageReceiptHandle); + receiptHandleProcessor.addReceiptHandle( + PROXY_CONTEXT, PROXY_CONTEXT.getChannel(), CONSUMER_GROUP, anotherMsgId, anotherHandle); + + Method method = DefaultReceiptHandleManager.class.getDeclaredMethod("scheduleRenewTask"); + method.setAccessible(true); + method.invoke(receiptHandleProcessor.receiptHandleManager); + + Mockito.verify(messagingProcessor, Mockito.timeout(10000).times(1)).batchChangeInvisibleTime( + Mockito.any(), Mockito.anyList(), Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), + Mockito.eq(ConfigurationManager.getProxyConfig().getDefaultInvisibleTimeMills()), + Mockito.eq(MessagingProcessor.DEFAULT_TIMEOUT_MILLS), Mockito.eq(false)); + assertEquals(2, handleMessageListCaptor.getValue().size()); + Mockito.verify(messagingProcessor, Mockito.never()).changeInvisibleTime( + Mockito.any(), Mockito.any(), Mockito.anyString(), Mockito.anyString(), Mockito.anyString(), + Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testClientOfflineClearGroupWithBatchChangeInvisibleTime() throws Exception { + ConfigurationManager.getProxyConfig().setEnableBatchChangeInvisibleTime(true); + ArgumentCaptor listenerArgumentCaptor = + ArgumentCaptor.forClass(ConsumerIdsChangeListener.class); + Mockito.verify(consumerManager).appendConsumerIdsChangeListener(listenerArgumentCaptor.capture()); + + String anotherMsgId = MessageClientIDSetter.createUniqID(); + MessageReceiptHandle anotherHandle = new MessageReceiptHandle( + CONSUMER_GROUP, + TOPIC, + QUEUE_ID, + ReceiptHandle.builder() + .startOffset(0L) + .retrieveTime(System.currentTimeMillis() - INVISIBLE_TIME + + ConfigurationManager.getProxyConfig().getRenewAheadTimeMillis() - 5) + .invisibleTime(INVISIBLE_TIME) + .reviveQueueId(1) + .topicType(ReceiptHandle.NORMAL_TOPIC) + .brokerName(BROKER_NAME) + .queueId(QUEUE_ID) + .offset(OFFSET + 1) + .commitLogOffset(0L) + .build().encode(), + anotherMsgId, + OFFSET + 1, + RECONSUME_TIMES); + + ArgumentCaptor handleMessageListCaptor = ArgumentCaptor.forClass(List.class); + Mockito.doAnswer((Answer>>) invocation -> { + List handleMessageList = invocation.getArgument(1, List.class); + List results = new ArrayList<>(handleMessageList.size()); + for (ReceiptHandleMessage handleMessage : handleMessageList) { + AckResult ackResult = new AckResult(); + ackResult.setStatus(AckStatus.OK); + results.add(new BatchChangeInvisibleTimeResult(handleMessage, ackResult)); + } + return CompletableFuture.completedFuture(results); + }).when(messagingProcessor).batchChangeInvisibleTime( + Mockito.any(), handleMessageListCaptor.capture(), Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), + Mockito.eq(ConfigurationManager.getProxyConfig().getInvisibleTimeMillisWhenClear()), + Mockito.eq(MessagingProcessor.DEFAULT_TIMEOUT_MILLS), Mockito.eq(false)); + + receiptHandleProcessor.addReceiptHandle( + PROXY_CONTEXT, PROXY_CONTEXT.getChannel(), CONSUMER_GROUP, MSG_ID, messageReceiptHandle); + receiptHandleProcessor.addReceiptHandle( + PROXY_CONTEXT, PROXY_CONTEXT.getChannel(), CONSUMER_GROUP, anotherMsgId, anotherHandle); + + listenerArgumentCaptor.getValue().handle(ConsumerGroupEvent.CLIENT_UNREGISTER, CONSUMER_GROUP, + new ClientChannelInfo(PROXY_CONTEXT.getChannel(), "clientId", LanguageCode.JAVA, 0)); + + Mockito.verify(messagingProcessor, Mockito.timeout(10000).times(1)).batchChangeInvisibleTime( + Mockito.any(), Mockito.anyList(), Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), + Mockito.eq(ConfigurationManager.getProxyConfig().getInvisibleTimeMillisWhenClear()), + Mockito.eq(MessagingProcessor.DEFAULT_TIMEOUT_MILLS), Mockito.eq(false)); + assertEquals(2, handleMessageListCaptor.getValue().size()); + Mockito.verify(messagingProcessor, Mockito.never()).changeInvisibleTime( + Mockito.any(), Mockito.any(), Mockito.anyString(), Mockito.anyString(), Mockito.anyString(), + Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testClientOfflineClearGroupWithSingleHandleUseSingleChangeInvisibleTime() throws Exception { + ConfigurationManager.getProxyConfig().setEnableBatchChangeInvisibleTime(true); + ArgumentCaptor listenerArgumentCaptor = + ArgumentCaptor.forClass(ConsumerIdsChangeListener.class); + Mockito.verify(consumerManager).appendConsumerIdsChangeListener(listenerArgumentCaptor.capture()); + + AckResult ackResult = new AckResult(); + ackResult.setStatus(AckStatus.OK); + Mockito.when(messagingProcessor.changeInvisibleTime( + Mockito.any(ProxyContext.class), Mockito.any(ReceiptHandle.class), Mockito.eq(MESSAGE_ID), + Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), + Mockito.eq(ConfigurationManager.getProxyConfig().getInvisibleTimeMillisWhenClear()), Mockito.eq(null))) + .thenReturn(CompletableFuture.completedFuture(ackResult)); + + receiptHandleProcessor.addReceiptHandle( + PROXY_CONTEXT, PROXY_CONTEXT.getChannel(), CONSUMER_GROUP, MSG_ID, messageReceiptHandle); + + listenerArgumentCaptor.getValue().handle(ConsumerGroupEvent.CLIENT_UNREGISTER, CONSUMER_GROUP, + new ClientChannelInfo(PROXY_CONTEXT.getChannel(), "clientId", LanguageCode.JAVA, 0)); + + Mockito.verify(messagingProcessor, Mockito.timeout(10000).times(1)).changeInvisibleTime( + Mockito.any(ProxyContext.class), Mockito.any(ReceiptHandle.class), Mockito.eq(MESSAGE_ID), + Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), + Mockito.eq(ConfigurationManager.getProxyConfig().getInvisibleTimeMillisWhenClear()), Mockito.eq(null)); + Mockito.verify(messagingProcessor, Mockito.never()).batchChangeInvisibleTime( + Mockito.any(), Mockito.anyList(), Mockito.anyString(), Mockito.anyString(), Mockito.anyLong(), + Mockito.anyLong(), Mockito.anyBoolean()); + } + } diff --git a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RequestCode.java b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RequestCode.java index b32dbbc87ea..d9a8a406a4f 100644 --- a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RequestCode.java +++ b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RequestCode.java @@ -82,6 +82,7 @@ public class RequestCode { public static final int BATCH_ACK_MESSAGE = 200151; public static final int PEEK_MESSAGE = 200052; public static final int CHANGE_MESSAGE_INVISIBLETIME = 200053; + public static final int BATCH_CHANGE_MESSAGE_INVISIBLETIME = 200153; public static final int NOTIFICATION = 200054; public static final int POLLING_INFO = 200055; public static final int POP_ROLLBACK = 200056; diff --git a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeRequestBody.java b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeRequestBody.java new file mode 100644 index 00000000000..f5b7695379f --- /dev/null +++ b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeRequestBody.java @@ -0,0 +1,33 @@ +/* + * 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.rocketmq.remoting.protocol.body; + +import java.util.List; +import org.apache.rocketmq.remoting.protocol.RemotingSerializable; + +public class BatchChangeInvisibleTimeRequestBody extends RemotingSerializable { + private List entries; + + public List getEntries() { + return entries; + } + + public void setEntries(List entries) { + this.entries = entries; + } +} diff --git a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeResponseBody.java b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeResponseBody.java new file mode 100644 index 00000000000..18733813ee9 --- /dev/null +++ b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeResponseBody.java @@ -0,0 +1,33 @@ +/* + * 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.rocketmq.remoting.protocol.body; + +import java.util.List; +import org.apache.rocketmq.remoting.protocol.RemotingSerializable; + +public class BatchChangeInvisibleTimeResponseBody extends RemotingSerializable { + private List entries; + + public List getEntries() { + return entries; + } + + public void setEntries(List entries) { + this.entries = entries; + } +} diff --git a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeRequestEntry.java b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeRequestEntry.java new file mode 100644 index 00000000000..d87aba16297 --- /dev/null +++ b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeRequestEntry.java @@ -0,0 +1,93 @@ +/* + * 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.rocketmq.remoting.protocol.body; + +public class ChangeInvisibleTimeRequestEntry { + private String consumerGroup; + private String topic; + private int queueId; + private String extraInfo; + private long offset; + private long invisibleTime; + private String liteTopic; + private boolean suspend; + + public String getConsumerGroup() { + return consumerGroup; + } + + public void setConsumerGroup(String consumerGroup) { + this.consumerGroup = consumerGroup; + } + + public String getTopic() { + return topic; + } + + public void setTopic(String topic) { + this.topic = topic; + } + + public int getQueueId() { + return queueId; + } + + public void setQueueId(int queueId) { + this.queueId = queueId; + } + + public String getExtraInfo() { + return extraInfo; + } + + public void setExtraInfo(String extraInfo) { + this.extraInfo = extraInfo; + } + + public long getOffset() { + return offset; + } + + public void setOffset(long offset) { + this.offset = offset; + } + + public long getInvisibleTime() { + return invisibleTime; + } + + public void setInvisibleTime(long invisibleTime) { + this.invisibleTime = invisibleTime; + } + + public String getLiteTopic() { + return liteTopic; + } + + public void setLiteTopic(String liteTopic) { + this.liteTopic = liteTopic; + } + + public boolean isSuspend() { + return suspend; + } + + public void setSuspend(boolean suspend) { + this.suspend = suspend; + } +} diff --git a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeResponseEntry.java b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeResponseEntry.java new file mode 100644 index 00000000000..f01013b2d70 --- /dev/null +++ b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeResponseEntry.java @@ -0,0 +1,57 @@ +/* + * 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.rocketmq.remoting.protocol.body; + +public class ChangeInvisibleTimeResponseEntry { + private int code; + private long popTime; + private long invisibleTime; + private int reviveQid; + + public int getCode() { + return code; + } + + public void setCode(int code) { + this.code = code; + } + + public long getPopTime() { + return popTime; + } + + public void setPopTime(long popTime) { + this.popTime = popTime; + } + + public long getInvisibleTime() { + return invisibleTime; + } + + public void setInvisibleTime(long invisibleTime) { + this.invisibleTime = invisibleTime; + } + + public int getReviveQid() { + return reviveQid; + } + + public void setReviveQid(int reviveQid) { + this.reviveQid = reviveQid; + } +} diff --git a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/BatchChangeInvisibleTimeRequestHeader.java b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/BatchChangeInvisibleTimeRequestHeader.java new file mode 100644 index 00000000000..ad76b3a23eb --- /dev/null +++ b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/BatchChangeInvisibleTimeRequestHeader.java @@ -0,0 +1,68 @@ +/* + * 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.rocketmq.remoting.protocol.header; + +import com.google.common.base.MoreObjects; +import org.apache.rocketmq.common.action.Action; +import org.apache.rocketmq.common.action.RocketMQAction; +import org.apache.rocketmq.common.resource.ResourceType; +import org.apache.rocketmq.common.resource.RocketMQResource; +import org.apache.rocketmq.remoting.annotation.CFNotNull; +import org.apache.rocketmq.remoting.exception.RemotingCommandException; +import org.apache.rocketmq.remoting.protocol.RequestCode; +import org.apache.rocketmq.remoting.rpc.TopicRequestHeader; + +@RocketMQAction(value = RequestCode.BATCH_CHANGE_MESSAGE_INVISIBLETIME, action = Action.SUB) +public class BatchChangeInvisibleTimeRequestHeader extends TopicRequestHeader { + @CFNotNull + @RocketMQResource(ResourceType.GROUP) + private String consumerGroup; + @CFNotNull + @RocketMQResource(ResourceType.TOPIC) + private String topic; + + @Override + public void checkFields() throws RemotingCommandException { + } + + public String getConsumerGroup() { + return consumerGroup; + } + + public void setConsumerGroup(String consumerGroup) { + this.consumerGroup = consumerGroup; + } + + @Override + public String getTopic() { + return topic; + } + + @Override + public void setTopic(String topic) { + this.topic = topic; + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("consumerGroup", consumerGroup) + .add("topic", topic) + .omitNullValues() + .toString(); + } +} diff --git a/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeTest.java b/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeTest.java new file mode 100644 index 00000000000..89540be62c9 --- /dev/null +++ b/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeTest.java @@ -0,0 +1,94 @@ +/* + * 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.rocketmq.remoting.protocol.body; + +import java.util.Arrays; +import java.util.Collections; +import org.apache.rocketmq.remoting.protocol.ResponseCode; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +public class BatchChangeInvisibleTimeTest { + + @Test + public void testBatchChangeInvisibleTimeRequestBody() { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + requestBody.setEntries(Arrays.asList(buildRequestEntry(1), buildRequestEntry(2))); + + BatchChangeInvisibleTimeRequestBody decoded = + BatchChangeInvisibleTimeRequestBody.decode(requestBody.encode(), BatchChangeInvisibleTimeRequestBody.class); + + assertThat(decoded.getEntries()).hasSize(2); + assertThat(decoded.getEntries().get(0).getConsumerGroup()).isEqualTo("group"); + assertThat(decoded.getEntries().get(0).getTopic()).isEqualTo("topic"); + assertThat(decoded.getEntries().get(0).getQueueId()).isEqualTo(1); + assertThat(decoded.getEntries().get(0).getExtraInfo()).isEqualTo("0 100 1000 0 broker 1 10"); + assertThat(decoded.getEntries().get(0).getOffset()).isEqualTo(10); + assertThat(decoded.getEntries().get(0).getInvisibleTime()).isEqualTo(3000); + assertThat(decoded.getEntries().get(0).getLiteTopic()).isEqualTo("lite"); + assertThat(decoded.getEntries().get(0).isSuspend()).isTrue(); + } + + @Test + public void testEmptyBatchChangeInvisibleTimeRequestBody() { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + requestBody.setEntries(Collections.emptyList()); + + BatchChangeInvisibleTimeRequestBody decoded = + BatchChangeInvisibleTimeRequestBody.decode(requestBody.encode(), BatchChangeInvisibleTimeRequestBody.class); + + assertThat(decoded.getEntries()).isEmpty(); + } + + @Test + public void testBatchChangeInvisibleTimeResponseBody() { + BatchChangeInvisibleTimeResponseBody responseBody = new BatchChangeInvisibleTimeResponseBody(); + ChangeInvisibleTimeResponseEntry successEntry = new ChangeInvisibleTimeResponseEntry(); + successEntry.setCode(ResponseCode.SUCCESS); + successEntry.setPopTime(200); + successEntry.setInvisibleTime(3000); + successEntry.setReviveQid(1); + ChangeInvisibleTimeResponseEntry failedEntry = new ChangeInvisibleTimeResponseEntry(); + failedEntry.setCode(ResponseCode.NO_MESSAGE); + responseBody.setEntries(Arrays.asList(successEntry, failedEntry)); + + BatchChangeInvisibleTimeResponseBody decoded = + BatchChangeInvisibleTimeResponseBody.decode(responseBody.encode(), BatchChangeInvisibleTimeResponseBody.class); + + assertThat(decoded.getEntries()).hasSize(2); + assertThat(decoded.getEntries().get(0).getCode()).isEqualTo(ResponseCode.SUCCESS); + assertThat(decoded.getEntries().get(0).getPopTime()).isEqualTo(200); + assertThat(decoded.getEntries().get(0).getInvisibleTime()).isEqualTo(3000); + assertThat(decoded.getEntries().get(0).getReviveQid()).isEqualTo(1); + assertThat(decoded.getEntries().get(1).getCode()).isEqualTo(ResponseCode.NO_MESSAGE); + } + + private ChangeInvisibleTimeRequestEntry buildRequestEntry(int queueId) { + ChangeInvisibleTimeRequestEntry entry = new ChangeInvisibleTimeRequestEntry(); + entry.setConsumerGroup("group"); + entry.setTopic("topic"); + entry.setQueueId(queueId); + entry.setExtraInfo("0 100 1000 0 broker " + queueId + " 10"); + entry.setOffset(10); + entry.setInvisibleTime(3000); + entry.setLiteTopic("lite"); + entry.setSuspend(true); + return entry; + } +} diff --git a/test/src/main/java/org/apache/rocketmq/test/client/rmq/RMQPopClient.java b/test/src/main/java/org/apache/rocketmq/test/client/rmq/RMQPopClient.java index c45a26c59d0..395a4581b34 100644 --- a/test/src/main/java/org/apache/rocketmq/test/client/rmq/RMQPopClient.java +++ b/test/src/main/java/org/apache/rocketmq/test/client/rmq/RMQPopClient.java @@ -17,6 +17,7 @@ package org.apache.rocketmq.test.client.rmq; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.CompletableFuture; import org.apache.rocketmq.client.ClientConfig; @@ -28,6 +29,8 @@ import org.apache.rocketmq.client.impl.mqclient.MQClientAPIExt; import org.apache.rocketmq.common.message.MessageQueue; import org.apache.rocketmq.remoting.netty.NettyClientConfig; +import org.apache.rocketmq.remoting.protocol.body.BatchChangeInvisibleTimeRequestBody; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; import org.apache.rocketmq.remoting.protocol.header.AckMessageRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ChangeInvisibleTimeRequestHeader; import org.apache.rocketmq.remoting.protocol.header.ExtraInfoUtil; @@ -162,6 +165,33 @@ public void onException(Throwable e) { return future; } + public CompletableFuture> batchChangeInvisibleTimeAsync(String brokerAddr, String topic, + String consumerGroup, List extraInfoList, long invisibleTime) { + CompletableFuture> future = new CompletableFuture<>(); + try { + BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); + List entries = new ArrayList<>(extraInfoList.size()); + for (String extraInfo : extraInfoList) { + String[] extraInfoStrs = ExtraInfoUtil.split(extraInfo); + ChangeInvisibleTimeRequestEntry entry = new ChangeInvisibleTimeRequestEntry(); + entry.setConsumerGroup(consumerGroup); + entry.setTopic(ExtraInfoUtil.getRealTopic(extraInfoStrs, topic, consumerGroup)); + entry.setQueueId(ExtraInfoUtil.getQueueId(extraInfoStrs)); + entry.setOffset(ExtraInfoUtil.getQueueOffset(extraInfoStrs)); + entry.setExtraInfo(extraInfo); + entry.setInvisibleTime(invisibleTime); + entries.add(entry); + } + requestBody.setEntries(entries); + String requestTopic = entries.isEmpty() ? topic : entries.get(0).getTopic(); + return this.mqClientAPI.batchChangeInvisibleTimeAsync( + brokerAddr, requestTopic, consumerGroup, requestBody, DEFAULT_TIMEOUT); + } catch (Throwable t) { + future.completeExceptionally(t); + } + return future; + } + public CompletableFuture changeInvisibleTimeAsync(String brokerAddr, String brokerName, String topic, String consumerGroup, String extraInfo, long invisibleTime) { String[] extraInfoStrs = ExtraInfoUtil.split(extraInfo); diff --git a/test/src/test/java/org/apache/rocketmq/test/client/consumer/pop/BatchChangeInvisibleTimeIT.java b/test/src/test/java/org/apache/rocketmq/test/client/consumer/pop/BatchChangeInvisibleTimeIT.java new file mode 100644 index 00000000000..771ce162460 --- /dev/null +++ b/test/src/test/java/org/apache/rocketmq/test/client/consumer/pop/BatchChangeInvisibleTimeIT.java @@ -0,0 +1,171 @@ +/* + * 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.rocketmq.test.client.consumer.pop; + +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import java.util.function.Supplier; +import org.apache.rocketmq.client.consumer.AckResult; +import org.apache.rocketmq.client.consumer.AckStatus; +import org.apache.rocketmq.client.consumer.PopResult; +import org.apache.rocketmq.client.consumer.PopStatus; +import org.apache.rocketmq.common.attribute.CQType; +import org.apache.rocketmq.common.attribute.TopicMessageType; +import org.apache.rocketmq.common.constant.ConsumeInitMode; +import org.apache.rocketmq.common.filter.ExpressionType; +import org.apache.rocketmq.common.message.MessageConst; +import org.apache.rocketmq.common.message.MessageExt; +import org.apache.rocketmq.common.message.MessageQueue; +import org.apache.rocketmq.remoting.protocol.header.ExtraInfoUtil; +import org.apache.rocketmq.test.base.IntegrationTestBase; +import org.apache.rocketmq.test.client.rmq.RMQNormalProducer; +import org.apache.rocketmq.test.client.rmq.RMQPopClient; +import org.apache.rocketmq.test.util.MQRandomUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import static org.awaitility.Awaitility.await; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +public class BatchChangeInvisibleTimeIT extends BasePop { + + protected static final long ORIGINAL_INVISIBLE_TIME = 3000L; + protected static final long CHANGED_INVISIBLE_TIME = 10000L; + + protected String topic; + protected String group; + protected RMQNormalProducer producer = null; + protected RMQPopClient client = null; + protected String brokerAddr; + protected MessageQueue messageQueue; + + @Before + public void setUp() { + brokerAddr = brokerController1.getBrokerAddr(); + topic = MQRandomUtils.getRandomTopic(); + group = initConsumerGroup(); + IntegrationTestBase.initTopic(topic, NAMESRV_ADDR, BROKER1_NAME, 8, CQType.SimpleCQ, TopicMessageType.NORMAL); + producer = getProducer(NAMESRV_ADDR, topic); + client = getRMQPopClient(); + messageQueue = new MessageQueue(topic, BROKER1_NAME, -1); + } + + @After + public void tearDown() { + shutdown(); + } + + @Test + public void testBatchChangeInvisibleTimeNormallyWithPopBuffer() throws Throwable { + brokerController1.getBrokerConfig().setEnablePopBufferMerge(true); + brokerController2.getBrokerConfig().setEnablePopBufferMerge(true); + brokerController1.getBrokerConfig().setPopConsumerKVServiceEnable(true); + brokerController2.getBrokerConfig().setPopConsumerKVServiceEnable(true); + + testBatchChangeInvisibleTime(() -> { + try { + return popMessageAsync().get(); + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + } + + @Test + public void testBatchChangeInvisibleTimeNormallyWithoutPopBuffer() throws Throwable { + brokerController1.getBrokerConfig().setEnablePopBufferMerge(false); + brokerController2.getBrokerConfig().setEnablePopBufferMerge(false); + brokerController1.getBrokerConfig().setPopConsumerKVServiceEnable(true); + brokerController2.getBrokerConfig().setPopConsumerKVServiceEnable(true); + + testBatchChangeInvisibleTime(() -> { + try { + return popMessageAsync().get(); + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + } + + @Test + public void testBatchChangeInvisibleTimeOrderly() throws Throwable { + brokerController1.getBrokerConfig().setPopConsumerKVServiceEnable(true); + brokerController2.getBrokerConfig().setPopConsumerKVServiceEnable(true); + + testBatchChangeInvisibleTime(() -> { + try { + return popMessageOrderlyAsync().get(); + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + } + + public void testBatchChangeInvisibleTime(Supplier popResultSupplier) throws Throwable { + producer.send(10); + List extraInfoList = new ArrayList<>(); + await().atMost(Duration.ofSeconds(3)).untilAsserted(() -> { + PopResult popResult = popResultSupplier.get(); + if (popResult.getPopStatus().equals(PopStatus.FOUND)) { + for (MessageExt messageExt : popResult.getMsgFoundList()) { + extraInfoList.add(messageExt.getProperty(MessageConst.PROPERTY_POP_CK)); + } + } + assertEquals(10, extraInfoList.size()); + }); + + List ackResultList = client.batchChangeInvisibleTimeAsync( + brokerAddr, topic, group, extraInfoList, CHANGED_INVISIBLE_TIME).get(); + assertEquals(extraInfoList.size(), ackResultList.size()); + + List changedExtraInfoList = new ArrayList<>(ackResultList.size()); + for (AckResult ackResult : ackResultList) { + assertEquals(AckStatus.OK, ackResult.getStatus()); + assertNotNull(ackResult.getExtraInfo()); + long invisibleTime = ExtraInfoUtil.getInvisibleTime(ExtraInfoUtil.split(ackResult.getExtraInfo())); + assertTrue(invisibleTime >= CHANGED_INVISIBLE_TIME); + assertTrue(invisibleTime <= CHANGED_INVISIBLE_TIME + 3000L); + changedExtraInfoList.add(ackResult.getExtraInfo()); + } + + TimeUnit.MILLISECONDS.sleep(ORIGINAL_INVISIBLE_TIME + 3000L); + PopResult popResult = popResultSupplier.get(); + assertEquals(PopStatus.POLLING_NOT_FOUND, popResult.getPopStatus()); + + AckResult ackResult = client.batchAckMessageAsync(brokerAddr, topic, group, changedExtraInfoList).get(); + assertEquals(AckStatus.OK, ackResult.getStatus()); + } + + private CompletableFuture popMessageAsync() { + return client.popMessageAsync( + brokerAddr, messageQueue, ORIGINAL_INVISIBLE_TIME, 10, group, 3000, false, + ConsumeInitMode.MIN, false, ExpressionType.TAG, "*"); + } + + private CompletableFuture popMessageOrderlyAsync() { + return client.popMessageAsync( + brokerAddr, messageQueue, ORIGINAL_INVISIBLE_TIME, 10, group, 3000, false, + ConsumeInitMode.MIN, true, ExpressionType.TAG, "*", null); + } +} From 5df26c48faef6136c725be0ae7f26b2a35b85153 Mon Sep 17 00:00:00 2001 From: qianye Date: Wed, 17 Jun 2026 14:03:33 +0800 Subject: [PATCH 02/10] Fix single change invisible time KV write batch --- .../broker/pop/PopConsumerService.java | 17 +++------ .../broker/pop/PopConsumerServiceTest.java | 35 +++++++++++++++++++ 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java index 36c3033f61e..c1d49b31f92 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java @@ -517,21 +517,14 @@ public void changeInvisibilityDuration(long popTime, long invisibleTime, long ch if (skipWrite) { log.info("PopConsumerService change invisibility skip, time={}, " + "groupId={}, topicId={}, queueId={}, offset={}", popTime, groupId, topicId, queueId, offset); - } else { - this.popConsumerStore.writeRecords(Collections.singletonList(ckRecord)); } + List ckRecords = skipWrite ? Collections.emptyList() : Collections.singletonList(ckRecord); + List ackRecords = Collections.singletonList(ackRecord); if (brokerConfig.isEnablePopBufferMerge() && popConsumerCache != null) { - if (popConsumerCache.deleteRecords(Collections.singletonList(ackRecord)).isEmpty()) { - return; - } - } - - // If the new CK has the same key as the old CK (same visibilityTimeout), - // the write already overwrites the old record in RocksDB, skip delete - // to avoid removing the newly written record. - if (skipWrite || ckRecord.getVisibilityTimeout() != ackRecord.getVisibilityTimeout()) { - this.popConsumerStore.deleteRecords(Collections.singletonList(ackRecord)); + popConsumerCache.writeAndDeleteRecords(ckRecords, ackRecords); + } else { + this.popConsumerStore.writeAndDeleteRecords(ckRecords, ackRecords); } } diff --git a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java index a847bbdd594..fccd16cb577 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java @@ -538,6 +538,41 @@ public void testChangeInvisibilityDurationWithSuspendFalse() { consumerService.shutdown(); } + @Test + public void testChangeInvisibilityDurationUseSingleWriteAndDelete() throws IllegalAccessException { + long current = System.currentTimeMillis(); + long popTime = current - 1000; + long invisibleTime = 10000; + long changedPopTime = current; + long changedInvisibleTime = 20000; + long offset = 300L; + + brokerController.getBrokerConfig().setEnablePopBufferMerge(false); + PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerKVStore.class); + FieldUtils.writeField(consumerService, "popConsumerStore", consumerKVStore, true); + Mockito.when(brokerController.getSubscriptionGroupManager().containsSubscriptionGroup(groupId)).thenReturn(true); + + consumerService.changeInvisibilityDuration(popTime, invisibleTime, changedPopTime, + changedInvisibleTime, groupId, topicId, queueId, offset, true); + + ArgumentCaptor writeCaptor = ArgumentCaptor.forClass(List.class); + ArgumentCaptor deleteCaptor = ArgumentCaptor.forClass(List.class); + Mockito.verify(consumerKVStore).writeAndDeleteRecords(writeCaptor.capture(), deleteCaptor.capture()); + Mockito.verify(consumerKVStore, Mockito.never()).writeRecords(any()); + Mockito.verify(consumerKVStore, Mockito.never()).deleteRecords(any()); + + Assert.assertEquals(1, writeCaptor.getValue().size()); + Assert.assertEquals(1, deleteCaptor.getValue().size()); + PopConsumerRecord ckRecord = (PopConsumerRecord) writeCaptor.getValue().get(0); + PopConsumerRecord ackRecord = (PopConsumerRecord) deleteCaptor.getValue().get(0); + Assert.assertEquals(changedPopTime, ckRecord.getPopTime()); + Assert.assertEquals(changedInvisibleTime, ckRecord.getInvisibleTime()); + Assert.assertEquals(popTime, ackRecord.getPopTime()); + Assert.assertEquals(invisibleTime, ackRecord.getInvisibleTime()); + Assert.assertTrue(ckRecord.isSuspend()); + Assert.assertTrue(ackRecord.isSuspend()); + } + @Test public void testBatchChangeInvisibilityDurationDeletesSameVisibilityOldRecordFromCache() throws IllegalAccessException { From 76cc6b4b6b7725e14eb34c38f441607d4ec25d7b Mon Sep 17 00:00:00 2001 From: qianye Date: Wed, 17 Jun 2026 14:34:00 +0800 Subject: [PATCH 03/10] Serialize oversized batch change invisible time chunks --- .../proxy/processor/ConsumerProcessor.java | 20 +++---- .../processor/ConsumerProcessorTest.java | 52 +++++++++++++++++++ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java index ad11221959c..ce38f0f340d 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java @@ -496,19 +496,19 @@ protected CompletableFuture> processBrokerC ProxyConfig proxyConfig = ConfigurationManager.getProxyConfig(); int batchMaxNum = Math.max(1, proxyConfig.getBatchChangeInvisibleTimeMaxNum()); if (handleMessageList.size() > batchMaxNum) { - List>> futures = new ArrayList<>(); + CompletableFuture> resultFuture = + CompletableFuture.completedFuture(new ArrayList(handleMessageList.size())); for (int fromIndex = 0; fromIndex < handleMessageList.size(); fromIndex += batchMaxNum) { int toIndex = Math.min(handleMessageList.size(), fromIndex + batchMaxNum); - futures.add(processBrokerChangeInvisibleTime(ctx, consumerGroup, topic, - handleMessageList.subList(fromIndex, toIndex), invisibleTime, timeoutMillis, suspend)); + List batchHandleList = handleMessageList.subList(fromIndex, toIndex); + // Keep oversized chunks sequential to avoid sending many write-heavy batches to the same broker at once. + resultFuture = resultFuture.thenCompose(results -> processBrokerChangeInvisibleTime(ctx, consumerGroup, + topic, batchHandleList, invisibleTime, timeoutMillis, suspend).thenApply(batchResults -> { + results.addAll(batchResults); + return results; + })); } - return CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).thenApply(ignored -> { - List results = new ArrayList<>(handleMessageList.size()); - for (CompletableFuture> resultFuture : futures) { - results.addAll(resultFuture.join()); - } - return results; - }); + return resultFuture; } CompletableFuture> future = new CompletableFuture<>(); this.serviceManager.getMessageService().batchChangeInvisibleTime( diff --git a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java index 755a7ed3d18..219725fbf4d 100644 --- a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java +++ b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java @@ -474,6 +474,48 @@ public void testBatchChangeInvisibleTimeSplitOversizedBrokerGroup() throws Throw verify(this.messageService).changeInvisibleTime(any(), any(), anyString(), any(), anyLong()); } + @Test + public void testBatchChangeInvisibleTimeSplitOversizedBrokerGroupSequentially() throws Throwable { + String brokerName = "brokerName1"; + int batchMaxNum = 2; + ConfigurationManager.getProxyConfig().setBatchChangeInvisibleTimeMaxNum(batchMaxNum); + List receiptHandleMessageList = new ArrayList<>(); + long now = System.currentTimeMillis(); + for (int i = 0; i < batchMaxNum * 2; i++) { + MessageExt brokerMessage = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, i + 1, brokerName); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(brokerMessage), brokerMessage.getMsgId())); + } + + List>> batchFutures = new ArrayList<>(); + ArgumentCaptor batchHandleListCaptor = ArgumentCaptor.forClass(List.class); + doAnswer((Answer>>) invocation -> { + CompletableFuture> batchFuture = new CompletableFuture<>(); + batchFutures.add(batchFuture); + return batchFuture; + }).when(this.messageService).batchChangeInvisibleTime( + any(), batchHandleListCaptor.capture(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + + CompletableFuture> resultFuture = this.consumerProcessor.batchChangeInvisibleTime( + createContext(), receiptHandleMessageList, CONSUMER_GROUP, TOPIC, 3000, 3000, true); + + assertEquals(1, batchFutures.size()); + assertEquals(batchMaxNum, batchHandleListCaptor.getAllValues().get(0).size()); + assertFalse(resultFuture.isDone()); + + batchFutures.get(0).complete(buildAckResultList(batchMaxNum)); + assertEquals(2, batchFutures.size()); + assertEquals(batchMaxNum, batchHandleListCaptor.getAllValues().get(1).size()); + assertFalse(resultFuture.isDone()); + + batchFutures.get(1).complete(buildAckResultList(batchMaxNum)); + List resultList = resultFuture.get(); + assertEquals(receiptHandleMessageList.size(), resultList.size()); + verify(this.messageService, times(2)).batchChangeInvisibleTime( + any(), anyList(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + verify(this.messageService, never()).changeInvisibleTime(any(), any(), anyString(), any(), anyLong()); + } + @Test public void testChangeInvisibleTime() throws Throwable { ReceiptHandle handle = create(createMessageExt(MixAll.RETRY_GROUP_TOPIC_PREFIX + TOPIC, "", 0, 3000)); @@ -778,4 +820,14 @@ public void testChangeInvisibleTimeWithSuspendTrue() throws Throwable { assertEquals(handle.getReceiptHandle(), requestHeaderArgumentCaptor.getValue().getExtraInfo()); assertTrue("Suspend should be true", requestHeaderArgumentCaptor.getValue().isSuspend()); } + + private List buildAckResultList(int size) { + List ackResultList = new ArrayList<>(); + for (int i = 0; i < size; i++) { + AckResult ackResult = new AckResult(); + ackResult.setStatus(AckStatus.OK); + ackResultList.add(ackResult); + } + return ackResultList; + } } From 6aa7d6f394cf8f29d2d01e2debb33f867640e7bc Mon Sep 17 00:00:00 2001 From: qianye Date: Wed, 17 Jun 2026 16:13:24 +0800 Subject: [PATCH 04/10] Refine batch change invisible time entries --- .../broker/pop/PopConsumerService.java | 80 +++---------------- .../ChangeInvisibleTimeProcessor.java | 23 +++--- .../broker/pop/PopConsumerServiceTest.java | 14 +++- .../ChangeInvisibleTimeProcessorTest.java | 9 +-- .../body/ChangeInvisibleTimeRequestEntry.java | 48 +++++++++++ .../body/BatchChangeInvisibleTimeTest.java | 24 ++++++ 6 files changed, 109 insertions(+), 89 deletions(-) diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java index c1d49b31f92..1da0a358182 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java @@ -56,6 +56,7 @@ import org.apache.rocketmq.common.message.MessageExt; import org.apache.rocketmq.common.message.MessageExtBrokerInner; import org.apache.rocketmq.common.utils.ConcurrentHashMapUtils; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; import org.apache.rocketmq.remoting.protocol.header.ExtraInfoUtil; import org.apache.rocketmq.remoting.protocol.subscription.SubscriptionGroupConfig; import org.apache.rocketmq.store.AppendMessageStatus; @@ -528,7 +529,7 @@ public void changeInvisibilityDuration(long popTime, long invisibleTime, long ch } } - public void batchChangeInvisibilityDuration(List changeRecords) { + public void batchChangeInvisibilityDuration(List changeRecords) { if (changeRecords == null || changeRecords.isEmpty()) { return; } @@ -537,32 +538,32 @@ public void batchChangeInvisibilityDuration(List chang List ackRecords = new ArrayList<>(changeRecords.size()); List storeAckRecords = new ArrayList<>(changeRecords.size()); - for (ChangeInvisibilityRecord changeRecord : changeRecords) { + for (ChangeInvisibleTimeRequestEntry changeRecord : changeRecords) { if (brokerConfig.isPopConsumerKVServiceLog()) { log.info("PopConsumerService batch change, time={}, invisible={}, " + "groupId={}, topic={}, queueId={}, offset={}, new time={}, new invisible={}", - changeRecord.getPopTime(), changeRecord.getInvisibleTime(), changeRecord.getGroupId(), - changeRecord.getTopicId(), changeRecord.getQueueId(), changeRecord.getOffset(), + changeRecord.getPopTime(), changeRecord.getOldInvisibleTime(), changeRecord.getConsumerGroup(), + changeRecord.getTopic(), changeRecord.getQueueId(), changeRecord.getOffset(), changeRecord.getChangedPopTime(), changeRecord.getChangedInvisibleTime()); } PopConsumerRecord ckRecord = new PopConsumerRecord( - changeRecord.getChangedPopTime(), changeRecord.getGroupId(), changeRecord.getTopicId(), + changeRecord.getChangedPopTime(), changeRecord.getConsumerGroup(), changeRecord.getTopic(), changeRecord.getQueueId(), 0, changeRecord.getChangedInvisibleTime(), changeRecord.getOffset(), null, changeRecord.isSuspend()); PopConsumerRecord ackRecord = new PopConsumerRecord( - changeRecord.getPopTime(), changeRecord.getGroupId(), changeRecord.getTopicId(), - changeRecord.getQueueId(), 0, changeRecord.getInvisibleTime(), changeRecord.getOffset(), + changeRecord.getPopTime(), changeRecord.getConsumerGroup(), changeRecord.getTopic(), + changeRecord.getQueueId(), 0, changeRecord.getOldInvisibleTime(), changeRecord.getOffset(), null, changeRecord.isSuspend()); boolean skipWrite = brokerConfig.isPopReviveSkipIfGroupAbsent() && - !brokerController.getSubscriptionGroupManager().containsSubscriptionGroup(changeRecord.getGroupId()); + !brokerController.getSubscriptionGroupManager().containsSubscriptionGroup(changeRecord.getConsumerGroup()); if (skipWrite) { log.info("PopConsumerService batch change invisibility skip, time={}, " + "groupId={}, topicId={}, queueId={}, offset={}", changeRecord.getPopTime(), - changeRecord.getGroupId(), changeRecord.getTopicId(), changeRecord.getQueueId(), + changeRecord.getConsumerGroup(), changeRecord.getTopic(), changeRecord.getQueueId(), changeRecord.getOffset()); } else { ckRecords.add(ckRecord); @@ -581,67 +582,6 @@ public void batchChangeInvisibilityDuration(List chang } } - public static class ChangeInvisibilityRecord { - private final long popTime; - private final long invisibleTime; - private final long changedPopTime; - private final long changedInvisibleTime; - private final String groupId; - private final String topicId; - private final int queueId; - private final long offset; - private final boolean suspend; - - public ChangeInvisibilityRecord(long popTime, long invisibleTime, long changedPopTime, - long changedInvisibleTime, String groupId, String topicId, int queueId, long offset, boolean suspend) { - this.popTime = popTime; - this.invisibleTime = invisibleTime; - this.changedPopTime = changedPopTime; - this.changedInvisibleTime = changedInvisibleTime; - this.groupId = groupId; - this.topicId = topicId; - this.queueId = queueId; - this.offset = offset; - this.suspend = suspend; - } - - public long getPopTime() { - return popTime; - } - - public long getInvisibleTime() { - return invisibleTime; - } - - public long getChangedPopTime() { - return changedPopTime; - } - - public long getChangedInvisibleTime() { - return changedInvisibleTime; - } - - public String getGroupId() { - return groupId; - } - - public String getTopicId() { - return topicId; - } - - public int getQueueId() { - return queueId; - } - - public long getOffset() { - return offset; - } - - public boolean isSuspend() { - return suspend; - } - } - // Use broker escape bridge to support remote read public CompletableFuture> getMessageAsync(PopConsumerRecord consumerRecord) { return this.brokerController.getEscapeBridge().getMessageAsync(consumerRecord.getTopicId(), diff --git a/broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java b/broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java index 9075d05a575..ac5eeea3205 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java @@ -24,13 +24,13 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Objects; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import org.apache.commons.lang3.StringUtils; import org.apache.rocketmq.broker.BrokerController; import org.apache.rocketmq.broker.offset.ConsumerOffsetManager; import org.apache.rocketmq.broker.offset.MemoryConsumerOrderInfoManager; -import org.apache.rocketmq.broker.pop.PopConsumerService.ChangeInvisibilityRecord; import org.apache.rocketmq.broker.pop.PopConsumerLockService; import org.apache.rocketmq.broker.pop.orderly.ConsumerOrderInfoManager; import org.apache.rocketmq.common.PopAckConstants; @@ -240,14 +240,14 @@ protected CompletableFuture processBatchRequestAsync(final Chan } List> futures = new ArrayList<>(); - if (!prepareBatchRequestEntries(batchRequestHeader, requestEntries)) { + if (!validateBatchRequestEntries(batchRequestHeader, requestEntries)) { response.setCode(ResponseCode.MESSAGE_ILLEGAL); response.setRemark("batch change invisible time entries must use the same topic and consumerGroup as request header"); return CompletableFuture.completedFuture(response); } if (brokerController.getBrokerConfig().isPopConsumerKVServiceEnable()) { - List kvChangeRecords = new ArrayList<>(); + List kvChangeRecords = new ArrayList<>(); List kvIndexes = new ArrayList<>(); List kvSuccessEntries = new ArrayList<>(); for (int i = 0; i < requestEntries.size(); i++) { @@ -288,7 +288,7 @@ protected CompletableFuture processBatchRequestAsync(final Chan }); } - protected boolean prepareBatchRequestEntries(BatchChangeInvisibleTimeRequestHeader batchRequestHeader, + protected boolean validateBatchRequestEntries(BatchChangeInvisibleTimeRequestHeader batchRequestHeader, List requestEntries) { if (batchRequestHeader == null || StringUtils.isBlank(batchRequestHeader.getConsumerGroup()) || @@ -306,8 +306,8 @@ protected boolean prepareBatchRequestEntries(BatchChangeInvisibleTimeRequestHead if (StringUtils.isBlank(requestEntry.getTopic())) { requestEntry.setTopic(batchRequestHeader.getTopic()); } - if (!StringUtils.equals(batchRequestHeader.getConsumerGroup(), requestEntry.getConsumerGroup()) || - !StringUtils.equals(batchRequestHeader.getTopic(), requestEntry.getTopic())) { + if (!Objects.equals(batchRequestHeader.getConsumerGroup(), requestEntry.getConsumerGroup()) || + !Objects.equals(batchRequestHeader.getTopic(), requestEntry.getTopic())) { return false; } } @@ -315,7 +315,7 @@ protected boolean prepareBatchRequestEntries(BatchChangeInvisibleTimeRequestHead } protected boolean tryAppendBatchKvChange(final Channel channel, ChangeInvisibleTimeRequestEntry requestEntry, int index, - ChangeInvisibleTimeResponseEntry[] responseEntries, List kvChangeRecords, + ChangeInvisibleTimeResponseEntry[] responseEntries, List kvChangeRecords, List kvIndexes, List kvSuccessEntries) { if (requestEntry == null) { responseEntries[index] = buildFailedResponseEntry(ResponseCode.SYSTEM_ERROR); @@ -338,10 +338,11 @@ protected boolean tryAppendBatchKvChange(final Channel channel, ChangeInvisibleT } long current = System.currentTimeMillis(); - kvChangeRecords.add(new ChangeInvisibilityRecord( - ExtraInfoUtil.getPopTime(extraInfo), ExtraInfoUtil.getInvisibleTime(extraInfo), current, - requestEntry.getInvisibleTime(), requestEntry.getConsumerGroup(), requestEntry.getTopic(), - requestEntry.getQueueId(), requestEntry.getOffset(), requestEntry.isSuspend())); + requestEntry.setPopTime(ExtraInfoUtil.getPopTime(extraInfo)); + requestEntry.setOldInvisibleTime(ExtraInfoUtil.getInvisibleTime(extraInfo)); + requestEntry.setChangedPopTime(current); + requestEntry.setChangedInvisibleTime(requestEntry.getInvisibleTime()); + kvChangeRecords.add(requestEntry); ChangeInvisibleTimeResponseEntry successEntry = new ChangeInvisibleTimeResponseEntry(); successEntry.setCode(ResponseCode.SUCCESS); successEntry.setPopTime(current); diff --git a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java index fccd16cb577..707b4a34fa5 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerServiceTest.java @@ -47,6 +47,7 @@ import org.apache.rocketmq.common.message.MessageDecoder; import org.apache.rocketmq.common.message.MessageExt; import org.apache.rocketmq.common.message.MessageExtBrokerInner; +import org.apache.rocketmq.remoting.protocol.body.ChangeInvisibleTimeRequestEntry; import org.apache.rocketmq.store.AppendMessageResult; import org.apache.rocketmq.store.AppendMessageStatus; import org.apache.rocketmq.store.GetMessageResult; @@ -593,9 +594,16 @@ public void testBatchChangeInvisibilityDurationDeletesSameVisibilityOldRecordFro consumerCache.writeRecords(Collections.singletonList(oldRecord)); Assert.assertEquals(1, consumerCache.getPopInFlightMessageCount(groupId, topicId, queueId)); - PopConsumerService.ChangeInvisibilityRecord changeRecord = - new PopConsumerService.ChangeInvisibilityRecord(popTime, invisibleTime, changedPopTime, - changedInvisibleTime, groupId, topicId, queueId, offset, false); + ChangeInvisibleTimeRequestEntry changeRecord = new ChangeInvisibleTimeRequestEntry(); + changeRecord.setConsumerGroup(groupId); + changeRecord.setTopic(topicId); + changeRecord.setQueueId(queueId); + changeRecord.setOffset(offset); + changeRecord.setPopTime(popTime); + changeRecord.setOldInvisibleTime(invisibleTime); + changeRecord.setChangedPopTime(changedPopTime); + changeRecord.setChangedInvisibleTime(changedInvisibleTime); + changeRecord.setSuspend(false); consumerService.batchChangeInvisibilityDuration(Collections.singletonList(changeRecord)); Assert.assertEquals(0, consumerCache.getPopInFlightMessageCount(groupId, topicId, queueId)); diff --git a/broker/src/test/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessorTest.java b/broker/src/test/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessorTest.java index 504008f1263..cae8de3df63 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessorTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessorTest.java @@ -25,7 +25,6 @@ import org.apache.rocketmq.broker.metrics.BrokerMetricsManager; import org.apache.rocketmq.broker.metrics.PopMetricsManager; import org.apache.rocketmq.broker.pop.PopConsumerService; -import org.apache.rocketmq.broker.pop.PopConsumerService.ChangeInvisibilityRecord; import org.apache.rocketmq.broker.topic.TopicConfigManager; import com.alibaba.fastjson2.JSON; import org.apache.rocketmq.common.BrokerConfig; @@ -303,13 +302,13 @@ protected ChangeInvisibleTimeRequestHeader buildRequestHeader(ChangeInvisibleTim ArgumentCaptor changeRecordsCaptor = ArgumentCaptor.forClass(List.class); Mockito.verify(popConsumerService).batchChangeInvisibilityDuration(changeRecordsCaptor.capture()); - List changeRecords = changeRecordsCaptor.getValue(); + List changeRecords = changeRecordsCaptor.getValue(); assertThat(changeRecords).hasSize(1); assertThat(changeRecords.get(0).getPopTime()).isEqualTo(popTime); - assertThat(changeRecords.get(0).getInvisibleTime()).isEqualTo(oldInvisibleTime); + assertThat(changeRecords.get(0).getOldInvisibleTime()).isEqualTo(oldInvisibleTime); assertThat(changeRecords.get(0).getChangedInvisibleTime()).isEqualTo(newInvisibleTime); - assertThat(changeRecords.get(0).getGroupId()).isEqualTo(group); - assertThat(changeRecords.get(0).getTopicId()).isEqualTo(topic); + assertThat(changeRecords.get(0).getConsumerGroup()).isEqualTo(group); + assertThat(changeRecords.get(0).getTopic()).isEqualTo(topic); assertThat(changeRecords.get(0).getQueueId()).isEqualTo(0); assertThat(changeRecords.get(0).getOffset()).isEqualTo(queueOffset); } diff --git a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeRequestEntry.java b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeRequestEntry.java index d87aba16297..9315daadef1 100644 --- a/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeRequestEntry.java +++ b/remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/ChangeInvisibleTimeRequestEntry.java @@ -17,6 +17,8 @@ package org.apache.rocketmq.remoting.protocol.body; +import com.alibaba.fastjson2.annotation.JSONField; + public class ChangeInvisibleTimeRequestEntry { private String consumerGroup; private String topic; @@ -27,6 +29,12 @@ public class ChangeInvisibleTimeRequestEntry { private String liteTopic; private boolean suspend; + // broker only + private transient long popTime; + private transient long oldInvisibleTime; + private transient long changedPopTime; + private transient long changedInvisibleTime; + public String getConsumerGroup() { return consumerGroup; } @@ -90,4 +98,44 @@ public boolean isSuspend() { public void setSuspend(boolean suspend) { this.suspend = suspend; } + + @JSONField(serialize = false, deserialize = false) + public long getPopTime() { + return popTime; + } + + @JSONField(serialize = false, deserialize = false) + public void setPopTime(long popTime) { + this.popTime = popTime; + } + + @JSONField(serialize = false, deserialize = false) + public long getOldInvisibleTime() { + return oldInvisibleTime; + } + + @JSONField(serialize = false, deserialize = false) + public void setOldInvisibleTime(long oldInvisibleTime) { + this.oldInvisibleTime = oldInvisibleTime; + } + + @JSONField(serialize = false, deserialize = false) + public long getChangedPopTime() { + return changedPopTime; + } + + @JSONField(serialize = false, deserialize = false) + public void setChangedPopTime(long changedPopTime) { + this.changedPopTime = changedPopTime; + } + + @JSONField(serialize = false, deserialize = false) + public long getChangedInvisibleTime() { + return changedInvisibleTime; + } + + @JSONField(serialize = false, deserialize = false) + public void setChangedInvisibleTime(long changedInvisibleTime) { + this.changedInvisibleTime = changedInvisibleTime; + } } diff --git a/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeTest.java b/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeTest.java index 89540be62c9..3d9aafcb7b7 100644 --- a/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeTest.java +++ b/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/body/BatchChangeInvisibleTimeTest.java @@ -17,6 +17,7 @@ package org.apache.rocketmq.remoting.protocol.body; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collections; import org.apache.rocketmq.remoting.protocol.ResponseCode; @@ -31,6 +32,9 @@ public void testBatchChangeInvisibleTimeRequestBody() { BatchChangeInvisibleTimeRequestBody requestBody = new BatchChangeInvisibleTimeRequestBody(); requestBody.setEntries(Arrays.asList(buildRequestEntry(1), buildRequestEntry(2))); + String json = new String(requestBody.encode(), StandardCharsets.UTF_8); + assertThat(json).doesNotContain("popTime", "oldInvisibleTime", "changedPopTime", "changedInvisibleTime"); + BatchChangeInvisibleTimeRequestBody decoded = BatchChangeInvisibleTimeRequestBody.decode(requestBody.encode(), BatchChangeInvisibleTimeRequestBody.class); @@ -43,6 +47,22 @@ public void testBatchChangeInvisibleTimeRequestBody() { assertThat(decoded.getEntries().get(0).getInvisibleTime()).isEqualTo(3000); assertThat(decoded.getEntries().get(0).getLiteTopic()).isEqualTo("lite"); assertThat(decoded.getEntries().get(0).isSuspend()).isTrue(); + assertThat(decoded.getEntries().get(0).getPopTime()).isZero(); + assertThat(decoded.getEntries().get(0).getOldInvisibleTime()).isZero(); + assertThat(decoded.getEntries().get(0).getChangedPopTime()).isZero(); + assertThat(decoded.getEntries().get(0).getChangedInvisibleTime()).isZero(); + + String rawJsonWithBrokerFields = "{\"entries\":[{\"consumerGroup\":\"group\",\"topic\":\"topic\"," + + "\"queueId\":1,\"extraInfo\":\"0 100 1000 0 broker 1 10\",\"offset\":10," + + "\"invisibleTime\":3000,\"popTime\":100,\"oldInvisibleTime\":1000," + + "\"changedPopTime\":200,\"changedInvisibleTime\":3000}]}"; + BatchChangeInvisibleTimeRequestBody decodedWithBrokerFields = + BatchChangeInvisibleTimeRequestBody.decode(rawJsonWithBrokerFields.getBytes(StandardCharsets.UTF_8), + BatchChangeInvisibleTimeRequestBody.class); + assertThat(decodedWithBrokerFields.getEntries().get(0).getPopTime()).isZero(); + assertThat(decodedWithBrokerFields.getEntries().get(0).getOldInvisibleTime()).isZero(); + assertThat(decodedWithBrokerFields.getEntries().get(0).getChangedPopTime()).isZero(); + assertThat(decodedWithBrokerFields.getEntries().get(0).getChangedInvisibleTime()).isZero(); } @Test @@ -89,6 +109,10 @@ private ChangeInvisibleTimeRequestEntry buildRequestEntry(int queueId) { entry.setInvisibleTime(3000); entry.setLiteTopic("lite"); entry.setSuspend(true); + entry.setPopTime(100); + entry.setOldInvisibleTime(1000); + entry.setChangedPopTime(200); + entry.setChangedInvisibleTime(3000); return entry; } } From 484578a7c2a930bbeb7471af1622cc3415ede623 Mon Sep 17 00:00:00 2001 From: qianye Date: Wed, 17 Jun 2026 16:18:07 +0800 Subject: [PATCH 05/10] Remove redundant pop cache batch validation --- .../rocketmq/broker/pop/PopConsumerCache.java | 25 +---------------- .../broker/pop/PopConsumerCacheTest.java | 28 ------------------- 2 files changed, 1 insertion(+), 52 deletions(-) diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java index 238524e83ca..0b2de785209 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java @@ -124,7 +124,7 @@ public void writeAndDeleteRecords(List writeRecordList, return; } - PopConsumerRecord lockRecord = validateAndGetLockRecord(writeRecordList, deleteRecordList); + PopConsumerRecord lockRecord = deleteRecordList.get(0); String lockTopic = KeyBuilder.parseNormalTopic(lockRecord.getTopicId(), lockRecord.getGroupId()); while (!consumerLockService.tryLock(lockRecord.getGroupId(), lockTopic)) { Thread.yield(); @@ -156,29 +156,6 @@ public void writeAndDeleteRecords(List writeRecordList, } } - private PopConsumerRecord validateAndGetLockRecord(List writeRecordList, - List deleteRecordList) { - PopConsumerRecord lockRecord = null; - lockRecord = validateAndGetLockRecord(lockRecord, deleteRecordList); - return validateAndGetLockRecord(lockRecord, writeRecordList); - } - - private PopConsumerRecord validateAndGetLockRecord(PopConsumerRecord expectedRecord, - List consumerRecordList) { - PopConsumerRecord lockRecord = expectedRecord; - for (PopConsumerRecord consumerRecord : consumerRecordList) { - if (lockRecord == null) { - lockRecord = consumerRecord; - continue; - } - if (!lockRecord.getGroupId().equals(consumerRecord.getGroupId()) || - !lockRecord.getTopicId().equals(consumerRecord.getTopicId())) { - throw new IllegalArgumentException("batch write and delete records must use the same consumer"); - } - } - return lockRecord; - } - public int cleanupRecords(Consumer consumer) { int remain = 0; Iterator> iterator = consumerRecordTable.entrySet().iterator(); diff --git a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java index 688d6466635..2f2b138a3e5 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java @@ -253,34 +253,6 @@ public void writeAndDeleteRecordsShouldLockNormalTopicForRetryTopic() { Mockito.verify(consumerLockService, Mockito.times(1)).unlock(groupId, topicId); } - @Test - public void writeAndDeleteRecordsShouldRejectMixedConsumerLockKey() { - BrokerController brokerController = Mockito.mock(BrokerController.class); - PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); - PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); - Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); - - PopConsumerCache consumerCache = - new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); - PopConsumerRecord oldRecord = new PopConsumerRecord(2L, groupId, topicId, queueId, - 0, 20000, 100, attemptId); - PopConsumerRecord mixedOldRecord = new PopConsumerRecord(3L, groupId, topicId + "_other", queueId, - 0, 20000, 101, attemptId); - PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, topicId, queueId, - 0, 30000, 100, attemptId); - - try { - consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), - Arrays.asList(oldRecord, mixedOldRecord)); - Assert.fail("Should reject mixed consumer lock key"); - } catch (IllegalArgumentException e) { - Assert.assertTrue(e.getMessage().contains("same consumer")); - } - - Mockito.verify(consumerLockService, Mockito.never()).tryLock(anyString(), anyString()); - Mockito.verify(consumerKVStore, Mockito.never()).writeAndDeleteRecords(any(), any()); - } - @Test public void writeAndDeleteRecordsShouldDeleteBufferOnlyWhenStoreKeyMatchesNewRecord() { BrokerController brokerController = Mockito.mock(BrokerController.class); From 765f4eefd0c800f7b33c02c88883f59c5fd4f74c Mon Sep 17 00:00:00 2001 From: qianye Date: Wed, 17 Jun 2026 16:44:22 +0800 Subject: [PATCH 06/10] Simplify pop cache batch write delete --- .../rocketmq/broker/pop/PopConsumerCache.java | 123 +++++++----------- .../broker/pop/PopConsumerCacheTest.java | 68 +--------- 2 files changed, 51 insertions(+), 140 deletions(-) diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java index 0b2de785209..8656496736c 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerCache.java @@ -29,7 +29,6 @@ import org.apache.rocketmq.broker.BrokerController; import org.apache.rocketmq.broker.offset.ConsumerOffsetManager; import org.apache.rocketmq.common.BrokerConfig; -import org.apache.rocketmq.common.KeyBuilder; import org.apache.rocketmq.common.ServiceThread; import org.apache.rocketmq.common.constant.LoggerName; import org.apache.rocketmq.common.utils.ConcurrentHashMapUtils; @@ -120,40 +119,23 @@ public List deleteRecords(List consumerRec public void writeAndDeleteRecords(List writeRecordList, List deleteRecordList) { if (deleteRecordList.isEmpty()) { - consumerRecordStore.writeAndDeleteRecords(writeRecordList, deleteRecordList); + consumerRecordStore.writeRecords(writeRecordList); return; } - PopConsumerRecord lockRecord = deleteRecordList.get(0); - String lockTopic = KeyBuilder.parseNormalTopic(lockRecord.getTopicId(), lockRecord.getGroupId()); - while (!consumerLockService.tryLock(lockRecord.getGroupId(), lockTopic)) { - Thread.yield(); - } - try { - List storeDeleteRecords = new ArrayList<>(deleteRecordList.size()); - List bufferDeleteRecords = new ArrayList<>(deleteRecordList.size()); - deleteRecordList.forEach(consumerRecord -> { - ConsumerRecords consumerRecords = consumerRecordTable.get(this.getKey(consumerRecord)); - if (consumerRecords != null && consumerRecords.contains(consumerRecord)) { - bufferDeleteRecords.add(consumerRecord); - } else { - storeDeleteRecords.add(consumerRecord); - } - }); - - consumerRecordStore.writeAndDeleteRecords(writeRecordList, storeDeleteRecords); - - int deleted = 0; - for (PopConsumerRecord consumerRecord : bufferDeleteRecords) { - ConsumerRecords consumerRecords = consumerRecordTable.get(this.getKey(consumerRecord)); - if (consumerRecords != null && consumerRecords.delete(consumerRecord)) { - deleted++; - } + List storeDeleteRecords = new ArrayList<>(deleteRecordList.size()); + List bufferDeleteRecords = new ArrayList<>(deleteRecordList.size()); + deleteRecordList.forEach(consumerRecord -> { + ConsumerRecords consumerRecords = consumerRecordTable.get(this.getKey(consumerRecord)); + if (consumerRecords != null && consumerRecords.contains(consumerRecord)) { + bufferDeleteRecords.add(consumerRecord); + } else { + storeDeleteRecords.add(consumerRecord); } - this.estimateCacheSize.addAndGet(-deleted); - } finally { - consumerLockService.unlock(lockRecord.getGroupId(), lockTopic); - } + }); + + consumerRecordStore.writeAndDeleteRecords(writeRecordList, storeDeleteRecords); + deleteRecords(bufferDeleteRecords); } public int cleanupRecords(Consumer consumer) { @@ -164,59 +146,44 @@ public int cleanupRecords(Consumer consumer) { ConsumerRecords records = iterator.next().getValue(); boolean timeout = consumerLockService.isLockTimeout( records.getGroupId(), records.getTopicId()); - String lockTopic = KeyBuilder.parseNormalTopic(records.getTopicId(), records.getGroupId()); - if (!consumerLockService.tryLock(records.getGroupId(), lockTopic)) { - continue; - } - try { - if (timeout) { - records.stageExpiredRecords(Long.MAX_VALUE); - List writeConsumerRecords = - new ArrayList<>(records.getRemoveTreeMap().values()); - if (!writeConsumerRecords.isEmpty()) { - consumerRecordStore.writeRecords(writeConsumerRecords); - } - records.clearStagedRecords(); - log.info("PopConsumerOffline, so clean expire records, groupId={}, topic={}, queueId={}, records={}", - records.getGroupId(), records.getTopicId(), records.getQueueId(), records.getInFlightRecordCount()); - iterator.remove(); - continue; - } - long currentTime = System.currentTimeMillis(); - records.stageExpiredRecords(currentTime); - List writeConsumerRecords = new ArrayList<>(); - records.getRemoveTreeMap().values().forEach(record -> { - if (record.getVisibilityTimeout() <= currentTime) { - consumer.accept(record); - } else { - writeConsumerRecords.add(record); - } - }); - - // write to store and handle it later - consumerRecordStore.writeRecords(writeConsumerRecords); + if (timeout) { + records.stageExpiredRecords(Long.MAX_VALUE); + List writeConsumerRecords = + new ArrayList<>(records.getRemoveTreeMap().values()); + if (!writeConsumerRecords.isEmpty()) { + consumerRecordStore.writeRecords(writeConsumerRecords); + } records.clearStagedRecords(); + log.info("PopConsumerOffline, so clean expire records, groupId={}, topic={}, queueId={}, records={}", + records.getGroupId(), records.getTopicId(), records.getQueueId(), records.getInFlightRecordCount()); + iterator.remove(); + continue; + } - // commit min offset in buffer to offset store - long offset = records.getMinOffsetInBuffer(); - if (offset > OFFSET_NOT_EXIST) { - ConsumerOffsetManager consumerOffsetManager = brokerController.getConsumerOffsetManager(); - long commit = consumerOffsetManager.queryOffset(records.getGroupId(), records.getTopicId(), - records.getQueueId()); - if (commit != OFFSET_NOT_EXIST && offset < commit) { - log.info("PopConsumerCache, consumer offset less than store, " + - "groupId={}, topicId={}, queueId={}, offset={}", records.getGroupId(), - records.getTopicId(), records.getQueueId(), offset); - } - consumerOffsetManager.commitOffset("PopConsumerCache", records.getGroupId(), - records.getTopicId(), records.getQueueId(), offset); + long currentTime = System.currentTimeMillis(); + records.stageExpiredRecords(currentTime); + List writeConsumerRecords = new ArrayList<>(); + records.getRemoveTreeMap().values().forEach(record -> { + if (record.getVisibilityTimeout() <= currentTime) { + consumer.accept(record); + } else { + writeConsumerRecords.add(record); } + }); + + // write to store and handle it later + consumerRecordStore.writeRecords(writeConsumerRecords); + records.clearStagedRecords(); - remain += records.getInFlightRecordCount(); - } finally { - consumerLockService.unlock(records.getGroupId(), lockTopic); + // commit min offset in buffer to offset store + long offset = records.getMinOffsetInBuffer(); + if (offset > OFFSET_NOT_EXIST) { + this.commitOffset("PopConsumerCache", + records.getGroupId(), records.getTopicId(), records.getQueueId(), offset); } + + remain += records.getInFlightRecordCount(); } return remain; } diff --git a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java index 2f2b138a3e5..ff08d6730dc 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/pop/PopConsumerCacheTest.java @@ -26,7 +26,6 @@ import org.apache.rocketmq.broker.BrokerController; import org.apache.rocketmq.broker.offset.ConsumerOffsetManager; import org.apache.rocketmq.common.BrokerConfig; -import org.apache.rocketmq.common.KeyBuilder; import org.awaitility.Awaitility; import org.junit.Assert; import org.junit.Test; @@ -103,7 +102,6 @@ public void consumerCacheTest() { PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); - Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); PopConsumerCache consumerCache = new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); @@ -150,30 +148,6 @@ record = new PopConsumerRecord(2L, groupId, topicId, queueId, Assert.assertEquals(0, consumerRecordList.size()); } - @Test - public void cleanupRecordsShouldCommitOffsetWhileHoldingConsumerLock() { - BrokerConfig brokerConfig = new BrokerConfig(); - brokerConfig.setPopCkStayBufferTime(60000); - BrokerController brokerController = Mockito.mock(BrokerController.class); - PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); - PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); - ConsumerOffsetManager consumerOffsetManager = Mockito.mock(ConsumerOffsetManager.class); - Mockito.when(brokerController.getBrokerConfig()).thenReturn(brokerConfig); - Mockito.when(brokerController.getConsumerOffsetManager()).thenReturn(consumerOffsetManager); - Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); - - PopConsumerCache consumerCache = - new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); - PopConsumerRecord record = new PopConsumerRecord(System.currentTimeMillis(), groupId, topicId, queueId, - 0, 20000, 100, attemptId); - consumerCache.writeRecords(Collections.singletonList(record)); - - int remain = consumerCache.cleanupRecords(ignored -> Assert.fail("Record should remain in cache")); - - Assert.assertEquals(1, remain); - Mockito.verify(consumerOffsetManager).commitOffset("PopConsumerCache", groupId, topicId, queueId, 100L); - } - @Test public void writeAndDeleteRecordsShouldSkipStoreDeleteForBufferedRecords() { BrokerController brokerController = Mockito.mock(BrokerController.class); @@ -205,52 +179,22 @@ public void writeAndDeleteRecordsShouldSkipStoreDeleteForBufferedRecords() { } @Test - public void writeAndDeleteRecordsShouldUseSingleConsumerLock() { - BrokerController brokerController = Mockito.mock(BrokerController.class); - PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); - PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); - Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); - Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); - - PopConsumerCache consumerCache = - new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); - PopConsumerRecord oldRecord1 = new PopConsumerRecord(2L, groupId, topicId, queueId, - 0, 20000, 100, attemptId); - PopConsumerRecord oldRecord2 = new PopConsumerRecord(3L, groupId, topicId, queueId + 1, - 0, 20000, 101, attemptId); - PopConsumerRecord newRecord1 = new PopConsumerRecord(4L, groupId, topicId, queueId, - 0, 30000, 100, attemptId); - PopConsumerRecord newRecord2 = new PopConsumerRecord(5L, groupId, topicId, queueId + 1, - 0, 30000, 101, attemptId); - - consumerCache.writeAndDeleteRecords(Arrays.asList(newRecord1, newRecord2), - Arrays.asList(oldRecord1, oldRecord2)); - - Mockito.verify(consumerLockService, Mockito.times(1)).tryLock(groupId, topicId); - Mockito.verify(consumerLockService, Mockito.times(1)).unlock(groupId, topicId); - } - - @Test - public void writeAndDeleteRecordsShouldLockNormalTopicForRetryTopic() { + public void writeAndDeleteRecordsShouldWriteDirectlyWhenDeleteListIsEmpty() { BrokerController brokerController = Mockito.mock(BrokerController.class); PopConsumerKVStore consumerKVStore = Mockito.mock(PopConsumerRocksdbStore.class); PopConsumerLockService consumerLockService = Mockito.mock(PopConsumerLockService.class); Mockito.when(brokerController.getBrokerConfig()).thenReturn(new BrokerConfig()); - Mockito.when(consumerLockService.tryLock(anyString(), anyString())).thenReturn(true); - String retryTopic = KeyBuilder.buildPopRetryTopicV2(topicId, groupId); PopConsumerCache consumerCache = new PopConsumerCache(brokerController, consumerKVStore, consumerLockService, null); - PopConsumerRecord oldRecord = new PopConsumerRecord(2L, groupId, retryTopic, queueId, - 0, 20000, 100, attemptId); - PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, retryTopic, queueId, + PopConsumerRecord newRecord = new PopConsumerRecord(4L, groupId, topicId, queueId, 0, 30000, 100, attemptId); - consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), - Collections.singletonList(oldRecord)); + consumerCache.writeAndDeleteRecords(Collections.singletonList(newRecord), Collections.emptyList()); - Mockito.verify(consumerLockService, Mockito.times(1)).tryLock(groupId, topicId); - Mockito.verify(consumerLockService, Mockito.times(1)).unlock(groupId, topicId); + Mockito.verify(consumerKVStore).writeRecords(Collections.singletonList(newRecord)); + Mockito.verify(consumerKVStore, Mockito.never()).writeAndDeleteRecords(any(), any()); + Mockito.verify(consumerLockService, Mockito.never()).tryLock(anyString(), anyString()); } @Test From 7d2bb3521bcbab4937b546658cb20853ee72177e Mon Sep 17 00:00:00 2001 From: qianye Date: Wed, 17 Jun 2026 17:26:19 +0800 Subject: [PATCH 07/10] Fix remoting serializable compat transient fields --- .../remoting/protocol/RemotingSerializableCompatTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/RemotingSerializableCompatTest.java b/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/RemotingSerializableCompatTest.java index 35c1c7b8912..249833d25f0 100644 --- a/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/RemotingSerializableCompatTest.java +++ b/remoting/src/test/java/org/apache/rocketmq/remoting/protocol/RemotingSerializableCompatTest.java @@ -94,7 +94,7 @@ private void fillDefaultFields(final Object obj, final Class clazz) throws Ex return; } for (Field field : clazz.getDeclaredFields()) { - if (Modifier.isStatic(field.getModifiers())) { + if (Modifier.isStatic(field.getModifiers()) || Modifier.isTransient(field.getModifiers())) { continue; } field.setAccessible(true); @@ -273,7 +273,7 @@ private boolean checkCompatible(final Object original, final Object deserialized Class clazz = original.getClass(); boolean result = true; for (Field field : clazz.getDeclaredFields()) { - if (Modifier.isStatic(field.getModifiers())) { + if (Modifier.isStatic(field.getModifiers()) || Modifier.isTransient(field.getModifiers())) { continue; } JSONField jsonField = field.getAnnotation(JSONField.class); From 29257442844b472f43304fbd125684a016257ff2 Mon Sep 17 00:00:00 2001 From: qianye Date: Thu, 18 Jun 2026 11:55:03 +0800 Subject: [PATCH 08/10] Fix receipt handle processor start test flake --- .../processor/ReceiptHandleProcessorTest.java | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessorTest.java b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessorTest.java index c771515a70c..a93b4533889 100644 --- a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessorTest.java +++ b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ReceiptHandleProcessorTest.java @@ -104,12 +104,27 @@ public void before() throws Throwable { @Test public void testStart() throws Exception { - receiptHandleProcessor.start(); - receiptHandleProcessor.addReceiptHandle(PROXY_CONTEXT, PROXY_CONTEXT.getChannel(), CONSUMER_GROUP, MSG_ID, messageReceiptHandle); Mockito.when(consumerManager.findChannel(Mockito.eq(CONSUMER_GROUP), Mockito.eq(PROXY_CONTEXT.getChannel()))).thenReturn(Mockito.mock(ClientChannelInfo.class)); - Mockito.verify(messagingProcessor, Mockito.timeout(10000).times(1)) - .changeInvisibleTime(Mockito.any(ProxyContext.class), Mockito.any(ReceiptHandle.class), Mockito.eq(MESSAGE_ID), - Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), Mockito.eq(ConfigurationManager.getProxyConfig().getDefaultInvisibleTimeMills()), Mockito.eq(null)); + AckResult ackResult = new AckResult(); + ackResult.setStatus(AckStatus.OK); + ackResult.setExtraInfo(messageReceiptHandle.getReceiptHandleStr()); + Mockito.when(messagingProcessor.changeInvisibleTime( + Mockito.any(ProxyContext.class), Mockito.any(ReceiptHandle.class), Mockito.eq(MESSAGE_ID), + Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), + Mockito.eq(ConfigurationManager.getProxyConfig().getDefaultInvisibleTimeMills()), Mockito.eq(null))) + .thenReturn(CompletableFuture.completedFuture(ackResult)); + + receiptHandleProcessor.addReceiptHandle(PROXY_CONTEXT, PROXY_CONTEXT.getChannel(), CONSUMER_GROUP, MSG_ID, messageReceiptHandle); + try { + receiptHandleProcessor.start(); + Mockito.verify(messagingProcessor, Mockito.timeout(10000).times(1)) + .changeInvisibleTime(Mockito.any(ProxyContext.class), Mockito.any(ReceiptHandle.class), Mockito.eq(MESSAGE_ID), + Mockito.eq(CONSUMER_GROUP), Mockito.eq(TOPIC), Mockito.eq(ConfigurationManager.getProxyConfig().getDefaultInvisibleTimeMills()), Mockito.eq(null)); + } finally { + receiptHandleProcessor.removeReceiptHandle(PROXY_CONTEXT, PROXY_CONTEXT.getChannel(), CONSUMER_GROUP, MSG_ID, + messageReceiptHandle.getReceiptHandleStr()); + receiptHandleProcessor.shutdown(); + } } @Test From 7a2e4bc21397fb347f95d48a4d3950b09fa57067 Mon Sep 17 00:00:00 2001 From: qianye Date: Thu, 18 Jun 2026 17:07:33 +0800 Subject: [PATCH 09/10] Fix batch change invisible time result ordering --- .../broker/pop/PopConsumerService.java | 42 ++++-------- .../proxy/processor/ConsumerProcessor.java | 64 +++++++++++++------ .../processor/ConsumerProcessorTest.java | 64 +++++++++++++++++++ 3 files changed, 122 insertions(+), 48 deletions(-) diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java index 1da0a358182..2b1ae8767af 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java @@ -497,36 +497,18 @@ public CompletableFuture ackAsync( public void changeInvisibilityDuration(long popTime, long invisibleTime, long changedPopTime, long changedInvisibleTime, String groupId, String topicId, int queueId, long offset, boolean suspend) { - - if (brokerConfig.isPopConsumerKVServiceLog()) { - log.info("PopConsumerService change, time={}, invisible={}, " + - "groupId={}, topic={}, queueId={}, offset={}, new time={}, new invisible={}", - popTime, invisibleTime, groupId, topicId, queueId, offset, changedPopTime, changedInvisibleTime); - } - - PopConsumerRecord ckRecord = new PopConsumerRecord( - changedPopTime, groupId, topicId, queueId, 0, changedInvisibleTime, offset, null, suspend); - - PopConsumerRecord ackRecord = new PopConsumerRecord( - popTime, groupId, topicId, queueId, 0, invisibleTime, offset, null, suspend); - - // No need to generate new records when the group does not exist, - // because these retry messages will not be consumed by anyone. - boolean skipWrite = brokerConfig.isPopReviveSkipIfGroupAbsent() && - !brokerController.getSubscriptionGroupManager().containsSubscriptionGroup(groupId); - - if (skipWrite) { - log.info("PopConsumerService change invisibility skip, time={}, " + - "groupId={}, topicId={}, queueId={}, offset={}", popTime, groupId, topicId, queueId, offset); - } - - List ckRecords = skipWrite ? Collections.emptyList() : Collections.singletonList(ckRecord); - List ackRecords = Collections.singletonList(ackRecord); - if (brokerConfig.isEnablePopBufferMerge() && popConsumerCache != null) { - popConsumerCache.writeAndDeleteRecords(ckRecords, ackRecords); - } else { - this.popConsumerStore.writeAndDeleteRecords(ckRecords, ackRecords); - } + ChangeInvisibleTimeRequestEntry changeRecord = new ChangeInvisibleTimeRequestEntry(); + changeRecord.setConsumerGroup(groupId); + changeRecord.setTopic(topicId); + changeRecord.setQueueId(queueId); + changeRecord.setOffset(offset); + changeRecord.setInvisibleTime(changedInvisibleTime); + changeRecord.setPopTime(popTime); + changeRecord.setOldInvisibleTime(invisibleTime); + changeRecord.setChangedPopTime(changedPopTime); + changeRecord.setChangedInvisibleTime(changedInvisibleTime); + changeRecord.setSuspend(suspend); + batchChangeInvisibilityDuration(Collections.singletonList(changeRecord)); } public void batchChangeInvisibilityDuration(List changeRecords) { diff --git a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java index ce38f0f340d..683398edd59 100644 --- a/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java +++ b/proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java @@ -445,40 +445,53 @@ public CompletableFuture> batchChangeInvisi ) { CompletableFuture> future = new CompletableFuture<>(); try { - List batchResultList = new ArrayList<>(handleMessageList.size()); - Map> brokerHandleListMap = new HashMap<>(); + BatchChangeInvisibleTimeResult[] batchResults = new BatchChangeInvisibleTimeResult[handleMessageList.size()]; + Map> brokerHandleIndexMap = new HashMap<>(); - for (ReceiptHandleMessage handleMessage : handleMessageList) { + for (int i = 0; i < handleMessageList.size(); i++) { + ReceiptHandleMessage handleMessage = handleMessageList.get(i); if (handleMessage.getReceiptHandle().isExpired()) { - batchResultList.add(new BatchChangeInvisibleTimeResult(handleMessage, EXPIRED_HANDLE_PROXY_EXCEPTION)); + batchResults[i] = new BatchChangeInvisibleTimeResult(handleMessage, EXPIRED_HANDLE_PROXY_EXCEPTION); continue; } ReceiptHandle handle = handleMessage.getReceiptHandle(); String realTopic = handle.getRealTopic(topic, consumerGroup); String batchKey = handle.getBrokerName() + "@" + realTopic; - brokerHandleListMap.computeIfAbsent(batchKey, key -> new ArrayList<>()) - .add(handleMessage); + brokerHandleIndexMap.computeIfAbsent(batchKey, key -> new ArrayList<>()).add(i); } - if (brokerHandleListMap.isEmpty()) { - return FutureUtils.addExecutor(CompletableFuture.completedFuture(batchResultList), this.executor); + if (brokerHandleIndexMap.isEmpty()) { + return FutureUtils.addExecutor(CompletableFuture.completedFuture( + buildBatchChangeInvisibleTimeResultList(handleMessageList, batchResults)), this.executor); } - CompletableFuture>[] futures = new CompletableFuture[brokerHandleListMap.size()]; - int futureIndex = 0; - for (List brokerHandleList : brokerHandleListMap.values()) { - futures[futureIndex++] = processBrokerChangeInvisibleTime( - ctx, consumerGroup, topic, brokerHandleList, invisibleTime, timeoutMillis, suspend); + List> futures = new ArrayList<>(brokerHandleIndexMap.size()); + for (List brokerHandleIndexes : brokerHandleIndexMap.values()) { + List brokerHandleList = new ArrayList<>(brokerHandleIndexes.size()); + for (Integer index : brokerHandleIndexes) { + brokerHandleList.add(handleMessageList.get(index)); + } + futures.add(processBrokerChangeInvisibleTime( + ctx, consumerGroup, topic, brokerHandleList, invisibleTime, timeoutMillis, suspend) + .thenAccept(results -> { + for (int i = 0; i < brokerHandleIndexes.size(); i++) { + int index = brokerHandleIndexes.get(i); + if (results == null || i >= results.size()) { + batchResults[index] = new BatchChangeInvisibleTimeResult(handleMessageList.get(index), + new ProxyException(ProxyExceptionCode.INTERNAL_SERVER_ERROR, + "batch change invisible time result missing")); + } else { + batchResults[index] = results.get(i); + } + } + })); } - CompletableFuture.allOf(futures).whenComplete((val, throwable) -> { + CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).whenComplete((val, throwable) -> { if (throwable != null) { future.completeExceptionally(throwable); return; } - for (CompletableFuture> resultFuture : futures) { - batchResultList.addAll(resultFuture.join()); - } - future.complete(batchResultList); + future.complete(buildBatchChangeInvisibleTimeResultList(handleMessageList, batchResults)); }); } catch (Throwable t) { future.completeExceptionally(t); @@ -486,6 +499,21 @@ public CompletableFuture> batchChangeInvisi return FutureUtils.addExecutor(future, this.executor); } + protected List buildBatchChangeInvisibleTimeResultList( + List handleMessageList, BatchChangeInvisibleTimeResult[] batchResults) { + List resultList = new ArrayList<>(batchResults.length); + for (int i = 0; i < batchResults.length; i++) { + BatchChangeInvisibleTimeResult result = batchResults[i]; + if (result == null) { + result = new BatchChangeInvisibleTimeResult(handleMessageList.get(i), + new ProxyException(ProxyExceptionCode.INTERNAL_SERVER_ERROR, + "batch change invisible time result missing")); + } + resultList.add(result); + } + return resultList; + } + protected CompletableFuture> processBrokerChangeInvisibleTime( ProxyContext ctx, String consumerGroup, String topic, List handleMessageList, long invisibleTime, long timeoutMillis, boolean suspend) { diff --git a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java index 219725fbf4d..eaac6b944b4 100644 --- a/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java +++ b/proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java @@ -362,6 +362,70 @@ public void testBatchChangeInvisibleTime() throws Throwable { assertNull(msgResult.get(expireMessage.getMsgId()).getAckResult()); } + @Test + public void testBatchChangeInvisibleTimePreserveInputOrderWithExpiredAndInterleavedGroups() throws Throwable { + String brokerName1 = "brokerName1"; + String brokerName2 = "brokerName2"; + long now = System.currentTimeMillis(); + + List receiptHandleMessageList = new ArrayList<>(); + MessageExt broker2Message1 = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, 1, brokerName2); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(broker2Message1), broker2Message1.getMsgId())); + + MessageExt expireMessage = createMessageExt(TOPIC, "", 0, 3000, now - 10000, + 0, 0, 0, 2, brokerName1); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(expireMessage), expireMessage.getMsgId())); + + MessageExt broker1Message1 = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, 3, brokerName1); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(broker1Message1), broker1Message1.getMsgId())); + + MessageExt broker2Message2 = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, 4, brokerName2); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(broker2Message2), broker2Message2.getMsgId())); + + MessageExt broker1Message2 = createMessageExt(TOPIC, "", 0, 3000, now, + 0, 0, 0, 5, brokerName1); + receiptHandleMessageList.add(new ReceiptHandleMessage(create(broker1Message2), broker1Message2.getMsgId())); + + doAnswer((Answer>>) invocation -> { + List handleMessageList = invocation.getArgument(1, List.class); + List ackResultList = new ArrayList<>(); + for (ReceiptHandleMessage handleMessage : handleMessageList) { + AckResult ackResult = new AckResult(); + ackResult.setStatus(AckStatus.OK); + ackResult.setExtraInfo("extra-" + handleMessage.getMessageId()); + ackResultList.add(ackResult); + } + return CompletableFuture.completedFuture(ackResultList); + }).when(this.messageService).batchChangeInvisibleTime( + any(), anyList(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + + List resultList = this.consumerProcessor.batchChangeInvisibleTime( + createContext(), receiptHandleMessageList, CONSUMER_GROUP, TOPIC, 3000, 3000, true).get(); + + assertEquals(receiptHandleMessageList.size(), resultList.size()); + for (int i = 0; i < receiptHandleMessageList.size(); i++) { + BatchChangeInvisibleTimeResult result = resultList.get(i); + ReceiptHandleMessage expectedHandleMessage = receiptHandleMessageList.get(i); + assertSame(expectedHandleMessage, result.getReceiptHandleMessage()); + if (expectedHandleMessage.getReceiptHandle().isExpired()) { + assertEquals(ProxyExceptionCode.INVALID_RECEIPT_HANDLE, result.getProxyException().getCode()); + assertNull(result.getAckResult()); + } else { + assertEquals(AckStatus.OK, result.getAckResult().getStatus()); + assertEquals("extra-" + expectedHandleMessage.getMessageId() + MessageConst.KEY_SEPARATOR + + expectedHandleMessage.getReceiptHandle().getCommitLogOffset(), + result.getAckResult().getExtraInfo()); + assertNull(result.getProxyException()); + } + } + verify(this.messageService, times(2)).batchChangeInvisibleTime( + any(), anyList(), anyString(), anyString(), anyLong(), anyLong(), anyBoolean()); + verify(this.messageService, never()).changeInvisibleTime(any(), any(), anyString(), any(), anyLong()); + } + @Test public void testBatchChangeInvisibleTimeSplitByRealTopic() throws Throwable { String brokerName = "brokerName1"; From 3c5cc1b0d9c9856a547f2a33114bb57f318ad583 Mon Sep 17 00:00:00 2001 From: qianye Date: Thu, 18 Jun 2026 17:32:40 +0800 Subject: [PATCH 10/10] Keep single invisible time change direct --- .../broker/pop/PopConsumerService.java | 41 +++++++++++++------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java index 2b1ae8767af..d5059d11066 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/pop/PopConsumerService.java @@ -497,18 +497,35 @@ public CompletableFuture ackAsync( public void changeInvisibilityDuration(long popTime, long invisibleTime, long changedPopTime, long changedInvisibleTime, String groupId, String topicId, int queueId, long offset, boolean suspend) { - ChangeInvisibleTimeRequestEntry changeRecord = new ChangeInvisibleTimeRequestEntry(); - changeRecord.setConsumerGroup(groupId); - changeRecord.setTopic(topicId); - changeRecord.setQueueId(queueId); - changeRecord.setOffset(offset); - changeRecord.setInvisibleTime(changedInvisibleTime); - changeRecord.setPopTime(popTime); - changeRecord.setOldInvisibleTime(invisibleTime); - changeRecord.setChangedPopTime(changedPopTime); - changeRecord.setChangedInvisibleTime(changedInvisibleTime); - changeRecord.setSuspend(suspend); - batchChangeInvisibilityDuration(Collections.singletonList(changeRecord)); + if (brokerConfig.isPopConsumerKVServiceLog()) { + log.info("PopConsumerService change, time={}, invisible={}, " + + "groupId={}, topic={}, queueId={}, offset={}, new time={}, new invisible={}", + popTime, invisibleTime, groupId, topicId, queueId, offset, changedPopTime, changedInvisibleTime); + } + + PopConsumerRecord ckRecord = new PopConsumerRecord( + changedPopTime, groupId, topicId, queueId, 0, changedInvisibleTime, offset, null, suspend); + + PopConsumerRecord ackRecord = new PopConsumerRecord( + popTime, groupId, topicId, queueId, 0, invisibleTime, offset, null, suspend); + + // No need to generate new records when the group does not exist, + // because these retry messages will not be consumed by anyone. + boolean skipWrite = brokerConfig.isPopReviveSkipIfGroupAbsent() && + !brokerController.getSubscriptionGroupManager().containsSubscriptionGroup(groupId); + + if (skipWrite) { + log.info("PopConsumerService change invisibility skip, time={}, " + + "groupId={}, topicId={}, queueId={}, offset={}", popTime, groupId, topicId, queueId, offset); + } + + List ckRecords = skipWrite ? Collections.emptyList() : Collections.singletonList(ckRecord); + List ackRecords = Collections.singletonList(ackRecord); + if (brokerConfig.isEnablePopBufferMerge() && popConsumerCache != null) { + popConsumerCache.writeAndDeleteRecords(ckRecords, ackRecords); + } else { + this.popConsumerStore.writeAndDeleteRecords(ckRecords, ackRecords); + } } public void batchChangeInvisibilityDuration(List changeRecords) {