Have client's TestOnly activate AsyncCompleter instead of BatchCompleter
#441
Conversation
…ompleter` I was looking into why River's example tests were so incredibly slow. By far the biggest reason was that maintenance services were jittering on start up, but by far the _second_ biggest reason is that the batch completer pauses before carrying out any completion (waiting to see if anymore jobs will come in to add to the completion batch), which delays subscribers from receiving a confirmation that a job's been finished. Here, I propose that we have the `TestOnly` flag activate the async completer instead of the normal batch completer. This removes the pause mentioned above, and especially in test cases where job volume is expected to be low, will have no other performance disadvantage. The batch completer is generally more complicated, so this will probably also lower memory overhead. There is some risk in removing the batch completer from standard test usage in that it means less load on the batch completer, but I think is probably okay because the batch completer's test suite in isolation is very comprehensive. An alternative to this is might be to make the client's completer configurable through `Config`. I thought about that approach, but it has the downside of exposing more API, and the async completer will be more appropriate for use in 99.9% of test cases anyway, so it's an extra complication with no real advantage.
|
@bgentry This one's a little more invasive, but wanted to get your thoughts on it. It'll make any integration-level tests that needs to wait for a job to round trip 100 ms or so faster. |
There was a problem hiding this comment.
(thought I submitted this last night, sorry!)
I think I'm generally ok with this (most people running tests won't want the batch completer) but I am slightly concerned about the loss of internal coverage from it. Trying to think if there are specific cases where we should explicitly keep the batch completer active in tests just to be good there. Maybe think about that, but LGTM regardless ✌️
|
Thanks! Yeah, thinking a little more on this one, I think I'm going to put it on ice for a while. If the test suite improvement was like 20% or some big number like that, it'd be totally worth it IMO, but it does seem to be only a few seconds right now, putting the cost/benefit more into question. |
| client.services = append(client.services, client.completer) | ||
|
|
||
| client.subscriptionManager = newSubscriptionManager(archetype, nil) | ||
| client.services = append(client.services, client.completer, client.subscriptionManager) | ||
| client.services = append(client.services, client.subscriptionManager) |
There was a problem hiding this comment.
It would be cleaner to delete the first append and merge it into the second with append(client.services, client.completer, client.subscriptionManager). This potentially also avoids an unnecessary allocation as append only needs to allocate more memory once instead of potentially twice.
There was a problem hiding this comment.
Realistically, if Go does need to grow the slice, it'll 2x the underlying size, so it's not going to make any difference. I'd take the small readability advantage instead of trying to micro-optimize here.
I was looking into why River's example tests were so incredibly slow. By
far the biggest reason was that maintenance services were jittering on
start up, but by far the second biggest reason is that the batch
completer pauses before carrying out any completion (waiting to see if
anymore jobs will come in to add to the completion batch), which delays
subscribers from receiving a confirmation that a job's been finished.
Here, I propose that we have the
TestOnlyflag activate the asynccompleter instead of the normal batch completer. This removes the pause
mentioned above, and especially in test cases where job volume is
expected to be low, will have no other performance disadvantage. The
batch completer is generally more complicated, so this will probably
also lower memory overhead.
There is some risk in removing the batch completer from standard test
usage in that it means less load on the batch completer, but I think is
probably okay because the batch completer's test suite in isolation is
very comprehensive.
An alternative to this is might be to make the client's completer
configurable through
Config. I thought about that approach, but it hasthe downside of exposing more API, and the async completer will be more
appropriate for use in 99.9% of test cases anyway, so it's an extra
complication with no real advantage.