You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "WireBaron (GitHub)" <gi...@apache.org> on 2018/11/01 23:32:04 UTC
[GitHub] [geode] WireBaron opened pull request #2764: GEODE-5674:
Stop using random values for test ports
It seems our test framework will fairly frequently have test failing due to
multiple users trying to bind the same port number. This happens when the
different users happen to randomly generate the same port. To keep this from
happening, this change will simply hand out the available ports consecutively
as the test runs. For dunit tests, each child vm will be given a portion of
the overall range for their use.
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?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [ ] 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/2764 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2764:
GEODE-5674: Stop using random values for test ports
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Wouldn't this static block be better as a method? You could still invoke the method from the static block.
Overall, it looks like this class would benefit from changing to be a singleton (I hate to say it) but at least that would be cleaner from a design perspective than having static variables and static blocks.
[ Full content available at: https://github.com/apache/geode/pull/2764 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #2764:
GEODE-5674: Stop using random values for test ports
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
To be clear: this is the only comment whose response is blocking merging this; all the rest of it is basically exposition and I'll merge without it.
[ Full content available at: https://github.com/apache/geode/pull/2764 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #2764:
GEODE-5674: Stop using random values for test ports
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
This looks wrong. I think you want `while (port % MAX_SITES != site)` or so.
[ Full content available at: https://github.com/apache/geode/pull/2764 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] WireBaron commented on issue #2764: GEODE-5674: Stop
using random values for test ports
Posted by "WireBaron (GitHub)" <gi...@apache.org>.
This change will fail any UDP connection as isPortKeepable will always fail on those. That ends up breaking the single point of contact between the AvailablePortHelper and AvailablePort code which was fundamental to this change. This is probably salvageable, but will require a rather fundamental rework. Closing this PR for now.
[ Full content available at: https://github.com/apache/geode/pull/2764 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] WireBaron closed pull request #2764: GEODE-5674:
Stop using random values for test ports
Posted by "WireBaron (GitHub)" <gi...@apache.org>.
[ pull request closed by WireBaron ]
[ Full content available at: https://github.com/apache/geode/pull/2764 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2764:
GEODE-5674: Stop using random values for test ports
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I recommend making these private final. You can still change the values of them in the static block.
[ Full content available at: https://github.com/apache/geode/pull/2764 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] galen-pivotal commented on pull request #2764:
GEODE-5674: Stop using random values for test ports
Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.
All the methods are static, so we could just make the constructor private and keep it a "singleton" that way.
[ Full content available at: https://github.com/apache/geode/pull/2764 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org