Respond to general feedback

neo4j · Apr 20, 2018 · c64434b · c64434b
1 parent c6180e9
commit c64434b
Show file tree

Hide file tree

Showing 7 changed files with 98 additions and 90 deletions.
diff --git a/...ing/src/main/java/org/neo4j/causalclustering/core/state/snapshot/CoreStateDownloader.java b/...ing/src/main/java/org/neo4j/causalclustering/core/state/snapshot/CoreStateDownloader.java
@@ -211,7 +211,7 @@ else if ( catchupResult != SUCCESS_END_OF_STREAM )
         ensure( localDatabase::start, "start local database after store copy" );
 
         coreStateMachines.installCommitProcess( localDatabase.getCommitProcess() );
-        ensure( enableDisableOnStoreCopy::enable, "start auxiliary services after store copy" );
+        ensure( enableDisableOnStoreCopy::enable, "enable auxiliary services after store copy" );
 
         return true;
     }

diff --git a/...ausal-clustering/src/main/java/org/neo4j/causalclustering/helper/EnableableLifeCycle.java b/...ausal-clustering/src/main/java/org/neo4j/causalclustering/helper/EnableableLifeCycle.java
@@ -34,7 +34,7 @@ public EnableableLifeCycle( Log debugLog )
     }
 
     @Override
