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:
us...@infra.apache.org
