-
Notifications
You must be signed in to change notification settings - Fork 735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose Rate Limit Headers #327
Conversation
Exposes the rate limit header responses so that consumers of the API can proactively tune their usage
I guess you far away from good gh integration, so soon you will get #303 |
88476a3
to
6fcddf4
Compare
@kohsuke ping |
r.reset = new Date(System.currentTimeMillis() + 1 * hours ); | ||
return r; | ||
long hour = 60L * 60L; // this is madness, storing the date as seconds in a Date object | ||
r.reset = new Date((System.currentTimeMillis() + hour) / 1000L ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry, but what the crazy calculation do you do here?
hour - value in second
System.currentTimeMillis() - return millisecond
(millisecond + second)/ 1000L - strange unpredictable type of second, we can say this type of seconds as X.
By javadoc:
public Date(long date)
date the milliseconds since January 1, 1970, 00:00:00 GMT.
if we will use new Date( X ), then we can't predict what type of date we will receive.
@stephenc @KostyaSha @kohsuke
with this new code you broke reset time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! Read the comment in GHRateLimit... for crazy reasons that class stores the date in Date with seconds expressed as milliseconds.
Without this fixup the reset date will be incorrect as returned by GHRateLimit.getResetDate()
I have comments indicating that it be madness... but actually is the fix if least harm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack the adding of an hours seconds should be outside of the division, but this will still give a result in the future for GHE and as GHE does not have a rate limit...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read about "this is madness, storing the date as seconds in a Date object"
but very confused by the division.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/kohsuke/github-api/blob/4ee3086b6dcb46913f1341b2500a4790db5813d6/src/main/java/org/kohsuke/github/GHRateLimit.java#L31 is why we need to do the division... and that is madness... but fixing that madness is orthogonal to this enhancement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#329 will fix the algebra... but it is not strictly required as a limit of no limit expiring in 3.6 seconds or expiring in 1 h is still a limit of no limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But without correcting for the 1000 fold factor then GHE would say it had a limit of 1000000 being refreshed on Tue Jun 20 01:41:44 IST 48997 which allows 21 API calls per year... so code that uses the rate limit to pre-throttle will run unnecessarily slow on GHE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or were you accessing the public field directly (oh how I love the way people make fields public in Java and make migration very difficult without breaking backwards compatibility)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is why we need to do the division... and that is madness... but fixing that madness is orthogonal to this enhancement
Yes, I agree with this, fixing this madness must doing not in this issue.
Exposes the rate limit header responses so that consumers of the API can proactively tune their usage.
@reviewbybees