You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "mhansonp (GitHub)" <gi...@apache.org> on 2018/09/15 00:42:49 UTC
[GitHub] [geode] mhansonp opened pull request #2482: GEODE-369: wait
for primary to be available when going from 0 to 1 server
Co-Authored-By: Bill Burcham <bb...@pivotal.io>
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:
- [ ] 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`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] 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/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp closed pull request #2482: GEODE-369: wait
for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
[ pull request closed by mhansonp ]
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I recommend deleting old author names from comments.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "Bill (GitHub)" <gi...@apache.org>.
Should throw `IllegalArgumentException` if `connectedServers < 1` or `redundantServers < 0`
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] Bill commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "Bill (GitHub)" <gi...@apache.org>.
This should say "1 or more". The big learning here was that it's not possible to test for zero connected servers via the API (calling through the API in that case results in the `NoSubscriptionServersAvailableException` exception).
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "kirklund (GitHub)" <gi...@apache.org>.
You might want to crank up the timeouts to FIVE MINUTES. I know it seems like a lot, but many of our flaky tests are caused by overloaded CI machine that results in some pauses in the test that surpass 1 or 2 minutes.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund closed pull request #2482: GEODE-369: wait
for primary to be available when going from 0 to 1 server
Posted by "kirklund (GitHub)" <gi...@apache.org>.
[ pull request closed by kirklund ]
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on issue #2482: GEODE-369: wait
for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
@upthewaterspout @WireBaron @balesh2 @kirklund @pdxrunner Can you take a look at this PR?
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Since you're changing so much, I'll mention this as a good thing to also do. Any time you see a test creating a new CacheServer and returning the server.getPort(), delete the use of getRandomAvailablePort, change it to setPort(0) and then return that port. It'll find a randomly available port on its own and return that. There's a tiny race condition when using getRandomAvailablePort which can cause a test to fail because another process grabbed that port out from under you.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "kirklund (GitHub)" <gi...@apache.org>.
You might want to rename all var names.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I would delete all usage of pollInterval and then crank up the atMost to FIVE MINUTES.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I would delete all System.out.println statements.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on issue #2482: GEODE-369: wait
for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
@upthewaterspout @kirklund I have reviewed Kirk's concerns and addressed them I believe. The failure in Build was in an unrelated are of the code. Please merge if you get the opportunity.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
Here is the important change for the primary problem. Elsewhere we cleaned up and normalized most of the timeouts to 3 minutes each, and used Awaitility.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I typically remove all usage of deprecated code. You can change this to:
```java
cache = new CacheFactory(props).create();
```
And delete assertThat(cache).isNotNull(); because it cannot be null unless it threw a Throwable.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
sometimes, I don't want the system to poll more frequently than specified. Thats why I use pollInterval. Do you think that is a bad approach? I thought the default was 100ms
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
shifting to get from get entry was the fix here. The rest is refactoring.
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mhansonp commented on pull request #2482: GEODE-369:
wait for primary to be available when going from 0 to 1 server
Posted by "mhansonp (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2482 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org