lhotari commented on code in PR #23449:
URL: https://github.com/apache/pulsar/pull/23449#discussion_r1801689178
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentSubscription.java:
##########
@@ -160,7 +160,29 @@ public synchronized CompletableFuture<Void>
addConsumer(Consumer consumer) {
}
} else {
if (consumer.subType() != dispatcher.getType()) {
- return FutureUtil.failedFuture(new
SubscriptionBusyException("Subscription is of different type"));
+ return FutureUtil.failedFuture(new SubscriptionBusyException(
+ String.format("Subscription is of different type.
Active subscription type of '%s' "
+ + "is different than the connecting
consumer's type '%s'.",
+ dispatcher.getType(), consumer.subType())));
+ } else if (dispatcher.getType() == SubType.Key_Shared) {
+ KeySharedMeta dispatcherKsm =
dispatcher.getConsumers().get(0).getKeySharedMeta();
+ KeySharedMeta consumerKsm = consumer.getKeySharedMeta();
+ if (dispatcherKsm.getKeySharedMode() !=
consumerKsm.getKeySharedMode()) {
+ return FutureUtil.failedFuture(new
SubscriptionBusyException(
+ String.format("Subscription is of different type.
Active subscription key_shared "
+ + "mode of '%s' is different than
the connecting consumer's "
+ + "key_shared mode '%s'.",
+ dispatcherKsm.getKeySharedMode(),
consumerKsm.getKeySharedMode())));
+ }
+ if (dispatcherKsm.isAllowOutOfOrderDelivery() !=
consumerKsm.isAllowOutOfOrderDelivery()) {
+ return FutureUtil.failedFuture(new
SubscriptionBusyException(
+ String.format("Subscription is of different type.
%s",
+ dispatcherKsm.isAllowOutOfOrderDelivery()
+ ? "Active subscription allows out
of order delivery while the "
+ + "connecting consumer does not
allow it." :
+ "Active subscription does not
allow out of order delivery while "
+ + "the connecting consumer
allows it.")));
+ }
}
Review Comment:
> That's an excellent idea. I moved it to AbstractSubscription. The return
type is `Optional<CompletableFuture<Void>>` now. What do you think, would it
make sense to add additional tests for the extracted method to
`AbstractSubscriptionTest`?
I think it's sufficient to test it with the actual behavior that is tested.
Testing private or protected methods is a "code smell" hinting that a new
concept or a "unit" should be extracted which has it's own API that is tested
in a unit test. We don't consistently follow this guideline in the Pulsar code
base so there's a lot of bad examples around. We can improve over time.
Reference
https://testing.googleblog.com/2013/08/testing-on-toilet-test-behavior-not.html?m=1
--
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]
