You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Christopher Dancy <no...@github.com> on 2014/06/23 16:01:24 UTC
[jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
This is the PR for this JIRA issue:
https://issues.apache.org/jira/browse/JCLOUDS-610
New tests have not been written as there are already the tests "testReboot" and "testSuspendResume" of class "BaseComputeServiceLiveTest" which already cover these calls. I have however refactored these testers to account for the changes made.
You can merge this Pull Request by running:
git pull https://github.com/cdancy/jclouds refactor-matching-methods
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/419
-- Commit Summary --
* NodePredicates gained static predicate SUSPENDED
* Merge branch 'master' of https://github.com/cdancy/jclouds
* Methods: resumeNodesMatching, suspendNodesMatching, and rebootNodesMatching have been refactored to match the functionality provided by method destroyNodesMatching.
-- File Changes --
M compute/src/main/java/org/jclouds/compute/ComputeService.java (21)
M compute/src/main/java/org/jclouds/compute/internal/BaseComputeService.java (102)
M compute/src/test/java/org/jclouds/compute/internal/BaseComputeServiceLiveTest.java (26)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/419.patch
https://github.com/jclouds/jclouds/pull/419.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> Are you still against explicitly declaring we throw "TransformParallelException" in ComputeService?
Personally, yes. It's a very implementation-specific exception that doesn't belong in the interface, if you ask me - it has nothing to do with the "public contract" of the method, in my view.
Will be interested to hear what others have to say.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47017606
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
Just to close the loop on porting back ... change is backwards compatible in that it does not break/change existing API's only what they return. Since previous users expected nothing to come back they can happily go along thinking so ;)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47175299
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Ignasi Barrera <no...@github.com>.
Are your messages being rejected? Try to send them as plain text, not html; I hope that will work.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47388427
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> @@ -423,24 +422,35 @@ public void rebootNode(String id) {
> boolean successful = nodeRunning.apply(node);
> logger.debug("<< rebooted node(%s) success(%s)", id, successful);
> }
> -
> +
[minor] remove this change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14126396
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> change is backwards compatible in that it does not break/change existing API's only what they return
At the source level, it should be. As far as I can make out, though, at the binary level this counts as a backwards-incompatible change:
"Changing the result type of a method, or replacing a result type with void, or replacing void with a result type, has the combined effect of deleting the old method and adding a new method with the new result type or newly void result"
([JLS 13.4.15](http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html)).
I'm not sure what the intended policy around avoiding backwards incompatible changes in minor versions. If we are looking for binary-level compatibility between 1.7.3 and 1.7.4, for example, it would seem (based on the JLS excerpt) that this could break that :-(
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47175984
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
Yea I see your point. However I don't actually think anyone has been using these methods anyway due to a bug I found here:
https://issues.apache.org/jira/browse/JCLOUDS-594
which @ccustine has fixed here:
https://github.com/jclouds/jclouds/pull/403
so with that in mind I wonder if it Ok to merge the functionality.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47176765
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1280](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1280/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47032661
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> We may want to fix karat first?
Indeed. About to push a branch and open a PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47163004
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Everett Toews <no...@github.com>.
> * @throws UnsupportedOperationException
> * if the underlying provider doesn't support suspend/resume
> * @throws NoSuchElementException
> * if no nodes matched the predicate specified
> + * @throws TransformParallelException
I agree with @demobox that it's an impl detail but it is behaviour. TransformParallelException isn't actually being thrown here, it's just being documented. catch-and-rethrow inside the implementation doesn't seem appropriate because TransformParallelException is really just a collection of Exceptions. Sure you could catch-and-rethrow but you wouldn't be adding anything of value by doing that.
@demobox If this is how this method behaves, why wouldn't we want to document it as such?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14148861
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
Did this PR get merged? I ask only because the last github message here is noting there are unmerged commits.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-48326473
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
> * @throws UnsupportedOperationException
> * if the underlying provider doesn't support suspend/resume
> * @throws NoSuchElementException
> * if no nodes matched the predicate specified
> + * @throws TransformParallelException
Is it specific? The call to 'ImmutableSet.copyOf(transformParallel(*)' is going to throw a TransformParallelException should any of the calls/nodes fail. It's easy enough to remove though in testing that is what popped the most so I thought it pertinent to add to the interface.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14127790
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #1402 UNSTABLE
[Spurious test failure](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/org.apache.jclouds$jclouds-compute/1402/testReport/junit/org.jclouds.compute.callables/BlockUntilInitScriptStatusIsZeroThenReturnOutputTest/testloopUntilTrueOrThrowCancellationExceptionReturnsWhenPredicateIsTrueSecondTimeWhileNotCancelled/).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47028402
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
> @@ -423,24 +422,35 @@ public void rebootNode(String id) {
> boolean successful = nodeRunning.apply(node);
> logger.debug("<< rebooted node(%s) success(%s)", id, successful);
> }
> -
> +
I removed this package import as it was no longer being used. Should I add it back?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14127871
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> +
> + // TODO make an async interface instead of re-wrapping
> + @Override
> + public ListenableFuture<NodeMetadata> apply(final NodeMetadata from) {
> + return userExecutor.submit(new Callable<NodeMetadata>() {
> + public NodeMetadata call() throws Exception {
> + suspendNode(from.getId());
> + return from;
> + }
> + public String toString() {
> + return "suspendNode(" + from.getId() + ")";
> + }
> + });
> + }
> +
> + }, userExecutor, null, logger, "suspendNodesMatching(" + filter + ")"));
Please fix indenting and rename to `suspendedNodes`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141489
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> @@ -548,16 +548,28 @@ protected void assertNodeZero(Collection<? extends NodeMetadata> metadataSet) {
> }
>
> @Test(enabled = true, dependsOnMethods = "testGet")
> - public void testReboot() throws Exception {
> - client.rebootNodesMatching(inGroup(group));// TODO test
> + public void testReboot() throws Exception {
> + Set<? extends NodeMetadata> rebootNodes = client.rebootNodesMatching(inGroup(group));
> + for (ComputeMetadata node : rebootNodes) {
> + assert node.getProviderId() != null : node;
> + assert node.getLocation() != null : node;
Please use `assertNotNull` from TestNG and fix indents here
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141526
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Chris Custine <no...@github.com>.
I figured it out, my response should finally be posted.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47414666
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
Looks like there is an issue in trying to post to the dev mailing list
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47385124
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1269](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1269/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-46855578
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
Do we want to/can we backport this to 1.7.x, or is this a backwards-incompatible change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47161034
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
Merged to [1.7.x](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;h=7f28453) (and the [karaf fix](https://git-wip-us.apache.org/repos/asf?p=jclouds-karaf.git;a=commit;h=ca038e938501c242de53ffeb9409407c43a2109e), too).
Thanks, @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47676045
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
Yea that's true. I'd be interested to hear what others have to say.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47247892
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
> + new Function<NodeMetadata, ListenableFuture<? extends NodeMetadata>>() {
> +
> + // TODO make an async interface instead of re-wrapping
> + @Override
> + public ListenableFuture<NodeMetadata> apply(final NodeMetadata from) {
> + return userExecutor.submit(new Callable<NodeMetadata>() {
> + public NodeMetadata call() throws Exception {
> + rebootNode(from.getId());
> + return from;
> + }
> + public String toString() {
> + return "rebootNode(" + from.getId() + ")";
> + }
> + });
> + }
> +
Fixing now ...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14144339
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #922](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/922/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-46856021
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1279](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1279/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47032633
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #931](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/931/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47027858
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
Pending the PR builders, +1 - looks good to me. Thanks for the updates, @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47017965
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
> logger.debug(">> rebooting nodes matching(%s)", filter);
> - transformParallel(nodesMatchingFilterAndNotTerminatedExceptionIfNotFound(filter),
> - new Function<NodeMetadata, ListenableFuture<? extends Void>>() {
> - // TODO use native async
> - @Override
> - public ListenableFuture<Void> apply(NodeMetadata from) {
> - rebootNode(from.getId());
> - return immediateFuture(null);
> - }
> -
> - }, userExecutor, null, logger, "rebootNodesMatching(" + filter + ")");
> + Set<NodeMetadata> set = ImmutableSet.copyOf(transformParallel(nodesMatchingFilterAndNotTerminatedExceptionIfNotFound(filter),
> + new Function<NodeMetadata, ListenableFuture<? extends NodeMetadata>>() {
The idea for this was copied from the method "destroyNodesMatching" which does exactly the same thing. Is there a better way to go about this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14142345
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> * @throws UnsupportedOperationException
> * if the underlying provider doesn't support suspend/resume
> * @throws NoSuchElementException
> * if no nodes matched the predicate specified
> + * @throws TransformParallelException
This is a bit of weird exception to be throwing at the interface - the fact that a parallel transform is happening is very implementation specific. Do we need to throw anything here? Can we catch-and-rethrow inside the implementation?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14126351
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
It does and thank you ;)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-48329444
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> + new Function<NodeMetadata, ListenableFuture<? extends NodeMetadata>>() {
> +
> + // TODO make an async interface instead of re-wrapping
> + @Override
> + public ListenableFuture<NodeMetadata> apply(final NodeMetadata from) {
> + return userExecutor.submit(new Callable<NodeMetadata>() {
> + public NodeMetadata call() throws Exception {
> + rebootNode(from.getId());
> + return from;
> + }
> + public String toString() {
> + return "rebootNode(" + from.getId() + ")";
> + }
> + });
> + }
> +
Please use 3-space indenting here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141364
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1393](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1393/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-46852146
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
@demobox to your last point: yea something like that would be perfect. I'll make a mental note to go back at some point and make note of such things inside code in a separate PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47027716
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Ignasi Barrera <no...@github.com>.
We may want to fix karat first? See https://github.com/jclouds/jclouds-karaf/pull/47#issuecomment-47153893.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47161380
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
@ccustine is noting he tried and they are being returned ...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47406944
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #923](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/923/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-46856019
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
> +
> + // TODO make an async interface instead of re-wrapping
> + @Override
> + public ListenableFuture<NodeMetadata> apply(final NodeMetadata from) {
> + return userExecutor.submit(new Callable<NodeMetadata>() {
> + public NodeMetadata call() throws Exception {
> + suspendNode(from.getId());
> + return from;
> + }
> + public String toString() {
> + return "suspendNode(" + from.getId() + ")";
> + }
> + });
> + }
> +
> + }, userExecutor, null, logger, "suspendNodesMatching(" + filter + ")"));
Ahhhh missed this. Fixing now ...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14142371
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> but some documentation as to say what could possibly be thrown from method a,b,c is I guess what
> I was looking and/or advocating for.
I can see your point, and in this case since we weren't actually adding it to the method signature it wouldn't have caused any issues for consumers. But writing it "officially" as a `@throws` seems to me a bit like indicating that a method that accesses a list somewhere in the implementation could throw an `IndexOutOfBoundsException`.
Also, I would be more tempted to add a note about this kind of possibility if we felt that there was something users could feasibly do in that case, e.g. "Implementation note: may throw a TransformParallelException. This can occur when X, Y and Z. Callers can catch this exception and do A, B and C to handle this."
Thanks for the squash'n'rebase!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47022429
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #929](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/929/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47019947
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #930](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/930/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47023822
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1394](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1394/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-46856249
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> @@ -572,7 +584,13 @@ public boolean apply(NodeMetadata input) {
>
> }) : stoppedNodes;
>
> - client.resumeNodesMatching(inGroup(group));
> + Set<? extends NodeMetadata> resumedNodes = client.resumeNodesMatching(inGroup(group));
> + for (ComputeMetadata node : resumedNodes) {
> + assert node.getProviderId() != null : node;
> + assert node.getLocation() != null : node;
> + assertEquals(node.getType(), ComputeType.NODE);
Please fix indents and use assertNotNull
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141565
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1401](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1401/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47023895
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> logger.debug(">> rebooting nodes matching(%s)", filter);
> - transformParallel(nodesMatchingFilterAndNotTerminatedExceptionIfNotFound(filter),
> - new Function<NodeMetadata, ListenableFuture<? extends Void>>() {
> - // TODO use native async
> - @Override
> - public ListenableFuture<Void> apply(NodeMetadata from) {
> - rebootNode(from.getId());
> - return immediateFuture(null);
> - }
> -
> - }, userExecutor, null, logger, "rebootNodesMatching(" + filter + ")");
> + Set<NodeMetadata> set = ImmutableSet.copyOf(transformParallel(nodesMatchingFilterAndNotTerminatedExceptionIfNotFound(filter),
> + new Function<NodeMetadata, ListenableFuture<? extends NodeMetadata>>() {
Ooo...eeek. What is this "dummy" function at the end here - is that just an evil way of calling `rebootNode` on all the matching nodes in parallel? If so, please add a comment?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141309
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1270](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1270/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-46863045
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
1.) Let me see what I can put together for better testing.
2.) I actually don't think that call is necessary and will remove. The implementation was copied over from method "destroyNodesMatching".
3.) Are you still against explicitly declaring we throw "TransformParallelException" in ComputeService? I can remove but it seems, at least with any of the "*Matching()" methods, that is the exception thrown so as to capture which nodes were successful and which failed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47005586
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
squash/rebase done
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47021203
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1402](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1402/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47028114
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
>
> + Set<? extends NodeMetadata> suspendedNodes = client.suspendNodesMatching(inGroup(group));
> + for (ComputeMetadata node : suspendedNodes) {
> + assert node.getProviderId() != null : node;
Please fix indents and use assertNotNull
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141559
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> + new Function<NodeMetadata, ListenableFuture<? extends NodeMetadata>>() {
> +
> + // TODO make an async interface instead of re-wrapping
> + @Override
> + public ListenableFuture<NodeMetadata> apply(final NodeMetadata from) {
> + return userExecutor.submit(new Callable<NodeMetadata>() {
> + public NodeMetadata call() throws Exception {
> + resumeNode(from.getId());
> + return from;
> + }
> + public String toString() {
> + return "resumeNode(" + from.getId() + ")";
> + }
> + });
> + }
> +
Please fix indenting and rename the set to `resumedNodes`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141461
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Christopher Dancy <no...@github.com>.
No I think in the end you're right. I guess from my POV I was expecting to see it being declared as being thrown from somewhere. Digging through I see it being thrown from inside FutureIterables.transformParallel, and I do understand it's a RuntimeException, but some documentation as to say what could possibly be thrown from method a,b,c is I guess what I was looking and/or advocating for.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47019097
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Ignasi Barrera <no...@github.com>.
Yes it is. You'll find it if you look at the history.
When you rebase to the latest master, the commit hash changes, and that's why GitHub does not detect the PR as merged. Usually we ask contributors to rebase the branch before merging, but sometimes is just more agile to rebase to the latest version ourselves before merging and just close the PR leaving a comment. Hope this answers your question! :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-48329354
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Ignasi Barrera <no...@github.com>.
>Yea I see your point. However I don't actually think anyone has been using these methods anyway due to a bug I found here
That only affects the OpenStack providers, but there are many others in jclouds. People like me) are already using those methods without issues.
Regarding the backward compatibility thing, I don't think there is a need to be compatible at binary level, but this might be something to discuss in the dev@ mailing list.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47199784
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
Just to summarize this: are we thus OK with backporting this to 1.7.x?
/cc @everett-toews @nacx?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47672386
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Ignasi Barrera <no...@github.com>.
I'm +1 on backporting to 1.7.x, as this is the consensus in the dev@ mailing list thread.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47672693
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1400](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1400/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47020112
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
Committed to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;h=8598ee8). Thanks, @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47029964
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
Closed #419.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#event-134752995
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> * @throws UnsupportedOperationException
> * if the underlying provider doesn't support suspend/resume
> * @throws NoSuchElementException
> * if no nodes matched the predicate specified
> + * @throws TransformParallelException
See comment above - here and in the subsequent calls
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14126382
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
>
> + Set<? extends NodeMetadata> suspendedNodes = client.suspendNodesMatching(inGroup(group));
Please fix indenting
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141542
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
Two questions:
* do we need some unit tests to verify that the _correct_ nodes are returned each time? The current tests only seem to check for some non-null properties
* we're adding a call to `cleanUpIncidentalResourcesOfDeadNodes`, it seems. Verify this using a test?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-47000329
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> I ask only because the last github message here is noting there are unmerged commits.
We also try to link to the commits in the ["merged" message](https://github.com/jclouds/jclouds/pull/419#issuecomment-47676045)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419#issuecomment-48518549
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> @@ -423,24 +422,35 @@ public void rebootNode(String id) {
> boolean successful = nodeRunning.apply(node);
> logger.debug("<< rebooted node(%s) success(%s)", id, successful);
> }
> -
> +
> Should I add it back?
No, sorry, my comment wasn't clear: the change looked like it was adding some unnecessary whitespace, that was all.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14149139
Re: [jclouds] JCLOUDS-610: reboot, suspend, and resumeNodesMatching
methods gain functionality similar to that of destroyNodesMatching (#419)
Posted by Andrew Phillips <no...@github.com>.
> logger.debug(">> rebooting nodes matching(%s)", filter);
> - transformParallel(nodesMatchingFilterAndNotTerminatedExceptionIfNotFound(filter),
> - new Function<NodeMetadata, ListenableFuture<? extends Void>>() {
> - // TODO use native async
> - @Override
> - public ListenableFuture<Void> apply(NodeMetadata from) {
> - rebootNode(from.getId());
> - return immediateFuture(null);
> - }
> -
> - }, userExecutor, null, logger, "rebootNodesMatching(" + filter + ")");
> + Set<NodeMetadata> set = ImmutableSet.copyOf(transformParallel(nodesMatchingFilterAndNotTerminatedExceptionIfNotFound(filter),
Rename the set to `rebootedNodes` or so?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/419/files#r14141322