You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Andrea Turli <no...@github.com> on 2013/11/11 17:40:00 UTC
[jclouds] fix for JCLOUDS-373 (#199)
You can merge this Pull Request by running:
git pull https://github.com/andreaturli/jclouds fix/JCLOUDS-373
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/199
-- Commit Summary --
* fix for JCLOUDS-373
-- File Changes --
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/VirtualGuestToNodeMetadata.java (18)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/199.patch
https://github.com/jclouds/jclouds/pull/199.diff
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
If there is a specific API call in SoftLayer that sometimes throws that unexpected error, shouldn't it be more appropriate to create a custom ExceptionParser for that API call, and let it transform the api response into the appropriate return value, rather than adding this try/catch everywhere you might get this error?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28217619
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #381](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/381/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28287113
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [1.6.x](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=6e781152954975a8e929576c6e4af49738cf35ea). In order to add this to master too, I think we first need to cherry-pick https://github.com/jclouds/jclouds/pull/77. @demobox?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28286887
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrea Turli <no...@github.com>.
@nacx @ahgittin @demobox I've committed a new version of the code. I'd appreciate to have your comments, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28280360
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #599](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/599/) 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/199#issuecomment-28285941
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #842](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/842/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28281502
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #844 UNSTABLE
Unrelated [test failure](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/org.apache.jclouds$jclouds-compute/844/testReport/junit/org.jclouds.compute.callables/BlockUntilInitScriptStatusIsZeroThenReturnOutputTest/testloopUntilTrueOrThrowCancellationExceptionReturnsWhenPredicateIsTrueSecondTimeWhileNotCancelled/)
> I think we first need to cherry-pick #77
@nacx: See [the comment at #77](https://github.com/jclouds/jclouds/pull/77#issuecomment-28287406)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28287446
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by ahgittin <no...@github.com>.
@andreaturli @nacx given the error which comes back from the server (a weird one, only for a few guests):
jclouds.headers: << HTTP/1.1 500 Internal Server Error
jclouds.wire: << "{"error":"Your order has a duplicate price for category \"Hardware & Software Firewalls\".","code":"SoftLayer_Exception_Order_Item_Duplicate"}"
the softlayer server is behaving badly with this particular guest VM. i don't think we should expect it to sort itself out with a retry. (unless we want to retry until softlayer fix the underlying cause, which could be years!)
so my sense (weakly) is that throwing the exception is the right behaviour for the API method, and for callers to handle as appropriate. in the case here, of getting the image for the purpose of node metadata, catching the exception and returning null seems reasonable -- meaning that the image is not available for some reason).
in short i think andrea's patch is the best solution in this case, unless there is a very different pattern we want to follow (cf `Optional`...).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28224912
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
@andreaturli, no offense, but take your time to check that yourself :)
What you propose would cause every 500 error response in that api call to be captured and transforned to a null return value. Is that the desired behavior? By definition, 500 errors should be server side errors that should disappear after q certain amount of time. In other words, retrying the request should succeed at some point (that is why jclouds provides the default retry handlers for 5xx errors).
This case seems to be a bit different and to require a more custom error handling, to convert the return value to null only if the error is the one you identified, and otherwise propagate the error so the retry logic is executed.
Does this sound good?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28220891
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #839](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/839/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28219879
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrew Phillips <no...@github.com>.
> @@ -64,6 +65,8 @@ public void handleError(HttpCommand command, HttpResponse response) {
> exception = new ResourceNotFoundException(message, exception);
> } else if (message.indexOf("currently an active transaction") != -1) {
> exception = new IllegalStateException(message, exception);
> + } else if (message.indexOf("SoftLayer_Exception_Order_Item_Duplicate") != -1) {
> + exception = new SoftLayerOrderItemDuplicateException(command.getCurrentRequest(), message);
+1 - if the exception is only used to recognize a certain failure state, it doesn't need any more heavyweight information than that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199/files#r7587281
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
LGTM too. Thanks all! Will merge this in a while.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28285159
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #375](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/375/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28218066
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for explaining the details @ahgittin. So is this exception appearing only with a concrete guest, or can it appear quite often? I mean, even if this is an issue in their side, if users are going to see this exception quite often, my preference would be to add a custom fallback to the api method, to mitigate the issue. However, if the exception only appears for concrete guests with a particular configuration, and is unlikely to be a common pattern, then I'm fine with this fix.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28250329
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by ahgittin <no...@github.com>.
2 minor comments, otherwise LGTM
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28217323
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #838](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/838/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28218217
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
@andreaturli, this looks better. A few considerations apart from my last comment:
* Address @demobox comment.
* Properly explain the change in the commit message. [Here](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html) you have a few recommendations.
* Do not squash the commits every time you address the review comments. That makes it difficult to identify the pieces of code that have been changed since last revision.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28281544
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrea Turli <no...@github.com>.
@nacx no offense here :)
Your analysis is correct but I thought that this 500 error response is a bug on the API call (server side) in fact I already tried a retry logic which doesn't seem to solve the issue.
As that getImage method is robust to null object elsewhere I think it is safe to replicate the try/catch idea but using a more jclouds approach (with a Fallback or else).
Does it answer your concern?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28221801
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrea Turli <no...@github.com>.
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.softlayer.exceptions;
> +
> +import org.jclouds.http.HttpRequest;
> +
> +public class SoftLayerOrderItemDuplicateException extends IllegalStateException {
@demobox RuntimeException is better?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199/files#r7587324
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
The 500 responses are handled in the [SoftLayerErrorHandler](https://github.com/jclouds/jclouds/blob/master/providers/softlayer/src/main/java/org/jclouds/softlayer/handlers/SoftLayerErrorHandler.java#L61-L67). Given the json from the wire logs, I think it would be good to also check if the `SoftLayer_Exception_Order_Item_Duplicate` is present to propagate an `IllegalStateException` (or a subclass including the Softlayer error code), which seems to be semantically more appropriate, and catch that exception.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28275510
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by ahgittin <no...@github.com>.
Only seen it once -- which was enough to cause it to crash and burn, but rare. (With the patch I can see it only affects 1 of 116 instances with quite an eclectic variety.) So I don't think it will affect too many users. If we get evidence that it is a widespread issue then I agree the jclouds API and/or fallbacks should be revisited.
That's far from being the biggest problem with the softlayer provider however! The fact that it is working reveals another big flaw: it takes more than 10m just to list the nodes! Seems to be because it makes several requests for every single guest VM to populate details. A cache (e.g. of product types) would speed that up enormously. Or having an option to `listNodes` with very sparse details -- e.g. to provision a machine it currently does `listNodes()` in `CreateNodesWithGroupEncodedIntoNameThenAddToSet.java` which goes and makes lots of requests to get all the details even though it only needs the names!
However: with this patch it is working. Great effort folks.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28256606
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrew Phillips <no...@github.com>.
@andreaturli: thanks! New exception type works for me. @nacx: Still thoughts on passing the whole message to the exception?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28284664
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by ahgittin <no...@github.com>.
> // 'bad' orders have no start cpu's and cause the order lookup to fail.
> if (guest.getStartCpus() < 1)
> return null;
> - ProductOrder order = client.getVirtualGuestClient().getOrderTemplate(guest.getId());
> - if (order == null)
> + try {
> + order = client.getVirtualGuestClient().getOrderTemplate(guest.getId());
> + } catch (RuntimeException e) {
> + logger.warn("Cannot getOrderTemplate " + guest.getId(), e);
> + return null;
> + }
> + if (order == null)
indentation is off on this line
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199/files#r7565035
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
@andreaturli Mind squashing the commits into a single one that properly references the issue and describes the fix? Thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28285210
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #376](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/376/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28219875
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #379](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/379/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28281481
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrea Turli <no...@github.com>.
hi @ahgittin @nacx @demobox thanks for your valuable feedbacks!
Here is my proposal, following your advices:
```java
try {
order = client.getVirtualGuestClient().getOrderTemplate(guest.getId());
} catch (HttpResponseException e) {
// this is a workaround because SoftLayer throws somethimes 500 internal server errors for the above method call
String message = "{\"error\":\"Your order has a duplicate price for category \\\"Hardware & Software Firewalls\\\".\",\"code\":\"SoftLayer_Exception_Order_Item_Duplicate\"}";
if(e.getContent().equals(message)) {
logger.warn("Cannot get order template for virtualGuest id(" + guest.getId() + "), because of " + message, e);
return null;
}
}
```
wdyt?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28275440
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrea Turli <no...@github.com>.
@ahgittin thanks for your review: your comments have been addressed.
@nacx that API call already defines a ```@Fallback(NullOnNotFoundOr404.class)```
is there anything like ```@Fallback(NullOnNotFoundOr500.class)``` ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28219424
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #843](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/843/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28283892
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by ahgittin <no...@github.com>.
@demobox I was wondering about that. Since the behaviour of that call is not crucial to this method, and there may be other gremlins lurking in softlayer (other instances where that command fails), logging a warning seems safer than rethrowing other *server errors*.
But I think you are right, if there is a neat way to check that we catch only exceptions due to HTTP code 500 that would be good. (Not sure whether that info is readily available here though ... @andreaturli ? )
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28259836
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -64,6 +65,8 @@ public void handleError(HttpCommand command, HttpResponse response) {
> exception = new ResourceNotFoundException(message, exception);
> } else if (message.indexOf("currently an active transaction") != -1) {
> exception = new IllegalStateException(message, exception);
> + } else if (message.indexOf("SoftLayer_Exception_Order_Item_Duplicate") != -1) {
> + exception = new SoftLayerOrderItemDuplicateException(command.getCurrentRequest(), message);
I think there is no point in providing the entire json string in a custom field of your exception. If the json error is not parsed, and its fields can not be easily accessed by users, then there is no point to store it in dedicated fields. I'd remove the fields of the custom exception class, and just propagate the `HttpResponseException`:
```java
new SoftLayerOrderItemDuplicateException(message, exception);
```
Or you can parse the json (using the jclouds json integration is just a one liner), and properly populate the error message and error code in the exception class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199/files#r7587104
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by ahgittin <no...@github.com>.
> // 'bad' orders have no start cpu's and cause the order lookup to fail.
> if (guest.getStartCpus() < 1)
> return null;
> - ProductOrder order = client.getVirtualGuestClient().getOrderTemplate(guest.getId());
> - if (order == null)
> + try {
> + order = client.getVirtualGuestClient().getOrderTemplate(guest.getId());
> + } catch (RuntimeException e) {
> + logger.warn("Cannot getOrderTemplate " + guest.getId(), e);
comment that SL sometimes throws errors for the above method call, which it appears there is no way around. ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199/files#r7565019
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #844](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/844/) 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/199#issuecomment-28287094
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #380](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/380/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28283826
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrew Phillips <no...@github.com>.
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.softlayer.exceptions;
> +
> +import org.jclouds.http.HttpRequest;
> +
> +public class SoftLayerOrderItemDuplicateException extends IllegalStateException {
No need to extend IllegalStateException? That normally indicates an illegal state _in this code_, rather than on the server?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199/files#r7587292
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrea Turli <no...@github.com>.
OK @nacx, done.
Thanks all!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199#issuecomment-28285876
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by Andrew Phillips <no...@github.com>.
> // 'bad' orders have no start cpu's and cause the order lookup to fail.
> if (guest.getStartCpus() < 1)
> return null;
> - ProductOrder order = client.getVirtualGuestClient().getOrderTemplate(guest.getId());
> + try {
> + order = client.getVirtualGuestClient().getOrderTemplate(guest.getId());
> + } catch (RuntimeException e) {
> + // this is a workaround because SoftLayer throws somethimes 500 internal server errors for the above method call
> + logger.warn("Cannot getOrderTemplate " + guest.getId(), e);
> + return null;
Since order == null triggers a return just one command later, make this `order = null` instead? This might also make it clearer that `null` is indeed a possible "normal" response for getOrderTemplate, even if no exception is thrown?
To @nacx's comments: is there some way we can at least verify that `e` is the "expected" RuntimeException here? Can we
* catch a narrower category or exceptions and
* check for the code, body or message somehow
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/199/files#r7579866
Re: [jclouds] fix for JCLOUDS-373 (#199)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #598](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/598/) 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/199#issuecomment-28283242