chore: slave cleanup and introduce tests #3728
Conversation
warehouse/slave_test.go
Outdated
| for i := 0; i < workerJobs; i++ { | ||
| subscriberCh <- claim | ||
| } | ||
| }() |
There was a problem hiding this comment.
Should we wait for this go routine to return before the test function concludes?
There was a problem hiding this comment.
We are waiting below response := <-publishCh
warehouse/slave_worker.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| jr.uuidTS = timeutil.Now() |
There was a problem hiding this comment.
Should we DI the Now() in our receiver instead of using timeutil ?
Yes. |
achettyiitr
force-pushed
the
chore.whBackendConfig+warehouse-handle-to-router+slave
branch
from
847e985
to
4c463b5
Compare
There was a problem hiding this comment.
Having a quick pass, assuming most of the code was moved and can be further improved on a second pass.
The introduction of tests is really appropriated. Some of the are too specific and testing private methods (example: testing schema is sorted), we might want to cover those private methods in a more behavioural way.
The usage of generator pattern and limit group for upload was a nice touch.
Keep them coming.
achettyiitr
force-pushed
the
chore.whBackendConfig+warehouse-handle-to-router+slave
branch
2 times, most recently
from
89206f2
to
f8aa39b
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3728 +/- ##
==========================================
+ Coverage 68.14% 68.63% +0.49%
==========================================
Files 334 337 +3
Lines 51517 51601 +84
==========================================
+ Hits 35105 35417 +312
+ Misses 14129 13918 -211
+ Partials 2283 2266 -17
☔ View full report in Codecov by Sentry. |
achettyiitr
force-pushed
the
chore.whBackendConfig+warehouse-handle-to-router+slave
branch
from
f8aa39b
to
9545713
Compare
warehouse/slave_test.go
Outdated
| for i := 0; i < workerJobs; i++ { | ||
| response := <-publishCh |
There was a problem hiding this comment.
If we wait for a proper shutdown we could also try to drain this channel at the end and assert that there are no more values in it and that it's been closed. I haven't checked the doability though, just posting here for feedback.
There was a problem hiding this comment.
Since these are mocked channels, not sure it would help much. Also, we are verifying the number of jobs send and received as well.
| sw.notifier.UpdateClaimedEvent(&claimedJob, &pgnotifier.ClaimResponse{ | ||
| Payload: jobResultJSON, | ||
| }) |
There was a problem hiding this comment.
[questions] what is the side effect of these calls failing? I see that we log the errors but beside that, if we don't update the claimed event, what happens next?
There was a problem hiding this comment.
If we don't update the claim event, the side effect is that the notifier.trackUploadBatch will wait for it to complete. Also, in case of failures, since we are just logging, there are maintenance workers who will mark them again as waiting so that it can be picked it up again by another slave.
There was a problem hiding this comment.
Got it, thanks. Does trackUploadBatch wait forever or we got a timeout there?
There was a problem hiding this comment.
Yeah, it waits forever for the job to be completed. Although we have
- maintenance workers which upon some threshold mark the job as waiting.
- also, we have alerts on long-running tasks in cases it exceeds 2 hours.
There was a problem hiding this comment.
We can definitely improve things on pgNotifier.
|
Thanks @lvrach and @fracasula for pointing out go-routine coordination around tests. I have made the changes. |
achettyiitr
deleted the
chore.whBackendConfig+warehouse-handle-to-router+slave
branch
Description
slave_worker_job.goLinear Ticket
Security