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_r3476256362


##########
src/couch_replicator/src/couch_replicator_httpc.erl:
##########
@@ -21,6 +21,7 @@
 -export([stop_http_worker/0]).
 -export([full_url/2]).
 
+

Review Comment:
   Avoid adding extra changes to the PR that are not relevant. We can do a separate PR later to cleanup or fix up whitespace.



##########
src/couch_replicator/src/couch_replicator_api_wrap.erl:
##########
@@ -477,40 +478,35 @@ update_docs(#httpdb{} = HttpDb, DocList, Options, UpdateType) ->
     % Note: nginx and other servers don't like PUT/POST requests without
     % a Content-Length header, so we can't do a chunked transfer encoding
     % and JSON encode each doc only before sending it through the socket.
-    {Docs, Len} = lists:mapfoldl(
+    {Docs, _} = lists:mapfoldl(
         fun
             (#doc{} = Doc, Acc) ->
                 Json = ?JSON_ENCODE(couch_doc:to_json_obj(Doc, [revs, attachments])),
                 {Json, Acc + iolist_size(Json)};
             (Doc, Acc) ->
                 {Doc, Acc + iolist_size(Doc)}
         end,
-        byte_size(Prefix) + byte_size(Suffix) + length(DocList) - 1,
+        0,

Review Comment:
   I think it might be nice to only materialize the full body and compress it if we're compressing it (algorithm != none) otherwise keep it as a stream
   
   ```erlang
       {Body, Headers} =
           case compress_requests(Len) of
               true ->
                   FullBody = [Prefix, lists:join(<<",">>, Docs), Suffix],
                   gzip_request_body(FullBody, Headers0);
               false ->
                   {{BodyFun, [prefix | Docs]}, Headers0}
           end
   ```



##########
src/couch_replicator/src/couch_replicator_api_wrap.erl:
##########
@@ -1052,6 +1048,33 @@ header_value(Key, Headers, Default) ->
         _ ->
             Default
     end.
+    
+%% Compress Body with gzip if enabled and body is large enough.
+%% Returns {Body, ExtraHeaders} where ExtraHeaders may contain Content-Encoding.
+maybe_compress(Body) when is_binary(Body) ->
+    case config:get_boolean("replicator", "compress_requests", false) of
+        true ->
+            Algorithm = config:get("replicator", "compression_algorithm", "gzip"),
+            MinSize = config:get_integer("replicator", "compress_min_size", 1024),
+            case byte_size(Body) >= MinSize of
+                true -> compress_with(Algorithm, Body);
+                false -> {Body, []}
+            end;
+        false ->
+            {Body, []}
+    end.
+
+compress_with("gzip", Body) ->
+    Compressed = zlib:gzip(Body),
+    couch_stats:increment_counter([couch_replicator, requests_compressed]),
+    couch_stats:increment_counter([couch_replicator, requests_compressed, gzip]),
+    {Compressed, [{"Content-Encoding", "gzip"}]};
+compress_with(Other, Body) ->

Review Comment:
   If user specified an unusable compression we'd spam the logs potentially thousands of times a second with the warning. We could probably leave it out and user can judge by the metrics



##########
src/couch_replicator/src/couch_replicator_api_wrap.erl:
##########
@@ -1052,6 +1048,33 @@ header_value(Key, Headers, Default) ->
         _ ->
             Default
     end.
+    
+%% Compress Body with gzip if enabled and body is large enough.
+%% Returns {Body, ExtraHeaders} where ExtraHeaders may contain Content-Encoding.
+maybe_compress(Body) when is_binary(Body) ->

Review Comment:
   Not sure if it would be much harder but this could also be a candidate to set per jobs and/or per cluster. For example replicating from a->b running on c we may want to compress requests to b but then for another job, a->d we might not, because d can't handle gzip. So it might be nice if this option can be the same as https://docs.couchdb.org/en/stable/config/replicator.html#replicator/worker_processes? If that looks too unwieldy or tricky we can skip it



##########
src/couch_replicator/src/couch_replicator_api_wrap.erl:
##########
@@ -1052,6 +1048,33 @@ header_value(Key, Headers, Default) ->
         _ ->
             Default
     end.
+    
+%% Compress Body with gzip if enabled and body is large enough.
+%% Returns {Body, ExtraHeaders} where ExtraHeaders may contain Content-Encoding.
+maybe_compress(Body) when is_binary(Body) ->
+    case config:get_boolean("replicator", "compress_requests", false) of
+        true ->
+            Algorithm = config:get("replicator", "compression_algorithm", "gzip"),
+            MinSize = config:get_integer("replicator", "compress_min_size", 1024),

Review Comment:
   Tiny nit: pull 1024 as a define to the top (`-define(COMPRESS_MIN_SIZE, 1024)`) also can make a few defines for compression algorithms: "none", "gzip" that way we can ensure we don't mistype them as the compiler will check them for us automatically



##########
src/couch_replicator/src/couch_replicator_api_wrap.erl:
##########
@@ -1052,6 +1048,33 @@ header_value(Key, Headers, Default) ->
         _ ->
             Default
     end.
+    
+%% Compress Body with gzip if enabled and body is large enough.
+%% Returns {Body, ExtraHeaders} where ExtraHeaders may contain Content-Encoding.
+maybe_compress(Body) when is_binary(Body) ->
+    case config:get_boolean("replicator", "compress_requests", false) of
+        true ->
+            Algorithm = config:get("replicator", "compression_algorithm", "gzip"),
+            MinSize = config:get_integer("replicator", "compress_min_size", 1024),
+            case byte_size(Body) >= MinSize of
+                true -> compress_with(Algorithm, Body);
+                false -> {Body, []}
+            end;
+        false ->
+            {Body, []}
+    end.
+
+compress_with("gzip", Body) ->
+    Compressed = zlib:gzip(Body),
+    couch_stats:increment_counter([couch_replicator, requests_compressed]),

Review Comment:
   Redundant/extra metric?
   
   



##########
rel/overlay/etc/default.ini:
##########
@@ -734,6 +724,14 @@ partitioned||* = true
 ;   *.example.com:443:[2001:db8::1]:443
 ;connect_to =
 
+; Compress outbound replication request bodies (_bulk_docs, _revs_diff) with gzip.
+; Disabled by default. Only gzip is supported. Enable only when talking to CouchDB
+; servers that support gzip Content-Encoding on inbound requests.
+;compress_requests = false

Review Comment:
   Maybe we can drive both the request compression toggle and the algorithm with just a single config. 
   
   We have `[couchdb] file_compression = none | snappy | deflate | zstd` maybe we can use the same scheme
   
   ```ini
   [replicator]
   request_compression = none | gzip
   ```
   
   Eventually add `zstd` in the future perhaps.



-- 
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