Set a custom API limit if specified in the config. #1043
Conversation
POEApi.Transport/TaskThrottle.cs
Outdated
| @@ -55,7 +61,7 @@ public void StartTask() | |||
| } | |||
|
|
|||
| RemvoeExpiredTasks(); | |||
There was a problem hiding this comment.
Appreciate it's not your typo, could you adjust please.
There was a problem hiding this comment.
Done. (I rewrote the class, so, ultimately, it is my typo. :) )
POEApi.Transport/TaskThrottle.cs
Outdated
| @@ -36,6 +36,12 @@ public TaskThrottle(TimeSpan windowSize, int windowLimit, int simultaneiousTasks | |||
| SimultaneiousTasksLimit = simultaneiousTasksLimit; | |||
POEApi.Model/POEModel.cs
Outdated
| if (Settings.UserSettings.Keys.Contains("ApiRequestLimit") && | ||
| int.TryParse(Settings.UserSettings["ApiRequestLimit"], out customRequestLimit)) | ||
| { | ||
| if (HttpTransport.AdjustThrottleWindowLimit(customRequestLimit)) |
There was a problem hiding this comment.
Can you change the message generation to a ternary please, more succinct.
There was a problem hiding this comment.
Something like this?
| @@ -36,6 +36,8 @@ protected enum HttpMethod { GET, POST } | |||
| private const string UpdateShopURL = @"https://www.pathofexile.com/forum/edit-thread/{0}"; | |||
| private const string BumpShopURL = @"https://www.pathofexile.com/forum/post-reply/{0}"; | |||
|
|
|||
| private const int _maximumWindowLimit = 42; | |||
There was a problem hiding this comment.
Can this be brought in as an application setting?
There was a problem hiding this comment.
I would rather not, for two reasons.
- This value will likely very rarely -- if ever -- change, so the user will be unlikely to modify the setting. He can also easily make Procurement work very poorly by increasing the value.
- This class can't reference the
Settingsdirectly, since that would introduce a circular dependency between the projects. Thus -- barring more substantial refactoring I'm not interested in doing now -- it would then have to be set after the static constructor finishes, potentially after it started handling requests.
POEApi.Transport/HttpTransport.cs
Outdated
|
|
||
| protected static TaskThrottle InitializeTaskThrottle() | ||
| { | ||
| return new TaskThrottle(TimeSpan.FromMinutes(1), _maximumWindowLimit, _maximumWindowLimit); |
There was a problem hiding this comment.
I can't tell just from the PR, isn't this just going to mindlessly overwrite the previously set task throttle?
There was a problem hiding this comment.
It would, if it were called again. As written, it is only ever invoked during HttpTransport's static constructor. It's also protected, so it can't be called by other classes, and HttpTransport itself doesn't call it. In any case, I changed it back to not call a function, so it is less ambiguous.
|
I'll take a look at this, and the other outsanding PR's as soon as I can. Thanks for the continued contributions and effort 👍 |
The HttpTransport will only allow a positive limit that is no larger than the real API limit. A custom, smaller API limit will allow Procurement to play nicely with other applications that perform a known maximum number of API requests per minute.
thailyn
force-pushed
the
custom-api-limit
branch
from
616434c
to
7fc1882
Compare
The HttpTransport will only allow a positive limit that is no larger than the real API limit. A custom, smaller API limit will allow Procurement to play nicely with other applications that perform a known maximum number of API requests per minute.