Closed Kyria closed 7 years ago
We know that CREST limits at 20 concurrent connections so it wouldn't be to bad to hard code this value by default. Assuming this is the only reason we would like to specify an TransportAdapter.
Ah I forgot about that connection limit, good catch!
Disadvantage of hardcoding it is if ccp changes stuff in the future.
but maybe just as a number we could set as a default parameter for future use?
if CCP upgrades hardware we might see an increase in number of connections, but the API servers probably are not going to be upgraded for a while and we could just be informed about it beforehand, if hardcoding it is not acceptable, we would probably be building a huge argument list to init an instance, as we are adding quite a few features on this rebuild.
Then lets put an optional parameter where the user can pass in a TransportAdaptor and control whatever parameters themselves. Then in basic use people don't have to pass one in, but if they need the extra functionality they can configure it exactly how they want. Then its more flexible than adding an optional parameter to control the number of connections, and there's no hardcoding of connection values to a particular number.
sounds good to me
fixed in PR https://github.com/pycrest/PyCrest/pull/48
One issue I encountered while I wrote a market price fetcher with CREST and PyCrest ("looping" through regions and items) was getting 503 errors, not because of my request/seconds, but concurrent connection to the CREST endpoints : CCP only allows 20 concurrent connection at any given time from one IP.
Right now, PyCrest, when calling multiple endpoints, will open multiple connection as no connection pool is set. In most cases, the connections will be fast enough to be opened, to get the data and close, and this error will almost never happen. But in some case, especially for higher load actions (such as getting all market orders in multiple regions) along with "normal use", the number of connection opened will get higher than 20, and then CREST will raise 503 errors.
After searching I ended up with this fix:
While I don't actually know if it's the best place to set this, I believe this should be set in PyCrest, or at least, the user must be given the options either to manually set the pool_connections values, or to give a HTTPAdapter (set for his needs) in parameter to the APIConnection when he uses PyCrest, like
EVE(http_adapter=HTTPAdapter(pool_connections=20))