You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "jujoramos (GitHub)" <gi...@apache.org> on 2018/10/15 12:51:23 UTC
[GitHub] [geode] jujoramos opened pull request #2604: GEODE-5739: Use
Launchers in StarterRules
GEODE-5739: Use Launchers in StarterRules
Rules ServerStarterRule and LocatorStarterRule now use LocatorLauncher
and ServerLauncher classes to start the members instead of the internal
API.
- Changed ServerStarterRule to use ServerLauncher.
- Changed LocatorStarterRule to use LocatorLauncher.
- Revisited tests using the changed rules, fixed some warnings,
eliminated duplicated code and replaced the usage of `junit.Assert`
by `assertj`.
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`)?
- [X] 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/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on issue #2604: GEODE-5739:
Use Launchers in StarterRules
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
I would agree with @jinmeiliao - there really isn't any advantage to switching out how the rule works internally. We're not experiencing any issues with the rule itself. I'd vote for just closing the Jira.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
@jujoramos Let me put my comment in the ticket. I don't think there is much point in doing this.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pdxrunner commented on issue #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "pdxrunner (GitHub)" <gi...@apache.org>.
I agree with @jdeppe-pivotal and @jinmeiliao. Injecting a dependency on the launchers into all tests that use {{ServerStarterRule}} and {{LocatorStarterRule}} will only serve to further separate the test code from the code under test, and seems counterproductive.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on issue #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
@jinmeiliao, @jhuynh1: no need to review yet, I've found the reason for the failures in `ClusterStartupRuleCanSpecifyOlderVersionsDUnitTest` but I'm not exactly sure which approach should be used to solve the problems. I've sent an email to dev@geode.apache.org with more details.
Cheers.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on issue #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
Hello all,
Thanks for the updates.
I'm not sure how these things are handled in the community, should I just close this pull request and the associated JIRA as `won't solve` or send an email to the dev list for further discussion?. Thoughts?.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on issue #2604: GEODE-5739:
Use Launchers in StarterRules
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
There seems to be a lot of code-cleanup in your diff in files that aren't otherwise touched. You'll get some conflicting opinions about whether or not code-cleanup on the side should be included in a ticket that isn't explicitly addressing code-cleanup, but I personally try to limit it to files I'm touching anyway. If you've found that your cleanup has spread out in scope, open a PR just for that.
The reason being here that it is now more difficult for me to review this PR, having to scroll through many low-impact improvements to find the actual scope of the ticket that is meaningfully distinct.
You'll also get conflicting opinions on this front as well, but I think it also helps to keep these sorts of things as separate commits that are squashed before merging, especially now that we have that nice `Squash and merge` button. That way, I can sift out the low-impact commits like `prefer assertThat().isNull() over assertNotNull`, and have the actual bulk of the PR in a commit like `Have StarterRules use Launcher classes` available at my fingertips.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
-1
When I first started these rules, I want the rule to use the api as close to the core as possible, so that we don't end up testing the launcher as well. We use the rules to test the functionality the developer is interested in, do not want to get launcher code (which seemed very messy at that time, probably still still is) in the way.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on issue #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
@jdeppe-pivotal, @jinmeiliao, @PurelyApplied:
Thanks for reviewing, I agree with your comments and it's clear that this PR way too big, full of unrelated changes (improvements to the test source code, but unrelated nevertheless)... by the time I realized how many files I've changed it was too late and I didn't want to do all over again, so I'll follow @PurelyApplied's comments and split the current PR in two commits directly: the first one with the actual changes and the second one with the cleanup/refactoring.
Either way, I'm still unsure how to proceed regarding the `ClusterStartupRule`: it will fail after my changes whenever a user tries to startup a member using an older version, basically because some methods are not public in older releases (`AbstractLauncher.getLogFile()`) and others are not even there (`XXXXStarter.setDeletePidFileOnStop()`). I've already sent an email to the dev list, would you mind sharing your thoughts on that?.
@jinmeiliao: I've started working on this because I thought there was enough consensus within the team about the changes, from the ticket:
> In a review, it was mentioned from that we can change ServerStarterRule and LocatorStarterRule to use ServerLauncher and LocatorLauncher. This way the rules are using the User API directly.
I agree with the statement above, we won't be testing the Launchers all over again, but us them as any user would instead. Do you think otherwise?, should I just close the `pull request` and delete the GEODE ticket all together?.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos closed pull request #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
[ pull request closed by jujoramos ]
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
@jujoramos you can do both or either. Thanks!
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jdeppe-pivotal commented on issue #2604: GEODE-5739:
Use Launchers in StarterRules
Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.
There seem to be a lot of changes here that are not really relevant to the actual ticket but more just cleanup/refactoring. Please can you break these up into more specific (and smaller) PRs.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on issue #2604: GEODE-5739: Use
Launchers in StarterRules
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
@jdeppe-pivotal, @jinmeiliao, @PurelyApplied:
Thanks for reviewing, I agree with your comments and it's clear that length of this PR is full of unrelated changes, improvements to the test source code, but unrelated in the end... by the time I realized how many files I've changed it was too late and I didn't want to do all over again, so I'll follow @PurelyApplied's comments and split the current PR in two commits directly: the first one with the actual changes and the second one with the cleanup/refactoring.
Either way, I'm still unsure how to proceed regarding the `ClusterStartupRule`: it will fail after my changes whenever a user tries to startup a member using an older version, basically because some methods are not public in older releases (`AbstractLauncher.getLogFile()`) and others are not even there (`XXXXStarter.setDeletePidFileOnStop()`). I've already sent an email to the dev list, would you mind sharing your thoughts on that?.
@jinmeiliao: I've started working on this because I thought there was enough consensus within the team about the changes, from the ticket:
> In a review, it was mentioned from that we can change ServerStarterRule and LocatorStarterRule to use ServerLauncher and LocatorLauncher. This way the rules are using the User API directly.
I agree with the statement above, we won't be testing the Launchers all over again, but us them as any user would instead. Do you think otherwise?, should I just close the `pull request` and delete the GEODE ticket all together?.
[ Full content available at: https://github.com/apache/geode/pull/2604 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org