[pulsar] branch master updated: Avoid race while mocking compactor (#7102)

[mm...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

mmerli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pulsar.git


The following commit(s) were added to refs/heads/master by this push:
     new c579d53  Avoid race while mocking compactor (#7102)
c579d53 is described below

commit c579d53a0f2d9cd6a60cc695f45b75a5a982047e
Author: Matteo Merli <mm...@apache.org>
AuthorDate: Sun May 31 09:17:05 2020 -0700

    Avoid race while mocking compactor (#7102)
    
    There seems to be a race when mocking the compactor in
    MockedPulsarServiceBaseTest which yields errors like:
    ```
    Caused by: java.lang.ClassCastException: org.apache.pulsar.compaction.TwoPhaseCompactor$MockitoMock$1141048386 cannot be cast to org.apache.pulsar.broker.service.BrokerService
    ```
    
    This obviously causes tests to fail in strange and surprising
    ways. I've not been able to reproduce, but the issue seems to be with
    how we mock the compactor.
    
    We don't want to have to construct the whole compactor as we just want
    to spy on it, so we get the current compactor, wrap it in spy and tell
    the PulsarService instance to return it when getCompactor is called.
    
    However, we are doing this after we have already called
    PulsarService#start(). So there are threads already using the mock
    structures. The mock structures themselves are not thread safe, so
    modifying them while they are in use, is not a good idea.
    
    The fix is to finish mocking before invoking #start(). Do do this,
    I've broken out the Compactor construction method in PulsarService, so
    that alone can be mocked to wrap the object in a spy.
    
    Co-authored-by: Ivan Kelly <ik...@splunk.com>
---
 .../main/java/org/apache/pulsar/broker/PulsarService.java | 15 ++++++++-------
 .../pulsar/broker/auth/MockedPulsarServiceBaseTest.java   |  8 +++++---
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
index 2584955..5e515aa 100644
--- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
+++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
@@ -932,15 +932,16 @@ public class PulsarService implements AutoCloseable {
         return this.compactorExecutor;
     }
 
+    // only public so mockito can mock it
+    public Compactor newCompactor() throws PulsarServerException {
+        return new TwoPhaseCompactor(this.getConfiguration(),
+                                     getClient(), getBookKeeperClient(),
+                                     getCompactorExecutor());
+    }
+
     public synchronized Compactor getCompactor() throws PulsarServerException {
         if (this.compactor == null) {
-            try {
-                this.compactor = new TwoPhaseCompactor(this.getConfiguration(),
-                                                       getClient(), getBookKeeperClient(),
-                                                       getCompactorExecutor());
-            } catch (Exception e) {
-                throw new PulsarServerException(e);
-            }
+            this.compactor = newCompactor();
         }
         return this.compactor;
     }
diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java
index 683c220..33fbea4 100644
--- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java
+++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java
@@ -18,6 +18,7 @@
  */
 package org.apache.pulsar.broker.auth;
 
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.spy;
 
@@ -246,9 +247,6 @@ public abstract class MockedPulsarServiceBaseTest {
         pulsar.start();
         conf.setAuthorizationEnabled(isAuthorizationEnabled);
 
-        Compactor spiedCompactor = spy(pulsar.getCompactor());
-        doReturn(spiedCompactor).when(pulsar).getCompactor();
-
         return pulsar;
     }
 
@@ -261,6 +259,10 @@ public abstract class MockedPulsarServiceBaseTest {
         doReturn(namespaceServiceSupplier).when(pulsar).getNamespaceServiceProvider();
 
         doReturn(sameThreadOrderedSafeExecutor).when(pulsar).getOrderedExecutor();
+
+        doAnswer((invocation) -> {
+                return spy(invocation.callRealMethod());
+            }).when(pulsar).newCompactor();
     }
 
     public TenantInfo createDefaultTenantInfo() throws PulsarAdminException {