Tagfetcherratelimit #9
Conversation
| Idle, | ||
| Initial, |
There was a problem hiding this comment.
IMO the renaming only makes the state machine more difficult to understand, but I'm not gonna start bikeshedding over a name.
| slotStart(m_parentTimeoutMillis, kRateLimitMillis); | ||
| auto timerSinceLastRequest = m_measurementTimer.elapsed(true); | ||
| int timerSinceLastRequestMillis = timerSinceLastRequest.toIntegerMillis(); | ||
| qDebug() << "Task took:" << timerSinceLastRequestMillis; | ||
| int delayMillis = kMinTimeBetweenMbRequests - timerSinceLastRequestMillis; | ||
| if (delayMillis <= 0) { | ||
| qDebug() << "Task took more than a second, slot is calling now."; | ||
| slotStart(m_parentTimeoutMillis, 0); | ||
| } else { | ||
| qDebug() << "Task took less than a second, slot is going to be called in:" << delayMillis; | ||
| slotStart(m_parentTimeoutMillis, delayMillis); | ||
| } |
There was a problem hiding this comment.
is this extra complexity really required? What does advantage does it have?
from what I can tell, delayMillis will always be positive because each task will take longer than a second. Also calling slotStart with 0 delay will cause the request to be fired shortly after the first one, resulting in more than one request per second.
There was a problem hiding this comment.
Imagine the a request itself takes 500 ms. Without it, we will have one request each 1500 ms and not as desired one on 1000 ms. The timer is started right after the wait time is over, just before the actual request is done in doStartNetworkRequest();
There was a problem hiding this comment.
Well how strict the the musicbrainz rate-limiting? If request A arrived at MB after 550ms (bad network routing, flaky connection, whatever) but then comes back after 50ms and request B only took 50ms per direction, the time between the two requests arriving at MB would only be 500ms.
There was a problem hiding this comment.
Two consecutive request do not triggered the rate penalty. My guess is actually for this reason.
@fatihemreyildiz can you confirm that?
There was a problem hiding this comment.
Yes, I can confirm that @daschuer 👍 .
I have tried it many times with the different tracks. After this fix, I've only hit the rate limit once (I think that is because of the global rate limit (300 requests per second) stated on the MB API doc ) besides that I've never hit the rate limit. If this happens, it can be easily fixed by a retry button replaced on the new GUI.
Since I played with the tag fetcher a lot. I also want to add few things that I noticed. If a track has about 8-9 recording IDs they might hit the rate limit time to time in my case (If someone has a way better connection than I have, this amount of recording IDs might be lower). After 10 recording IDs, I always hit the rate limit, as I said this can be different in your case.
One of my rough test result was like this:
- Abba - Super Trouper -> Has 5 recording IDs normally took 4.78 seconds - after fix took 6.40 seconds
- Axwell Ingrosso - MTYK -> Has 6 recording IDs normally took 4.30 seconds - after fix took 6.81 seconds
- DVBBS - Tsunami -> Has 12 recording IDs normally failed in 8:36 seconds - after fix took 18.15 seconds.
In this case with the delayMillis the difference is not too much for the tracks which has less recording IDs. If we delay the every request by 1000ms that would be easily noticeable for popular tracks.
There was a problem hiding this comment.
fine. If this avoids a failure we can keep it.
|
Also, alternatively we could ask the musicbrainz folks if they can make a special rule for us (just as they do for beets for example). Having to do requests in serial which we could technically do in parallel results in much worse UX... |
|
We already request the releases in chunks of ~5. The wait time happens only if you have a recording with many releases like Abba or other popular tracks like that. So it is effordable. Before we request an extra rule, it is a good idea to not ignore the current rule. |
I totally agree with that, I was thinking to send a detailed message about our situation. Then I wanted to mention that delaying our requests might be bad in user experience. For that I was waiting fix PR to be merged. |
| if (m_state == State::Aborted) { | ||
| emitAborted(responseWithContent.requestUrl()); |
There was a problem hiding this comment.
The state is still aborted, why remove the emitAborted signal?
There was a problem hiding this comment.
This condition can no longer be true because we have removed m_state = State::Aborted; above.
The original issue was that emitAborted() is designed to be the responds to manual user Abort(). There is no change in the GUI. Now we emitNetworkError() in the client timeout case as well.
There was a problem hiding this comment.
So does the Abort state have any purpose then? Can we remove it?
There was a problem hiding this comment.
We need it to change the state from any other state when calling slotAbort()
However we have some other places where State::Abort is used in responds of an error. I will do some checks if this even works as desired.
This is the second part of the PR taken from mixxxdj#10822
It calculates the time from the start of the request instead of the time in between.
It also cleans up the state chart and fixes the stalled Window on case of client side timeout.