Copilot commented on code in PR #25077:
URL: https://github.com/apache/pulsar/pull/25077#discussion_r2626559363
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SharedConsumerAssignor.java:
##########
@@ -58,7 +62,11 @@ public Map<Consumer, List<EntryAndMetadata>> assign(final
List<EntryAndMetadata>
Consumer consumer = getConsumer(numConsumers);
if (consumer == null) {
- entryAndMetadataList.forEach(EntryAndMetadata::release);
+ if (subscription != null) {
+ log.info("Not found assign consumer in
topic:{},subscription:{}, redelivery {} messages.",
+ subscription.getTopic(), subscription.getName(),
entryAndMetadataList.size());
+ }
+ entryAndMetadataList.forEach(unassignedMessageProcessor);
Review Comment:
The fix for handling chunked messages when no consumers are available lacks
test coverage. Consider adding a test case in SharedConsumerAssignorTest that
verifies the behavior when `numConsumers` is 0 or when the consumer selector
returns null from the start. The test should verify that chunked messages are
properly added to the replay queue instead of being released.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SharedConsumerAssignor.java:
##########
@@ -58,7 +62,11 @@ public Map<Consumer, List<EntryAndMetadata>> assign(final
List<EntryAndMetadata>
Consumer consumer = getConsumer(numConsumers);
if (consumer == null) {
- entryAndMetadataList.forEach(EntryAndMetadata::release);
+ if (subscription != null) {
+ log.info("Not found assign consumer in
topic:{},subscription:{}, redelivery {} messages.",
Review Comment:
The log message "Not found assign consumer" has grammatical issues. Consider
rephrasing to "No consumer found to assign" or "Could not find consumer for
assignment" for better clarity and readability.
```suggestion
log.info("No consumer found to assign in topic:{},
subscription:{}, redelivering {} messages.",
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SharedConsumerAssignor.java:
##########
@@ -58,7 +62,11 @@ public Map<Consumer, List<EntryAndMetadata>> assign(final
List<EntryAndMetadata>
Consumer consumer = getConsumer(numConsumers);
if (consumer == null) {
- entryAndMetadataList.forEach(EntryAndMetadata::release);
+ if (subscription != null) {
+ log.info("Not found assign consumer in
topic:{},subscription:{}, redelivery {} messages.",
+ subscription.getTopic(), subscription.getName(),
entryAndMetadataList.size());
Review Comment:
The log statement uses `subscription.getTopic()` which returns a Topic
object, not a String. This will invoke the object's toString() method which may
not produce a user-friendly topic name. Consider using
`subscription.getTopic().getName()` instead to get the actual topic name string.
```suggestion
subscription.getTopic().getName(),
subscription.getName(), entryAndMetadataList.size());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
