You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by ahgittin <no...@github.com> on 2013/11/11 15:45:35 UTC
[jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
You can merge this Pull Request by running:
git pull https://github.com/ahgittin/jclouds-labs-google fix/jclouds-372
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-google/pull/14
-- Commit Summary --
* fix for JCLOUDS-372, changing MINUTES to SECONDS
-- File Changes --
M oauth/src/main/java/org/jclouds/oauth/v2/config/OAuthModule.java (9)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/14.patch
https://github.com/jclouds/jclouds-labs-google/pull/14.diff
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by Andrew Phillips <no...@github.com>.
> @@ -66,8 +66,11 @@ protected void configure() {
> }
>
> /**
> - * Provides a cache for tokens. Cache is time based and expires after 59 minutes (the maximum time a token is
> - * valid is 60 minutes)
> + * Provides a cache for tokens. Cache is time based and by default expires after 59 minutes
> + * (the maximum time a token is valid is 60 minutes).
> + * This cache and expiry period is system-wide and does not attend to per-instance expiry time
> + * (e.g. "expires_in" from Google Compute -- which is set to the standard 3600 seconds).
> + * If per-instance expiry time is required, refactoring will be needed.
Odd comment for Javadoc...move the last line to an implementation comment?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14/files#r7587531
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by Andrew Phillips <no...@github.com>.
Please submit a PR for 1.6.x if we want to backport this...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14#issuecomment-28285233
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by Andrew Phillips <no...@github.com>.
> @@ -78,7 +81,7 @@ protected void configure() {
> // bit before the deadline to make sure there aren't session expiration exceptions
> sessionIntervalInSeconds = sessionIntervalInSeconds > 30 ? sessionIntervalInSeconds - 30 :
> sessionIntervalInSeconds;
> - return CacheBuilder.newBuilder().expireAfterWrite(sessionIntervalInSeconds, TimeUnit.MINUTES).build(CacheLoader
> + return CacheBuilder.newBuilder().expireAfterWrite(sessionIntervalInSeconds, TimeUnit.SECONDS).build(CacheLoader
Not sure I'm getting this: in the updated Javadoc, we explicitly state that the time configured _here_ is **not** related to the `expires_in` parameter (which, as per documentation, is in seconds).
Is the change to seconds here motivated by that piece of Google documentation? Or do we think `sessionIntervalInSeconds, TimeUnit.MINUTES` is simply a bug, which is indeed what it looks like, from the variable names, at least?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14/files#r7587601
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by ahgittin <no...@github.com>.
thanks @demobox - opened https://github.com/jclouds/jclouds-labs/pull/34 for 1.6.x
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14#issuecomment-28312376
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by ahgittin <no...@github.com>.
@demobox moved the impl detail comment out of javadoc
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14#issuecomment-28284303
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by ahgittin <no...@github.com>.
> @@ -78,7 +81,7 @@ protected void configure() {
> // bit before the deadline to make sure there aren't session expiration exceptions
> sessionIntervalInSeconds = sessionIntervalInSeconds > 30 ? sessionIntervalInSeconds - 30 :
> sessionIntervalInSeconds;
> - return CacheBuilder.newBuilder().expireAfterWrite(sessionIntervalInSeconds, TimeUnit.MINUTES).build(CacheLoader
> + return CacheBuilder.newBuilder().expireAfterWrite(sessionIntervalInSeconds, TimeUnit.SECONDS).build(CacheLoader
All the OAuth usages so far (precisely 1) use 3600 seconds as the expiry time, so I think the intention was to make that a global default. However it appears to have been fat-fingered and 3600 minutes used instead. This fixes that, and I've confirmed that it causes the GCE ComputeService no longer to fail after 1h.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14/files#r7588200
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @ahgittin! The fix LGTM. If someone with better knowledge of GCE than me does not say the contrary, I'll merge this PR later today.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14#issuecomment-28205809
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by Andrew Phillips <no...@github.com>.
Committed to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds-labs-google.git;a=commit;h=4e6892116819c320d7dd99d0708304b3e670ccd3). Thanks, @ahgittin!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14#issuecomment-28285219
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by Richard Downer <no...@github.com>.
+1 for merging:
https://developers.google.com/accounts/docs/OAuth2UserAgent#handlingtheresponse states "...Other parameters included in the response include expires_in and token_type. **These parameters describe the lifetime of the token in seconds**"
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14#issuecomment-28206901
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #20](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/20/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14#issuecomment-28205106
Re: [jclouds-labs-google] fix for JCLOUDS-372, changing MINUTES to
SECONDS (#14)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #21](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/21/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/14#issuecomment-28284362