cloudsoft / jclouds-vcloud-director

0 stars 9 forks source link

ISSUE #28: Retry on OPERATION_LIMITS_EXCEEDED #41

Closed aledsage closed 8 years ago

aledsage commented 8 years ago

Note that to get the live tests working for me, I needed to do a few hacks (see https://github.com/cloudsoft/jclouds-vcloud-director/compare/1.9.x...live-test-hacks?expand=1). I have not included them in this PR - was it perhaps because of my system properties I use for testing? How do other people run the live tests?

andreaturli commented 8 years ago

Great work @aledsage

Minor suggestion about the overall implementation: have a look at https://github.com/jclouds/jclouds/blob/master/providers/digitalocean2/src/main/java/org/jclouds/digitalocean2/handlers/DigitalOcean2ErrorHandler.java I think following that recommended patternfor error handling on VCDErrorHandler could simplify a bit the rest of your patch.

aledsage commented 8 years ago

@andreaturli I'm very tempted to simplify the retry-handler code to be more like DigitalOcean2ErrorHandler.java, but on balance I think probably not. The reasons are:

Thoughts?

Note the DigitalOcean rate limit handler is entirely different impl, because the response explicitly says how long one should wait before retrying (so that pattern isn't really helpful): https://github.com/jclouds/jclouds/blob/master/providers/digitalocean2/src/main/java/org/jclouds/digitalocean2/handlers/RateLimitRetryHandler.java

For deleting VcloudDirectorServerErrorRetryHandler, I'll do that. I had wondered about leaving it in so that one could override the config that said what error codes to retry on - but given I didn't bother implementing it in VcloudDirectorServerErrorRetryHandler, then definitely better that we delete the code.

@bostko I'll change the injected constructor to be protected.

aledsage commented 8 years ago

@bostko @andreaturli I've incorporated those review comments. If you're happy for this to be merged, then let me know. I can squash the commits and merge (or you can!).

bostko commented 8 years ago

It may need a logger to notify when retry happened

bostko commented 8 years ago

@aledsage I added a logger locally on top of your latest code but I saw that jclouds is logging the client retries. I tested it and I see messages like Retry 1/6: delaying for 104 ms: server error. So the fix is working fine.

aledsage commented 8 years ago

Tested by @bostko - thanks. Merging now.