Split up SyncClient and fix bandwidth tracking
#85
Conversation
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
==========================================
+ Coverage 82.85% 84.47% +1.62%
==========================================
Files 37 37
Lines 2385 2442 +57
Branches 301 303 +2
==========================================
+ Hits 1976 2063 +87
+ Misses 309 280 -29
+ Partials 100 99 -1 |
bjester
changed the title
SyncClientSyncClient and fix bandwidth tracking
There was a problem hiding this comment.
Looks great! One of my main questions was addresses in the latest commit (the JSON-serialization of the client_fsic field), so it seems basically good to go from my side, just some small suggestions and then one thing around the cache key that seems important.
| except TypeError: | ||
| pass | ||
| self.bytes_sent += _length_of_headers(prepped.headers) | ||
| self.bytes_sent += _headers_content_length(prepped.headers) |
There was a problem hiding this comment.
When we're doing the sending, do we assume the content length header will always be set, and hence this would never be 0?
There was a problem hiding this comment.
No we aren't assuming it's always set. If the request has no body, the requests helper will not add it. So I believe this should be okay.
There was a problem hiding this comment.
Right, sorry, what I mean is: can we safely assume that if the request has a body, the Content-Length header will have already been added, so that we wouldn't miss counting that transfer amount? I know on the receiving side we handle the case of the missing header (in case it was... removed by a proxy or something?), but here I'm guessing it's OK, as we have more control (request is being initiated locally).
There was a problem hiding this comment.
The missing header on the response side is for when the response is chunked. The request body can be chunked too, but for the sync, none of the requests are chunked because we give the whole body. So if there is a body, then it would have the content length header. I can add comments so that's clear.
Summary
Splits up the
SyncClientclass into a base class and two classes for each sync direction,PullClientandPushClient. This will enable us to better handling any database locking that should occur during the sync stages. I've left a small wrapper class in place forSyncClientsuch that this change should be backwards compatible for now.This also fixes bandwidth tracking by attempting to track the response body size if there isn't a
Content-Lengthheader. It also now tracks the headers to get a better estimate of the entire request/response size.Additionally, this fixes request exception handling in the
SessionWrapper. Firstly, it was casting errors to strings which the errors being captured don't have a__str__method defined, so it now simply logs the error class name. Also, it's more defensive on accessing the response from the error since it was always assuming it existed and that it was JSON.Lastly, this adds a condition to only dequeue from the API to delete a transfer session when the session actually had records. This avoids a lengthy, serialize and then deserialize within the dequeue function when it's unnecessary.
TODO
Reviewer guidance
This will be most easily tested when integrated with Kolibri
Issues addressed
Related to learningequality/kolibri#7125