asafm commented on code in PR #22058:
URL: https://github.com/apache/pulsar/pull/22058#discussion_r1497913315
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/BrokerServiceLookupTest.java:
##########
@@ -178,6 +188,49 @@ public void testMultipleBrokerLookup() throws Exception {
doReturn(Optional.of(resourceUnit)).when(loadManager2).getLeastLoaded(any(ServiceUnitId.class));
loadManagerField.set(pulsar.getNamespaceService(), new
AtomicReference<>(loadManager1));
+ var metricReader = pulsarTestContext.getOpenTelemetryMetricReader();
Review Comment:
I'm not sure I understand why this won't work.
You hold the latch - making the lookup operation takes as much as you
control. You test the metric before and assert it is equal to 0, then run the
lookup command using the subscribe(), which will make the lock trigger; at this
point, you assert it is equal to 1, and then you release the countDown latch.
It seems like two latches are in order.
Scope-wise: I agree that the test is an integration test, so technically,
mocking namespace service and spying on a specific method is coupling to
implementation. Yet, when I compare mocking a defined service
(NamespaceService) method - actually spying on it - versus an internal
implementation detail - a semaphore created in broker service, which happens to
be used by multiple other classes, I opt for the mocking of namespace service.
In summary, I'm ok with leaving it as is, but I am not very content with it
:)
--
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]
