nickva commented on code in PR #6013: URL: https://github.com/apache/couchdb/pull/6013#discussion_r3725654966
########## src/couch_replicator/test/eunit/couch_replicator_compression_tests.erl: ########## @@ -0,0 +1,126 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_compression_tests). + +-include_lib("couch/include/couch_eunit.hrl"). +-include_lib("couch/include/couch_db.hrl"). + +-define(DOCS_COUNT, 10). Review Comment: Since we're testing large compression bodies. At at least one test with say 500 docs ########## src/couch_replicator/test/eunit/couch_replicator_compression_tests.erl: ########## @@ -0,0 +1,126 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_compression_tests). + +-include_lib("couch/include/couch_eunit.hrl"). +-include_lib("couch/include/couch_db.hrl"). + +-define(DOCS_COUNT, 10). +-define(TIMEOUT_EUNIT, 30). + +compression_test_() -> + { + "Replication compression tests", + { + foreach, + fun couch_replicator_test_helper:test_setup/0, + fun couch_replicator_test_helper:test_teardown/1, + [ + ?TDEF_FE(should_not_compress_by_default, ?TIMEOUT_EUNIT), + ?TDEF_FE(should_compress_when_enabled, ?TIMEOUT_EUNIT), + ?TDEF_FE(should_compress_per_job, ?TIMEOUT_EUNIT), + ?TDEF_FE(job_compression_overrides_global_disabled, ?TIMEOUT_EUNIT) + ] + } + }. + +should_not_compress_by_default({_Ctx, {Source, Target}}) -> + Before = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + populate_db(Source, ?DOCS_COUNT), + replicate(Source, Target), + compare_dbs(Source, Target), + After = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + ?assertEqual(Before, After). + +should_compress_when_enabled({_Ctx, {Source, Target}}) -> + config:set("replicator", "request_compression", "gzip", false), + config:set("replicator", "compress_min_size", "10", false), + try + Before = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + populate_db(Source, ?DOCS_COUNT), + replicate(Source, Target), + compare_dbs(Source, Target), + After = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + ?assert(After > Before) + after + config:delete("replicator", "request_compression", false), + config:delete("replicator", "compress_min_size", false) + end. Review Comment: As a style nit, typically we don't do a lot of try...after...cleanup in tests. It's better to use proper test setup/cleanup functions. For example this pattern: ```erlang fun setup/0, fun teardown/1, [ ?TDEF_FE(some_test) ] setup() -> config:set("replicator", "request_compression", "none", false), ... other set... couch_replicator_test_helper:test_setup(). teardown(Ctx) -> config:delete("replicator", "request_compression", false), ... other config:delete couch_replicator_test_helper:test_teardown(Ctx). ``` Then each test only has to set non-default values and we skip an extra indent level and an an extra 5 lines with try...after....end block. ########## rel/overlay/etc/default.ini: ########## @@ -734,6 +724,13 @@ partitioned||* = true ; *.example.com:443:[2001:db8::1]:443 ;connect_to = +; Compress outbound replication request bodies (_bulk_docs, _revs_diff). Review Comment: I think we forgot to mention _bulk_gets as well? ########## src/couch_replicator/src/couch_replicator_api_wrap.erl: ########## @@ -1052,6 +1068,32 @@ 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{request_compression = ?COMPRESS_GZIP}, BodySize) -> + MinSize = config:get_integer("replicator", "compress_min_size", ?COMPRESS_MIN_SIZE), + BodySize >= MinSize; +compress_requests(#httpdb{}, _BodySize) -> + false. + +%% Compress Body with gzip, prepend Content-Length and Content-Encoding headers. +%% Returns {CompressedBody, Headers}. +gzip_request_body(Body, Headers) -> + Compressed = zlib:gzip(iolist_to_binary(Body)), + couch_stats:increment_counter([couch_replicator, requests_compressed, gzip]), + {Compressed, [{"Content-Length", byte_size(Compressed)}, {"Content-Encoding", "gzip"} | Headers]}. Review Comment: Style nit: this line is getting a bit too long. Maybe break out `Len = byte_size(Compressed)` to shorten it a bit ########## src/couch_replicator/test/eunit/couch_replicator_compression_tests.erl: ########## @@ -0,0 +1,126 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_compression_tests). + +-include_lib("couch/include/couch_eunit.hrl"). +-include_lib("couch/include/couch_db.hrl"). + +-define(DOCS_COUNT, 10). +-define(TIMEOUT_EUNIT, 30). + +compression_test_() -> + { + "Replication compression tests", + { + foreach, + fun couch_replicator_test_helper:test_setup/0, + fun couch_replicator_test_helper:test_teardown/1, + [ + ?TDEF_FE(should_not_compress_by_default, ?TIMEOUT_EUNIT), + ?TDEF_FE(should_compress_when_enabled, ?TIMEOUT_EUNIT), + ?TDEF_FE(should_compress_per_job, ?TIMEOUT_EUNIT), + ?TDEF_FE(job_compression_overrides_global_disabled, ?TIMEOUT_EUNIT) + ] + } + }. + +should_not_compress_by_default({_Ctx, {Source, Target}}) -> + Before = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + populate_db(Source, ?DOCS_COUNT), + replicate(Source, Target), + compare_dbs(Source, Target), + After = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + ?assertEqual(Before, After). + +should_compress_when_enabled({_Ctx, {Source, Target}}) -> + config:set("replicator", "request_compression", "gzip", false), + config:set("replicator", "compress_min_size", "10", false), + try + Before = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + populate_db(Source, ?DOCS_COUNT), + replicate(Source, Target), + compare_dbs(Source, Target), + After = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + ?assert(After > Before) + after + config:delete("replicator", "request_compression", false), + config:delete("replicator", "compress_min_size", false) + end. + +should_compress_per_job({_Ctx, {Source, Target}}) -> + % global config is none (default), but job sets gzip + config:set("replicator", "compress_min_size", "10", false), + try + Before = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + populate_db(Source, ?DOCS_COUNT), + replicate_with_options(Source, Target, [{<<"request_compression">>, <<"gzip">>}]), + compare_dbs(Source, Target), + After = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + ?assert(After > Before) + after + config:delete("replicator", "compress_min_size", false) + end. + +job_compression_overrides_global_disabled({_Ctx, {Source, Target}}) -> + % global config is gzip, but job disables it + config:set("replicator", "request_compression", "gzip", false), + config:set("replicator", "compress_min_size", "10", false), + try + Before = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + populate_db(Source, ?DOCS_COUNT), + replicate_with_options(Source, Target, [{<<"request_compression">>, <<"none">>}]), + compare_dbs(Source, Target), + After = couch_stats:sample([couch_replicator, requests_compressed, gzip]), + ?assertEqual(Before, After) + after + config:delete("replicator", "request_compression", false), + config:delete("replicator", "compress_min_size", false) + end. + +populate_db(DbName, Count) -> + Docs = lists:map( + fun(I) -> + Id = iolist_to_binary(io_lib:format("doc~p", [I])), + Data = list_to_binary(lists:duplicate(100, $x)), + {[ + {<<"_id">>, Id}, + {<<"value">>, I}, + {<<"data">>, Data} + ]} + end, + lists:seq(1, Count) + ), + {ok, _} = fabric:update_docs(DbName, Docs, [?ADMIN_CTX]), + ok. + +replicate(Source, Target) -> + replicate_with_options(Source, Target, []). + +replicate_with_options(Source, Target, ExtraOptions) -> + SourceUrl = couch_replicator_test_helper:cluster_db_url(Source), + TargetUrl = couch_replicator_test_helper:cluster_db_url(Target), + RepObject = {[ + {<<"source">>, SourceUrl}, + {<<"target">>, TargetUrl}, + {<<"continuous">>, false} + | ExtraOptions + ]}, + {ok, _} = couch_replicator_test_helper:replicate(RepObject). + +compare_dbs(Source, Target) -> Review Comment: Can we use `cluster_compare_dbs/2` instead from the test utils module? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
