[GitHub] [phoenix-omid] stoty commented on a change in pull request #74: OMID-167 fix flaky TestTSOChannelHandlerNetty

[GitBox <gi...@apache.org>]

stoty commented on a change in pull request #74:
URL: https://github.com/apache/phoenix-omid/pull/74#discussion_r521890938



##########
File path: tso-server/src/main/java/org/apache/omid/tso/TSOChannelHandler.java
##########
@@ -104,21 +100,23 @@ void reconnect() {
         // Create the global ChannelGroup
         channelGroup = new DefaultChannelGroup(TSOChannelHandler.class.getName());
         LOG.debug("\tCreating channel to listening for incoming connections in port {}", config.getPort());
-        listeningChannel = bootstrap.bind(new InetSocketAddress(config.getPort()));
+        Channel listeningChannel = bootstrap.bind(new InetSocketAddress(config.getPort()));
         channelGroup.add(listeningChannel);
         LOG.debug("\tListening channel created and connected: {}", listeningChannel);
     }
 
+    /**
+     * @return the listening channels
+     */
+    Set<Channel> channels() {
+        return channelGroup == null ? Collections.<Channel>emptySet() : new HashSet<>(channelGroup);

Review comment:
       What is the point of copying the channels to a new Hash ?
   If you want to block the caller from modfiying it, maybe you could use Collections.unmodifiableSet() ?

##########
File path: common/src/main/java/org/apache/omid/NetworkUtils.java
##########
@@ -17,15 +17,19 @@
  */
 package org.apache.omid;
 
+import org.slf4j.Logger;

Review comment:
       Please see https://omid.incubator.apache.org/coding-guide-and-style.html on import ordering convetions

##########
File path: tso-server/src/main/java/org/apache/omid/tso/TSOChannelHandler.java
##########
@@ -59,43 +61,37 @@
 
     private static final Logger LOG = LoggerFactory.getLogger(TSOChannelHandler.class);
 
-    private final ChannelFactory factory;
-
     private final ServerBootstrap bootstrap;
-
-    @VisibleForTesting
-    Channel listeningChannel;
-    @VisibleForTesting
-    ChannelGroup channelGroup;
-
-    private RequestProcessor requestProcessor;
-
-    private TSOServerConfig config;
-
-    private MetricsRegistry metrics;
+    private ChannelGroup channelGroup;
+    private boolean closed = false;
+    private final RequestProcessor requestProcessor;
+    private final TSOServerConfig config;
+    private final MetricsRegistry metrics;
 
     @Inject
     public TSOChannelHandler(TSOServerConfig config, RequestProcessor requestProcessor, MetricsRegistry metrics) {
-
         this.config = config;
         this.metrics = metrics;
         this.requestProcessor = requestProcessor;
         // Setup netty listener
-        this.factory = new NioServerSocketChannelFactory(
+        this.bootstrap = new ServerBootstrap(new NioServerSocketChannelFactory(

Review comment:
       There is quite bit a refactoring here that is not strictly related to the test failure.
   Please update the ticket summary and/or description to mention this.

##########
File path: tso-server/src/test/java/org/apache/omid/tso/TestTSOChannelHandlerNetty.java
##########
@@ -72,12 +73,13 @@
 
     // Component under test
     private TSOChannelHandler channelHandler;
+    private final int port = NetworkUtils.availablePort();
 
     @BeforeMethod
     public void beforeTestMethod() {
         MockitoAnnotations.initMocks(this);
         TSOServerConfig config = new TSOServerConfig();
-        config.setPort(1434);
+        config.setPort(port);

Review comment:
       This doesn't mesh with the ticket description.
   We are still using the same port for all tests. 
   This may help when the 1434 port is unavailable for some other reason, but AFAICT the TSOChannelHandler lifecycle is the same as it was pre-patch.
   If the problem is really what you describe, then we need to get a new port for every method.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org