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] Extend unicast ports generation to support more concurrent clusters #8634

Merged
merged 1 commit into from
Nov 27, 2014

Conversation

javanna
Copy link
Member

@javanna javanna commented Nov 24, 2014

The current UnicastZen only supports up to 10 different clusters per jvm, after which it generates ports that overlap between different jvms.

Make it possible to run multiple tests with unicast configuration, by assigning ports based on their test scope.
Every jvm still gets its own port range based on the jvm id, but we now make sure that the different jvms ranges never overlap. The global cluster gets a reserved port range, while SUITE and TEST scopes are treated equally, just assuming that they never run concurrently on the same jvm, thus ports can be safely reused.

@javanna javanna added >test Issues or PRs that are addressing/adding tests review labels Nov 24, 2014
default:
//ports can be reused as suite or test clusters are never run concurrently
//prevent conflicts between jvms by never going above 9
return RandomizedTest.randomIntBetween(1, 9);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not round robin on this? I think the randomness still allows us to have collisions and will keep us wondering.

+1 on the insight that suite and test scope don't co-exists! Also, this makes us one step closer to using it randomly in our global cluster scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

why not round robin on this? I think the randomness still allows us to have collisions and will keep us wondering.

the assumption is that only one cluster with either SUITE or TEST is on at a given time within a jvm, so there shouldn't be conflicts. Randomizing is not strictly necessary (we just have to skip 0 and make sure not to go above 9) but it does help verifying our assumption? I am conflicted here...

+1 on the insight that suite and test scope don't co-exists! Also, this makes us one step closer to using it randomly in our global cluster scope.

Indeed, this change should make this possible as the next step!

Copy link
Contributor

Choose a reason for hiding this comment

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

Randomness is awesome when it's needed, but also adds complexity. Also, we need to let the OS free the old ports before reusing them. I think having some gaps is good? I'm hoping a cycle of 9 is good enough, but we'll see...

@bleskes
Copy link
Contributor

bleskes commented Nov 26, 2014

I like it. Thx Luca for looking into this. Left two comments.

@javanna
Copy link
Member Author

javanna commented Nov 27, 2014

Thanks @bleskes updated

@@ -39,6 +39,8 @@
final Settings nodeSettings;
final Settings transportClientSettings;

private static final AtomicInteger portCounter = new AtomicInteger();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is used for the unicast discovery - can we this back to the class? (or reuse the old variable :) )

Copy link
Member Author

Choose a reason for hiding this comment

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

yep

@bleskes
Copy link
Contributor

bleskes commented Nov 27, 2014

@javanna two little minor details and we're done. thx.

@javanna
Copy link
Member Author

javanna commented Nov 27, 2014

thanks @bleskes can you have another look please?

@bleskes
Copy link
Contributor

bleskes commented Nov 27, 2014

LGTM!

…sters

Make it possible to run multiple tests with unicast configuration, by assigning ports based on their test scope.
Every jvm still gets its own port range based on the jvm id, but we now make sure that the different jvms ranges never overlap. The global cluster gets a reserved port range, while SUITE and TEST scopes are treated equally, just assuming that they never run concurrently on the same jvm, thus ports can be safely reused.

Closes elastic#8634
javanna added a commit to javanna/elasticsearch that referenced this pull request Nov 27, 2014
…sters

Make it possible to run multiple tests with unicast configuration, by assigning ports based on their test scope.
Every jvm still gets its own port range based on the jvm id, but we now make sure that the different jvms ranges never overlap. The global cluster gets a reserved port range, while SUITE and TEST scopes are treated equally, just assuming that they never run concurrently on the same jvm, thus ports can be safely reused.

Closes elastic#8634
@javanna javanna merged commit e07b0de into elastic:master Nov 27, 2014
@javanna javanna removed the review label Nov 27, 2014
javanna added a commit that referenced this pull request Dec 2, 2014
None gateway was removed in master hence setting it to local is the default there, applied in #8634 . 1.x still requires setting the geteway type to local explicitly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>test Issues or PRs that are addressing/adding tests v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants