Re: [PR] Add compression to replicator [couchdb]

["nickva (via GitHub)" <gi...@apache.org>]

nickva commented on code in PR #6013:
URL: https://github.com/apache/couchdb/pull/6013#discussion_r3617956601


##########
src/couch_replicator/src/couch_replicator_api_wrap.erl:
##########
@@ -1052,6 +1065,30 @@ header_value(Key, Headers, Default) ->
         _ ->
             Default
     end.
+    
+%% Returns true if compression is enabled for HttpDb and Body is large enough.
+compress_requests(#httpdb{request_compression = ?COMPRESS_NONE}, _BodySize) ->
+    false;
+compress_requests(#httpdb{}, BodySize) ->

Review Comment:
   I wonder if we're missing a compression algorithm check here. If we get ?NONE we catch the first clause but here if we get a gzzzip and over minsize we'd end up compressing. We should probably check explicitly, keeping a shape of the code where we could easily add zstd or other compression methods. If we get a bogus method we could try falling back to ?NONE but not the new gzip default.



##########
src/couch_replicator/src/couch_replicator_api_wrap.erl:
##########
@@ -171,13 +175,15 @@ ensure_full_commit(#httpdb{} = Db) ->
 
 get_missing_revs(#httpdb{} = Db, IdRevs) ->
     JsonBody = {[{Id, couch_doc:revs_to_strs(Revs)} || {Id, Revs} <- IdRevs]},
+    RawBody = ?JSON_ENCODE(JsonBody),
+    {Body, ExtraHeaders} = maybe_compress(Db, RawBody),

Review Comment:
   We have to be careful here. ?JSON_ENCODE/1 may also return iodata not just binaries and maybe compress handles binary with a guard. The fix would be in maybe_compress to drop the guard and use iolist_size instead



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org