[GitHub] [geode] WireBaron opened pull request #2958: GEODE-5674: Stop using random values for test ports

["WireBaron (GitHub)" <gi...@apache.org>]

Rebased to current develop and no longer try to get keeper objects for UDP ports.  Also added better testing of the new behavior.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [x] Does `gradlew build` run cleanly?

- [x] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] WireBaron commented on pull request #2958: GEODE-5674: Stop using random values for test ports

["WireBaron (GitHub)" <gi...@apache.org>]

Yeah, this is definitely wrong, but it's not clear what the right behavior is (well, it should be port % FOO == site, but it's unclear what FOO is).  The bigger problem is that this is long standing behavior (logic copied from AvailablePort.getRandomAvailablePortWithMod) that's called in over 60 places from over a dozen tests.  While the intended protection should be redundant with the new behavior I'm introducing, I think there may be older pieces of code using this that aren't going to be calling initializeUniquePortRange.  I'm not sure whether any of them depend on the current behavior and would become flakier if this was changed to just use a random port.

I would like to change this, but just have no confidence that doing so isn't going to introduce some unintended flakiness somewhere in the test code (particularly worried about hydra).

[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #2958: GEODE-5674: Stop using random values for test ports

["galen-pivotal (GitHub)" <gi...@apache.org>]

This `site` logic looks wrong, and I'm pretty sure that it's legacy from the pre-Geode days. Can we remove it?

[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] WireBaron commented on pull request #2958: GEODE-5674: Stop using random values for test ports

["WireBaron (GitHub)" <gi...@apache.org>]

This function specifically returns a contiguous range of ports.  Not sure why that's necessary, but it is functionality provided by this class, so I'm reluctant to break it.

[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #2958: GEODE-5674: Stop using random values for test ports

["galen-pivotal (GitHub)" <gi...@apache.org>]

Why should we break if one port fails to bind?

[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] WireBaron closed pull request #2958: GEODE-5674: Stop using random values for test ports

["WireBaron (GitHub)" <gi...@apache.org>]

[ pull request closed by WireBaron ]

[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on pull request #2958: GEODE-5674: Stop using random values for test ports

["galen-pivotal (GitHub)" <gi...@apache.org>]

I think I'd prefer to have a fixed limit of, say, 100 ports per site, rather than this logic, which is more complicated and, if it ever breaks, will do so in confusing and unpredictable ways.

[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] WireBaron commented on pull request #2958: GEODE-5674: Stop using random values for test ports

["WireBaron (GitHub)" <gi...@apache.org>]

I agree that this is significantly more complicated, but the benefit is that it gives each host the largest possible range of ports possible, without actually needing to know how many hosts there are or how many ports each one needs.  While it seems generally safe to assume that no test will ever need more than 100 ports, or 20 sites, it isn't necessary to introduce these limits, and it does put us more at risk of failing if we happen to running on machine where some other process took a large chunk of ephemeral address that happens to overlap with our range.

If we do end up in an environment where we do have to spill over into another host's range in order to fill a request we will have an nasty situation where we'll be racing with them in port assignments.  However, in order to end up in such a situation, we either have a crazy number of hosts, or have assigned a crazy number of ports from a particular host (likely both).  In this case, we'd likely have failed far sooner had we tried to impose limits on the number of hosts or ports.

[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] WireBaron commented on issue #2958: GEODE-5674: Stop using random values for test ports

["WireBaron (GitHub)" <gi...@apache.org>]

Talked to Galen and got a sign off on the changes requested, merging this now.

[ Full content available at: https://github.com/apache/geode/pull/2958 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org