Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TEST] Select free port for Minio #32837

Merged
merged 2 commits into from
Aug 14, 2018
Merged

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Aug 14, 2018

Minio does not support dynamic ports. The workaround here is to scan for a free port first. This is not foolproof, but as we don't expect too many of these builds to run at once on the same machine, this should do the trick.

Closes #32701 and #32208

@ywelsch ywelsch added >test Issues or PRs that are addressing/adding tests :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 v6.5.0 v6.4.1 labels Aug 14, 2018
@ywelsch ywelsch requested review from tlrx and alpar-t August 14, 2018 09:12
Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I left a minor comment

// get free port
for (int port = 60920; port < 60940; port++) {
try {
javax.net.ServerSocketFactory.getDefault().createServerSocket(port, 1, InetAddress.getByName(minioAddress)).close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we let the default 0 value for the backlog instead of 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it matters (we shouldn't expect anyone to connect to this), but I'm just following the example of https:/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/util/SocketUtils.java#L201-L202 here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. Let's use 1.


doLast {
// get free port
for (int port = 60920; port < 60940; port++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be (60920..60942).each{

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meh, love my for loops.

minioPort = port
break
} catch (Exception e) {
logger.error("Failed to determine free port " + port + " for Minio process", e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not log this at error level, as the retry will solve it, also would be better to catch the specific BindException and just log info

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I've fixed this in d4add8b. Catching Exception here will possibly hide other issues.

@ywelsch ywelsch merged commit cd0de16 into elastic:master Aug 14, 2018
ywelsch added a commit that referenced this pull request Aug 14, 2018
Minio does not support dynamic ports. The workaround here is to scan for a free port first. This is
not foolproof, but as we don't expect too many of these builds to run at once on the same machine,
this should do the trick.

Closes #32701
Closes #32208
ywelsch added a commit that referenced this pull request Aug 14, 2018
Minio does not support dynamic ports. The workaround here is to scan for a free port first. This is
not foolproof, but as we don't expect too many of these builds to run at once on the same machine,
this should do the trick.

Closes #32701
Closes #32208
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2018
…e-types

* elastic/master: (199 commits)
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2018
…listeners

* elastic/master:
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
jasontedor added a commit that referenced this pull request Aug 15, 2018
* 6.x: (96 commits)
  Introduce global checkpoint listeners (#32696)
  Use JDK 10 for 6.4 BWC builds (#32866)
  Remove unused imports - follow up to removal of test in issue 32855
  Removed flaky test. Looks like randomisation makes these assertions unreliable. This test is superfluous - it was added to address #32770 but it later turned out there was an existing test that just required a fix to provide the missing test coverage.
  [test] mute IndexShardTests.testDocStats
  Test: Fix forbidden uses in test framework (#32824)
  Security: remove password hash bootstrap check (#32440)
  Move validation to server for put user requests (#32471)
  [ML] Add high level REST client docs for ML put job endpoint (#32843)
  Painless: Change fqn_only to no_import (#32817)
  [test] mute testSearchWithSignificantTermsAgg
  Backport: CompletableContext class to avoid throwable (#32829)
  [TEST] Select free port for Minio (#32837)
  SCRIPTING: Support BucketAggScript return null (#32811) (#32833)
  HLRC: Add Delete License API (#32586)
  Aggregations/HL Rest client fix: missing scores (#32774)
  HLRC: migration get assistance API (#32744)
  Fix NOOP bulk updates (#32819)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >test Issues or PRs that are addressing/adding tests v6.4.1 v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants