You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by GitBox <gi...@apache.org> on 2021/02/17 11:07:41 UTC
[GitHub] [phoenix-omid] chrajeshbabu opened a new pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
chrajeshbabu opened a new pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89
…n the tests(Rajeshbabu)
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] chrajeshbabu commented on pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89#issuecomment-780522059
> Usually this construct is used to generate a free port:
>
> https://github.com/apache/hbase/blob/01876071b990063899e0839cdda9745783fe814c/hbase-http/src/test/java/org/apache/hadoop/hbase/http/HttpServerFunctionalTest.java#L278
yes in HBase or Hadoop port 0 will be used to get free port. Let me check the same way.
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] chrajeshbabu commented on pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89#issuecomment-780482831
@stoty took some simple step of creating random number between 1050 to 20000 and use as a random port number in the tests. WDYT?
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] stoty commented on a change in pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
stoty commented on a change in pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89#discussion_r582668337
##########
File path: tso-server/src/test/java/org/apache/omid/tso/TestTSOChannelHandlerNetty.java
##########
@@ -70,186 +72,197 @@
private
RequestProcessor requestProcessor;
- // Component under test
- private TSOChannelHandler channelHandler;
-
@BeforeMethod
public void beforeTestMethod() {
MockitoAnnotations.initMocks(this);
- TSOServerConfig config = new TSOServerConfig();
- config.setPort(1434);
- channelHandler = new TSOChannelHandler(config, requestProcessor, new NullMetricsProvider());
}
- @AfterMethod
- public void afterTestMethod() throws IOException {
- channelHandler.close();
+ private TSOChannelHandler getTSOChannelHandler(int port) {
+ TSOServerConfig config = new TSOServerConfig();
+ config.setPort(port);
+ return new TSOChannelHandler(config, requestProcessor, new NullMetricsProvider());
}
@Test(timeOut = 10_000)
public void testMainAPI() throws Exception {
-
- // Check initial state
- assertNull(channelHandler.listeningChannel);
- assertNull(channelHandler.channelGroup);
-
- // Check initial connection
- channelHandler.reconnect();
- assertTrue(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 1);
- assertEquals(((InetSocketAddress) channelHandler.listeningChannel.getLocalAddress()).getPort(), 1434);
-
- // Check connection close
- channelHandler.closeConnection();
- assertFalse(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 0);
-
- // Check re-closing connection
- channelHandler.closeConnection();
- assertFalse(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 0);
-
- // Check connection after closing
- channelHandler.reconnect();
- assertTrue(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 1);
-
- // Check re-connection
- channelHandler.reconnect();
- assertTrue(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 1);
-
- // Exercise closeable with re-connection trial
- channelHandler.close();
- assertFalse(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 0);
+ int port = NetworkUtils.getFreePort();
+ TSOChannelHandler channelHandler = getTSOChannelHandler(port);
try {
+ // Check initial state
+ assertNull(channelHandler.listeningChannel);
+ assertNull(channelHandler.channelGroup);
+
+ // Check initial connection
channelHandler.reconnect();
- } catch (ChannelException e) {
- // Expected: Can't reconnect after closing
+ assertTrue(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 1);
+ assertEquals(((InetSocketAddress) channelHandler.listeningChannel.getLocalAddress()).getPort(), port);
+
+ // Check connection close
+ channelHandler.closeConnection();
assertFalse(channelHandler.listeningChannel.isOpen());
assertEquals(channelHandler.channelGroup.size(), 0);
- }
+ // Check re-closing connection
+ channelHandler.closeConnection();
+ assertFalse(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 0);
+
+ // Check connection after closing
+ channelHandler.reconnect();
+ assertTrue(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 1);
+
+ // Check re-connection
+ channelHandler.reconnect();
+ assertTrue(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 1);
+
+ // Exercise closeable with re-connection trial
+ channelHandler.close();
+ assertFalse(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 0);
+ try {
+ channelHandler.reconnect();
Review comment:
If this is not supposed to succed, then please add an Assert.fail after.
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] chrajeshbabu edited a comment on pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu edited a comment on pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89#issuecomment-780482831
@stoty took some simple step of creating random number between 1050 to 20000 and use as a random port number in tests not changing any core. WDYT?
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] stoty commented on a change in pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
stoty commented on a change in pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89#discussion_r582666495
##########
File path: hbase-client/src/test/java/org/apache/omid/transaction/OmidTestBase.java
##########
@@ -99,12 +102,12 @@ public void beforeGroups(ITestContext context) throws Exception {
HBaseTimestampStorageConfig hBaseTimestampStorageConfig = injector.getInstance(HBaseTimestampStorageConfig.class);
tso.startAsync();
tso.awaitRunning();
- TestUtils.waitForSocketListening("localhost", 1234, 100);
+ TestUtils.waitForSocketListening("localhost", port, 100);
LOG.info("Finished loading TSO");
context.setAttribute("tso", tso);
OmidClientConfiguration clientConf = new OmidClientConfiguration();
- clientConf.setConnectionString("localhost:1234");
+ clientConf.setConnectionString("localhost:"+port);
Review comment:
please add spaces around "+" here, and every such occurence
##########
File path: hbase-client/src/test/java/org/apache/omid/transaction/TestFilters.java
##########
@@ -71,9 +71,8 @@ public void testGetWithValueFilter(ITestContext context) throws Exception {
private void testGet(ITestContext context, Filter f) throws Exception {
CommitTable.Client commitTableClient = spy(getCommitTable(context).getClient());
-
HBaseOmidClientConfiguration hbaseOmidClientConf = new HBaseOmidClientConfiguration();
- hbaseOmidClientConf.setConnectionString("localhost:1234");
+ hbaseOmidClientConf.setConnectionString("localhost:"+port+"");
Review comment:
The last +"" is not needed, please remove it everywhere in the patch.
##########
File path: tso-server/src/test/java/org/apache/omid/tso/TestTSOChannelHandlerNetty.java
##########
@@ -70,186 +72,197 @@
private
RequestProcessor requestProcessor;
- // Component under test
- private TSOChannelHandler channelHandler;
-
@BeforeMethod
public void beforeTestMethod() {
MockitoAnnotations.initMocks(this);
- TSOServerConfig config = new TSOServerConfig();
- config.setPort(1434);
- channelHandler = new TSOChannelHandler(config, requestProcessor, new NullMetricsProvider());
}
- @AfterMethod
- public void afterTestMethod() throws IOException {
- channelHandler.close();
+ private TSOChannelHandler getTSOChannelHandler(int port) {
+ TSOServerConfig config = new TSOServerConfig();
+ config.setPort(port);
+ return new TSOChannelHandler(config, requestProcessor, new NullMetricsProvider());
}
@Test(timeOut = 10_000)
public void testMainAPI() throws Exception {
-
- // Check initial state
- assertNull(channelHandler.listeningChannel);
- assertNull(channelHandler.channelGroup);
-
- // Check initial connection
- channelHandler.reconnect();
- assertTrue(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 1);
- assertEquals(((InetSocketAddress) channelHandler.listeningChannel.getLocalAddress()).getPort(), 1434);
-
- // Check connection close
- channelHandler.closeConnection();
- assertFalse(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 0);
-
- // Check re-closing connection
- channelHandler.closeConnection();
- assertFalse(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 0);
-
- // Check connection after closing
- channelHandler.reconnect();
- assertTrue(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 1);
-
- // Check re-connection
- channelHandler.reconnect();
- assertTrue(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 1);
-
- // Exercise closeable with re-connection trial
- channelHandler.close();
- assertFalse(channelHandler.listeningChannel.isOpen());
- assertEquals(channelHandler.channelGroup.size(), 0);
+ int port = NetworkUtils.getFreePort();
+ TSOChannelHandler channelHandler = getTSOChannelHandler(port);
try {
+ // Check initial state
+ assertNull(channelHandler.listeningChannel);
+ assertNull(channelHandler.channelGroup);
+
+ // Check initial connection
channelHandler.reconnect();
- } catch (ChannelException e) {
- // Expected: Can't reconnect after closing
+ assertTrue(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 1);
+ assertEquals(((InetSocketAddress) channelHandler.listeningChannel.getLocalAddress()).getPort(), port);
+
+ // Check connection close
+ channelHandler.closeConnection();
assertFalse(channelHandler.listeningChannel.isOpen());
assertEquals(channelHandler.channelGroup.size(), 0);
- }
+ // Check re-closing connection
+ channelHandler.closeConnection();
+ assertFalse(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 0);
+
+ // Check connection after closing
+ channelHandler.reconnect();
+ assertTrue(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 1);
+
+ // Check re-connection
+ channelHandler.reconnect();
+ assertTrue(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 1);
+
+ // Exercise closeable with re-connection trial
+ channelHandler.close();
+ assertFalse(channelHandler.listeningChannel.isOpen());
+ assertEquals(channelHandler.channelGroup.size(), 0);
+ try {
+ channelHandler.reconnect();
Review comment:
If this is not supposed to succed, then please an Assert.fail after.
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] chrajeshbabu closed pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu closed pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] chrajeshbabu commented on pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89#issuecomment-785460431
@stoty handled the review comment and fixed the failing test cases. Please take a look. Thanks
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] stoty commented on pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89#issuecomment-780499275
Usually this construct is used to generate a free port:
https://github.com/apache/hbase/blob/01876071b990063899e0839cdda9745783fe814c/hbase-http/src/test/java/org/apache/hadoop/hbase/http/HttpServerFunctionalTest.java#L278
----------------------------------------------------------------
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
[GitHub] [phoenix-omid] chrajeshbabu edited a comment on pull request #89: OMID-198 Replace static ports used for TSO server with random ports i…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu edited a comment on pull request #89:
URL: https://github.com/apache/phoenix-omid/pull/89#issuecomment-780482831
@stoty @chia7712 took some simple step of creating random number between 1050 to 20000 and use as a random port number in tests not changing any core. WDYT?
----------------------------------------------------------------
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