Improve Overpass query - #336
Conversation
- Change from GET to POST - Set Timeout in Seconds - Implement sequential querying with one query for a server at a time
| final _random = Random(); | ||
| int _currentServerIndex = 0; | ||
|
|
||
| final Map<String, Completer<void>> _serverLocks = {}; |
There was a problem hiding this comment.
Why not also work with the index / via List here? final List<Completer<void>>
| 'https://overpass-api.de/api/interpreter', | ||
| 'https://maps.mail.ru/osm/tools/overpass/api/interpreter', | ||
| 'https://overpass.private.coffee/api/interpreter', |
There was a problem hiding this comment.
Last time we checked private coffee was way more responsive than the main overpass server. This is why I would only include it for now (even though it reduces the usefulness of the round robin implementation).
| for (final server in apiServers) { | ||
| _serverLocks[server] = Completer<void>()..complete(); | ||
| } | ||
| } |
There was a problem hiding this comment.
You should be able to use _serverLocks = List.filled(3, Completer<void>()..complete(), gorwable = false); or List.generate directly inside the constructor assignment.
| _currentServerIndex = (_currentServerIndex + 1) % apiServers.length; | ||
|
|
||
| // Wait for the server to be free | ||
| await _serverLocks[url]!.future; |
There was a problem hiding this comment.
Couldn't it be the case that:
_currentServerIndex= 1- server at index 0 and 1 are already done
- server at index 2 is still busy
-> we wait even though there are free servers?
Also if _query is called rapidly from the outside (e.g. called 4 times when we have 3 servers) then 2 of these queries will wait on the same completer/future to be finished and once it is finished one will override the lock of the other and both server requests will be run.
I think I prefer a queue based approach where all query tasks are queued inside a Queue and once a query finishes it checks whether something is still in the queue. If it finds something it takes it (removes it from the queue) and runs another query.
Also I think we could improve the retry functionality. I suspect that some requests get blocked by the server with "too many requests" or something similar, in this case retrying the query just makes everything worse.
| // Release lock before waiting for retry to allow other cells to use this server slot during delay | ||
| _serverLocks[url]!.complete(); |
There was a problem hiding this comment.
Shouldn't be necessary as return is immediately called (there is no await) wherefore the finally block will run afterwards. For clarification: finally always runs even if the function already returns. It will run after the return.
Also I wonder whether a completer is really necessary. We might just be able to store the Future (or a wrapped version of it) from the _dio.post directly inside _serverLocks
2 participants
This PR will hopefully make the Overpass query for stop areas more reliable.
It adresses the following things:
Change from GET to POST
According to this best practice:
Set Timeout in Seconds
Up until now the timeout was send in milliseconds and ccording to the language reference:
Round Robin
The PR introduces a sequential querying with one query for a server at a time. Only if the server responded the next query is sent.
Additional Server
An additional Overpass Server was added to better distribute the load.