-    public synchronized void enable() throws Throwable
+    public final synchronized void enable() throws Throwable
     {
         if ( !stoppedByLifeCycle )
         {
@@ -48,20 +48,20 @@ public synchronized void enable() throws Throwable
     }
 
     @Override
-    public synchronized void disable() throws Throwable
+    public final synchronized void disable() throws Throwable
     {
         stop0();
         enabled = false;
     }
 
     @Override
-    public synchronized void init() throws Throwable
+    public final synchronized void init() throws Throwable
     {
         init0();
     }
 
     @Override
-    public synchronized void start() throws Throwable
+    public final synchronized void start() throws Throwable
     {
         if ( !enabled )
         {
@@ -75,14 +75,14 @@ public synchronized void start() throws Throwable
     }
 
     @Override
-    public synchronized void stop() throws Throwable
+    public final synchronized void stop() throws Throwable
     {
         stop0();
         stoppedByLifeCycle = true;
     }
 
     @Override
-    public synchronized void shutdown() throws Throwable
+    public final synchronized void shutdown() throws Throwable
     {
         shutdown0();
         stoppedByLifeCycle = true;

diff --git a/enterprise/causal-clustering/src/main/java/org/neo4j/causalclustering/net/Server.java b/enterprise/causal-clustering/src/main/java/org/neo4j/causalclustering/net/Server.java
@@ -37,6 +37,8 @@
 import org.neo4j.logging.LogProvider;
 import org.neo4j.logging.NullLogProvider;
 
+import static java.lang.String.format;
+
 public class Server extends EnableableLifeCycle
 {
     private final Log debugLog;
@@ -161,6 +163,6 @@ public ListenSocketAddress address()
     @Override
     public String toString()
     {
-        return serverName;
+        return format( "Server[%s]", serverName );
     }
 }
diff --git a/...g/src/test/java/org/neo4j/causalclustering/helper/EnableableLifeCycleStateChangeTest.java b/...g/src/test/java/org/neo4j/causalclustering/helper/EnableableLifeCycleStateChangeTest.java
@@ -32,7 +32,6 @@
 import org.neo4j.logging.AssertableLogProvider;
 
 import static org.junit.Assert.assertEquals;
-import static org.neo4j.causalclustering.helper.EnableableLifecycleStateTestHelpers.setEnableableState;
 import static org.neo4j.causalclustering.helper.EnableableLifecycleStateTestHelpers.setInitialState;
 
 @RunWith( Parameterized.class )
@@ -79,7 +78,7 @@ public void setUpServer() throws Throwable
     {
         lifeCycle = new StateAwareEnableableLifeCycle( new AssertableLogProvider( false ).getLog( "log" ) );
         setInitialState( lifeCycle, fromState );
-        setEnableableState( lifeCycle, fromEnableableState );
+        fromEnableableState.set( lifeCycle );
     }
 
     @Test

diff --git a/...java/org/neo4j/causalclustering/helper/EnableableLifeCylcleEnableableStateChangeTest.java b/...java/org/neo4j/causalclustering/helper/EnableableLifeCylcleEnableableStateChangeTest.java
@@ -32,7 +32,6 @@
 import org.neo4j.logging.NullLogProvider;
 
 import static org.junit.Assert.assertEquals;
-import static org.neo4j.causalclustering.helper.EnableableLifecycleStateTestHelpers.setEnableableState;
 import static org.neo4j.causalclustering.helper.EnableableLifecycleStateTestHelpers.setInitialState;
 
 @RunWith( Parameterized.class )
@@ -79,13 +78,13 @@ public void setUpServer() throws Throwable
     {
         lifeCycle = new StateAwareEnableableLifeCycle( NullLogProvider.getInstance().getLog( "log" ) );
         setInitialState( lifeCycle, fromState );
-        setEnableableState( lifeCycle, fromEnableableState );
+        fromEnableableState.set( lifeCycle );
     }
 
     @Test
     public void executeEnableable() throws Throwable
     {
-        setEnableableState( lifeCycle, toEnableableState );
+        toEnableableState.set( lifeCycle );
         assertEquals( shouldEndInState, lifeCycle.status );
     }
 

diff --git a/.../src/test/java/org/neo4j/causalclustering/helper/EnableableLifecycleStateTestHelpers.java b/.../src/test/java/org/neo4j/causalclustering/helper/EnableableLifecycleStateTestHelpers.java
@@ -19,27 +19,11 @@
  */
 package org.neo4j.causalclustering.helper;
 
+import org.neo4j.function.ThrowingConsumer;
 import org.neo4j.kernel.lifecycle.Lifecycle;
 
 class EnableableLifecycleStateTestHelpers
 {
-    static void setEnableableState( Enableable enableable, EnableableState enableableState ) throws Throwable
-    {
-        switch ( enableableState )
-        {
-        case Enabled:
-            enableable.enable();
-            return;
-        case Disabled:
-            enableable.disable();
-            return;
-        case Untouched:
-            return;
-        default:
-            throw new IllegalStateException( "Not recognized state " + enableableState );
-        }
-    }
-
     static void setInitialState( StateAwareEnableableLifeCycle lifeCycle, LifeCycleState state ) throws Throwable
     {
         for ( LifeCycleState lifeCycleState : LifeCycleState.values() )
@@ -53,46 +37,40 @@ static void setInitialState( StateAwareEnableableLifeCycle lifeCycle, LifeCycleS
 
     enum LifeCycleState
     {
-        Init
-                {
-                    @Override
-                    void set( Lifecycle lifecycle ) throws Throwable
-                    {
-                        lifecycle.init();
-                    }
-                },
-        Start
-                {
-                    @Override
-                    void set( Lifecycle lifecycle ) throws Throwable
-                    {
-                        lifecycle.start();
-                    }
-                },
-        Stop
-                {
-                    @Override
-                    void set( Lifecycle lifecycle ) throws Throwable
-                    {
-                        lifecycle.stop();
-                    }
-                },
-        Shutdown
-                {
-                    @Override
-                    void set( Lifecycle lifecycle ) throws Throwable
-                    {
-                        lifecycle.shutdown();
-                    }
-                };
+        Init( Lifecycle::init ),
+        Start( Lifecycle::start ),
+        Stop( Lifecycle::stop ),
+        Shutdown( Lifecycle::shutdown );
+
+        private final ThrowingConsumer<Lifecycle,Throwable> operation;
+
+        LifeCycleState( ThrowingConsumer<Lifecycle,Throwable> operation )
+        {
+            this.operation = operation;
+        }
 
-        abstract void set( Lifecycle lifecycle ) throws Throwable;
+        void set( Lifecycle lifecycle ) throws Throwable
+        {
+            operation.accept( lifecycle );
+        }
     }
 
     enum EnableableState
     {
-        Untouched,
-        Enabled,
-        Disabled
+        Untouched( enableable -> {} ),
+        Enabled( Enableable::enable ),
+        Disabled( Enableable::disable );
+
+        private final ThrowingConsumer<Enableable,Throwable> consumer;
+
+        EnableableState( ThrowingConsumer<Enableable,Throwable> consumer )
+        {
+            this.consumer = consumer;
+        }
+
+        void set( Enableable enableable ) throws Throwable
+        {
+            consumer.accept( enableable );
+        }
     }
 }
diff --git a/...prise/causal-clustering/src/test/java/org/neo4j/causalclustering/net/ServerStateTest.java b/...prise/causal-clustering/src/test/java/org/neo4j/causalclustering/net/ServerStateTest.java
@@ -20,12 +20,16 @@
 package org.neo4j.causalclustering.net;
 
 import io.netty.bootstrap.Bootstrap;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelInitializer;
 import io.netty.channel.EventLoopGroup;
 import io.netty.channel.nio.NioEventLoopGroup;
 import io.netty.channel.socket.nio.NioSocketChannel;
 import org.junit.After;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
 import org.neo4j.causalclustering.helper.EnableableLifeCycleStateChangeTest;
@@ -44,52 +48,84 @@
  */
 public class ServerStateTest
 {
+    private static Bootstrap bootstrap;
+    private static EventLoopGroup clientGroup;
     private Server server;
-    private final EventLoopGroup clientGroup = new NioEventLoopGroup();
+    private Channel channel;
+
+    @BeforeClass
+    public static void initialSetup()
+    {
+        clientGroup = new NioEventLoopGroup();
+        bootstrap = new Bootstrap()
+                .group( clientGroup )
+                .channel( NioSocketChannel.class )
+                .handler( new ChannelInitializer<NioSocketChannel>()
+                {
+                    @Override
+                    protected void initChannel( NioSocketChannel ch )
+                    {
+
+                    }
+                } );
+    }
 
     @Before
     public void setUp() throws Throwable
     {
         server = createServer();
         server.init();
-        assertFalse( canConnect( server.address(), clientGroup ) );
+        assertFalse( canConnect() );
     }
 
     @After
     public void tearDown() throws Throwable
     {
-        server.stop();
-        server.shutdown();
+        if ( server != null )
+        {
+            server.stop();
+            server.shutdown();
+        }
+        if ( channel != null )
+        {
+            channel.close();
+        }
+    }
+
+    @AfterClass
+    public static void finalTearDown()
+    {
+        clientGroup.shutdownGracefully();
     }
 
     @Test
     public void shouldStartServerNormally() throws Throwable
     {
         server.start();
-        assertTrue( canConnect( server.address(), clientGroup ) );
+        assertTrue( canConnect() );
     }
 
     @Test
     public void canDisableAndEnableServer() throws Throwable
     {
         server.start();
-        assertTrue( canConnect( server.address(), clientGroup ) );
+        assertTrue( canConnect() );
 
         server.disable();
-        assertFalse( canConnect( server.address(), clientGroup ) );
+        assertFalse( canConnect() );
 
         server.enable();
-        assertTrue( canConnect( server.address(), clientGroup ) );
+        assertTrue( canConnect() );
     }
 
     @Test
     public void serverCannotBeEnabledIfLifeCycleHasNotStarted() throws Throwable
     {
         server.enable();
-        assertFalse( canConnect( server.address(), clientGroup ) );
+        assertFalse( canConnect() );
 
         server.start();
-        assertTrue( canConnect( server.address(), clientGroup ) );
+        assertTrue( canConnect() );
     }
 
     @Test
@@ -98,30 +134,24 @@ public void serverCannotStartIfDisabled() throws Throwable
         server.disable();
 
         server.start();
-        assertFalse( canConnect( server.address(), clientGroup ) );
+        assertFalse( canConnect() );
 
         server.enable();
-        assertTrue( canConnect( server.address(), clientGroup ) );
+        assertTrue( canConnect() );
     }
 
     private static Server createServer()
     {
-        return new Server( channel ->
-                           {
-                           }, FormattedLogProvider.withDefaultLogLevel( Level.DEBUG ).toOutputStream( System.out ),
+        return new Server( channel -> {}, FormattedLogProvider.withDefaultLogLevel( Level.DEBUG ).toOutputStream( System.out ),
                            FormattedLogProvider.withDefaultLogLevel( Level.DEBUG ).toOutputStream( System.out ),
                            new ListenSocketAddress( "localhost", PortAuthority.allocatePort() ), "serverName" );
     }
 
-    private static boolean canConnect( ListenSocketAddress socketAddress, EventLoopGroup eventExecutors ) throws InterruptedException
+    private boolean canConnect() throws InterruptedException
     {
-        return new Bootstrap().group( eventExecutors ).channel( NioSocketChannel.class ).handler( new ChannelInitializer<NioSocketChannel>()
-        {
-            @Override
-            protected void initChannel( NioSocketChannel ch )
-            {
-
-            }
-        } ).connect( socketAddress.getHostname(), socketAddress.getPort() ).await().isSuccess();
+        ListenSocketAddress socketAddress = server.address();
+        ChannelFuture channelFuture = bootstrap.connect( socketAddress.getHostname(), socketAddress.getPort() );
+        channel = channelFuture.channel();
+        return channelFuture.await().isSuccess();
     }
 }