You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "abmo-x (via GitHub)" <gi...@apache.org> on 2023/02/21 17:52:39 UTC
[GitHub] [iceberg] abmo-x opened a new pull request, #6895: Core: use random port for rest catalog unit tests
abmo-x opened a new pull request, #6895:
URL: https://github.com/apache/iceberg/pull/6895
Rest catalog test uses fixed port 8181, which can cause random test failures on CI.
Fix to use random available port for unit tests.
Failure on oss https://github.com/apache/iceberg/actions/runs/4229402634/jobs/7345745908
<img width="876" alt="image" src="https://media.github.pie.apple.com/user/128939/files/dce1d1e0-458b-4bb2-8d21-af0240325143">
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] youngxinler commented on pull request #6895: Core: use random port for rest catalog unit tests
Posted by "youngxinler (via GitHub)" <gi...@apache.org>.
youngxinler commented on PR #6895:
URL: https://github.com/apache/iceberg/pull/6895#issuecomment-1446036220
nice job, I also ran into this problem two days ago. thanks @abmo-x
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6895: Core: use random port for rest catalog unit tests
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6895:
URL: https://github.com/apache/iceberg/pull/6895#discussion_r1114240697
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -150,7 +154,8 @@ public <T extends RESTResponse> T execute(
servletContext.setVirtualHosts(null);
servletContext.setGzipHandler(new GzipHandler());
- this.httpServer = new Server(8181);
+ initializePort();
+ this.httpServer = new Server(port);
Review Comment:
Jetty already provides functionality to bind to a random port by setting `this.httpServer = new Server(0);`.
And then we can call `localPort()` in those 2 methods that require the port. The content of the method should be:
```
private int localPort() {
assertThat(httpServer.isRunning()).isTrue();
return ((ServerConnector) httpServer.getConnectors()[0]).getLocalPort();
}
```
Therefore we don't need to introduce a `port` variable and also we don't need to find a random port ourselves
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko merged pull request #6895: Core: use random port for rest catalog unit tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #6895:
URL: https://github.com/apache/iceberg/pull/6895
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #6895: Core: use random port for rest catalog unit tests
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #6895:
URL: https://github.com/apache/iceberg/pull/6895#discussion_r1114688001
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -150,7 +154,8 @@ public <T extends RESTResponse> T execute(
servletContext.setVirtualHosts(null);
servletContext.setGzipHandler(new GzipHandler());
- this.httpServer = new Server(8181);
+ initializePort();
+ this.httpServer = new Server(port);
Review Comment:
Makes sense! updated it, thanks for the code snippet.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on pull request #6895: Core: use random port for rest catalog unit tests
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on PR #6895:
URL: https://github.com/apache/iceberg/pull/6895#issuecomment-1446856475
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #6895: Core: use random port for rest catalog unit tests
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #6895:
URL: https://github.com/apache/iceberg/pull/6895#discussion_r1113446608
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -150,7 +154,8 @@ public <T extends RESTResponse> T execute(
servletContext.setVirtualHosts(null);
servletContext.setGzipHandler(new GzipHandler());
- this.httpServer = new Server(8181);
+ initializePort();
+ this.httpServer = new Server(port);
Review Comment:
[doubt] Thinking of a case like, we find the free port first and then try to create a server, can it happen that in before creating a server the free port we found, can get occupied, by some other process ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #6895: Core: use random port for rest catalog unit tests
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #6895:
URL: https://github.com/apache/iceberg/pull/6895#discussion_r1113504174
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -150,7 +154,8 @@ public <T extends RESTResponse> T execute(
servletContext.setVirtualHosts(null);
servletContext.setGzipHandler(new GzipHandler());
- this.httpServer = new Server(8181);
+ initializePort();
+ this.httpServer = new Server(port);
Review Comment:
I think the chances of that happening are pretty low as the binding port is determined by the kernel randomly from available ports. It can happen but very low as all this is happening is nano seconds at the kernel to pick a socket, close it and then start the rest server at that port.
I think this is better than having it hard coded to 8181 as we know our CI build fails due to parallel runs
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] abmo-x commented on a diff in pull request #6895: Core: use random port for rest catalog unit tests
Posted by "abmo-x (via GitHub)" <gi...@apache.org>.
abmo-x commented on code in PR #6895:
URL: https://github.com/apache/iceberg/pull/6895#discussion_r1113504174
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -150,7 +154,8 @@ public <T extends RESTResponse> T execute(
servletContext.setVirtualHosts(null);
servletContext.setGzipHandler(new GzipHandler());
- this.httpServer = new Server(8181);
+ initializePort();
+ this.httpServer = new Server(port);
Review Comment:
I think the chances of that happening are pretty low as the binding port is determined by the kernel randomly from available ports. It can happen but very low as all this is happening at the kernel to pick a socket, close it and then start the rest server at that port.
I think this is better than having it hard coded to 8181 as we know our CI build fails due to parallel runs
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org