You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@couchdb.apache.org by va...@apache.org on 2022/11/18 22:04:43 UTC
[couchdb] 02/05: Improve smoosh_utils
This is an automated email from the ASF dual-hosted git repository.
vatamane pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/couchdb.git
commit 57b2dc340efc5f267f3200d17e30f977eedf491b
Author: Nick Vatamaniuc <va...@gmail.com>
AuthorDate: Fri Nov 4 00:59:47 2022 -0400
Improve smoosh_utils
Add functions related to dealing with new index cleanup channel.
Add functions which fetch the channels list from config. Fix an unfortunate
corner case where operators when creating custom smoosh channel may omit by
mistake some of the built-in channel like view upgrades. To improve the
behavior, make built-in channel always available. Channels configured by the
user are always appended to the list of built-in ones.
The new validate_arg/1 function is in charge of validation smoosh's input args.
It centralizes ?b2l, ?l2b and other transforms and checks in one place so they
are not sprinkled throughout the internal functions. After the validation
steps all the db and view names are guaranteed to be binaries.
The most significant changes is probably addition of extra tests to get to 100%
test coverage. Since we cover all of the cases in time window checking, remove
the more heavyweight elixir test, since we now get a nice coverage report in
Erlang and the test is overall smaller as well.
---
src/smoosh/src/smoosh_utils.erl | 280 +++++++++++++++++-----
src/smoosh/test/exunit/scheduling_window_test.exs | 79 ------
src/smoosh/test/exunit/test_helper.exs | 2 -
3 files changed, 217 insertions(+), 144 deletions(-)
diff --git a/src/smoosh/src/smoosh_utils.erl b/src/smoosh/src/smoosh_utils.erl
index 1942aebf4..9a2f59086 100644
--- a/src/smoosh/src/smoosh_utils.erl
+++ b/src/smoosh/src/smoosh_utils.erl
@@ -14,8 +14,20 @@
-include_lib("couch/include/couch_db.hrl").
-export([get/2, get/3, split/1, stringify/1, ignore_db/1]).
--export([in_allowed_window/1, is_view_channel/1, write_to_file/3]).
+-export([db_channels/0, view_channels/0, cleanup_channels/0]).
+-export([in_allowed_window/1]).
+-export([concurrency/1, capacity/1]).
-export([log_level/2]).
+-export([validate_arg/1]).
+
+-define(BUILT_IN_DB_CHANNELS, "upgrade_dbs,ratio_dbs,slack_dbs").
+-define(BUILT_IN_VIEW_CHANNELS, "upgrade_views,ratio_views,slack_views").
+-define(BUILT_IN_CLEANUP_CHANNELS, "index_cleanup").
+
+-define(INDEX_CLEANUP, index_cleanup).
+
+-define(DEFAULT_CONCURRENCY, "1").
+-define(DEFAULT_CAPACITY, "9999").
get(Channel, Key) ->
?MODULE:get(Channel, Key, undefined).
@@ -23,14 +35,22 @@ get(Channel, Key) ->
get(Channel, Key, Default) ->
config:get("smoosh." ++ Channel, Key, Default).
+split(undefined) ->
+ [];
+split("") ->
+ [];
split(CSV) ->
re:split(CSV, "\\s*,\\s*", [{return, list}, trim]).
+stringify({?INDEX_CLEANUP, DbName}) ->
+ io_lib:format("~s index_cleanup", [DbName]);
stringify({DbName, GroupId}) ->
io_lib:format("~s ~s", [DbName, GroupId]);
stringify(DbName) ->
io_lib:format("~s", [DbName]).
+ignore_db({?INDEX_CLEANUP, DbName}) ->
+ ignore_db(DbName);
ignore_db({DbName, _GroupName}) ->
ignore_db(DbName);
ignore_db(DbName) when is_binary(DbName) ->
@@ -41,13 +61,11 @@ ignore_db(DbName) when is_list(DbName) ->
true;
_ ->
false
- end;
-ignore_db(Db) ->
- ignore_db(couch_db:name(Db)).
+ end.
in_allowed_window(Channel) ->
- From = parse_time(get(Channel, "from"), {00, 00}),
- To = parse_time(get(Channel, "to"), {24, 00}),
+ From = parse_time(?MODULE:get(Channel, "from"), {00, 00}),
+ To = parse_time(?MODULE:get(Channel, "to"), {24, 00}),
in_allowed_window(From, To).
in_allowed_window(From, To) ->
@@ -59,63 +77,6 @@ in_allowed_window(From, To) ->
({HH, MM} >= From) orelse ({HH, MM} < To)
end.
-is_view_channel(ChannelName) ->
- ViewChannels = smoosh_utils:split(
- config:get("smoosh", "view_channels", "upgrade_views,ratio_views,slack_views")
- ),
- lists:member(ChannelName, ViewChannels).
-
-file_delete(Path) ->
- case file:delete(Path) of
- % When deleting a state file, we do not care if it does not exist.
- Ret when Ret =:= ok; Ret =:= {error, enoent} ->
- ok;
- {error, Reason} ->
- {error, Reason}
- end.
-
-write_to_file(Content, FileName, VSN) ->
- Level = log_level("compaction_log_level", "debug"),
- couch_log:Level("~p Writing state ~s", [?MODULE, FileName]),
- OnDisk = <<VSN, (erlang:term_to_binary(Content, [compressed, {minor_version, 1}]))/binary>>,
- TmpFileName = FileName ++ ".tmp",
- case file_delete(TmpFileName) of
- ok ->
- case file:write_file(TmpFileName, OnDisk, [sync]) of
- ok ->
- case file_delete(FileName) of
- ok ->
- case file:rename(TmpFileName, FileName) of
- ok ->
- ok;
- {error, Reason} ->
- couch_log:error(
- "~p (~p) Error renaming temporary state file ~s to ~s", [
- ?MODULE, Reason, TmpFileName, FileName
- ]
- ),
- ok
- end;
- {error, Reason} ->
- couch_log:error("~p (~p) Error deleting state file ~s", [
- ?MODULE, Reason, FileName
- ]),
- ok
- end;
- {error, Reason} ->
- couch_log:error("~p (~p) Error writing state to temporary file ~s", [
- ?MODULE, Reason, TmpFileName
- ]),
- ok
- end;
- {error, Reason} ->
- % Failing to persist the queue should not crash smoosh. Return ok.
- couch_log:error("~p (~p) Error deleting temporary state file ~s", [
- ?MODULE, Reason, TmpFileName
- ]),
- ok
- end.
-
parse_time(undefined, Default) ->
Default;
parse_time(String, Default) ->
@@ -135,3 +96,196 @@ parse_time(String, Default) ->
log_level(Key, Default) when is_list(Key), is_list(Default) ->
list_to_existing_atom(config:get("smoosh", Key, Default)).
+
+db_channels() ->
+ ConfStr = config:get("smoosh", "db_channels"),
+ channel_list(ConfStr, ?BUILT_IN_DB_CHANNELS).
+
+view_channels() ->
+ Conf = config:get("smoosh", "view_channels"),
+ channel_list(Conf, ?BUILT_IN_VIEW_CHANNELS).
+
+cleanup_channels() ->
+ Conf = config:get("smoosh", "cleanup_channels"),
+ channel_list(Conf, ?BUILT_IN_CLEANUP_CHANNELS).
+
+channel_list(ConfStr, Default) ->
+ DefaultList = split(Default),
+ ConfList = split(ConfStr),
+ lists:usort(DefaultList ++ ConfList).
+
+concurrency(ChannelName) ->
+ list_to_integer(?MODULE:get(ChannelName, "concurrency", ?DEFAULT_CONCURRENCY)).
+
+capacity(ChannelName) ->
+ list_to_integer(?MODULE:get(ChannelName, "capacity", ?DEFAULT_CAPACITY)).
+
+% Validate enqueue arg at the front of the API instead of adding ?l2b and ?b2l
+% everywhere internally
+%
+validate_arg({?INDEX_CLEANUP, DbName}) when is_list(DbName) ->
+ validate_arg({?INDEX_CLEANUP, ?l2b(DbName)});
+validate_arg(DbName) when is_list(DbName) ->
+ validate_arg(?l2b(DbName));
+validate_arg({DbName, GroupId}) when is_list(DbName) ->
+ validate_arg({?l2b(DbName), GroupId});
+validate_arg({DbName, GroupId}) when is_list(GroupId) ->
+ validate_arg({DbName, ?l2b(GroupId)});
+validate_arg(DbName) when is_binary(DbName) ->
+ DbName;
+validate_arg({DbName, GroupId}) when is_binary(DbName), is_binary(GroupId) ->
+ {DbName, GroupId};
+validate_arg({?INDEX_CLEANUP, DbName}) when is_binary(DbName) ->
+ {?INDEX_CLEANUP, DbName};
+validate_arg(_) ->
+ error(invalid_smoosh_arg).
+
+-ifdef(TEST).
+
+-include_lib("couch/include/couch_eunit.hrl").
+
+smoosh_util_validate_test() ->
+ ?assertEqual(<<"x">>, validate_arg(<<"x">>)),
+ ?assertEqual(<<"x">>, validate_arg("x")),
+ ?assertEqual({<<"x">>, <<"y">>}, validate_arg({"x", "y"})),
+ ?assertEqual({<<"x">>, <<"y">>}, validate_arg({<<"x">>, "y"})),
+ ?assertEqual({<<"x">>, <<"y">>}, validate_arg({"x", <<"y">>})),
+ ?assertEqual({<<"x">>, <<"y">>}, validate_arg({<<"x">>, <<"y">>})),
+ ?assertEqual({?INDEX_CLEANUP, <<"x">>}, validate_arg({?INDEX_CLEANUP, "x"})),
+ ?assertEqual({?INDEX_CLEANUP, <<"x">>}, validate_arg({?INDEX_CLEANUP, <<"x">>})),
+ ?assertError(invalid_smoosh_arg, validate_arg(foo)),
+ ?assertError(invalid_smoosh_arg, validate_arg({foo, bar})),
+ ?assertError(invalid_smoosh_arg, validate_arg({?INDEX_CLEANUP, foo})).
+
+smoosh_utils_test_() ->
+ {
+ foreach,
+ fun() ->
+ meck:new(calendar, [passthrough, unstick]),
+ meck:expect(config, get, fun(_, _, Default) -> Default end)
+ end,
+ fun(_) ->
+ meck:unload()
+ end,
+ [
+ ?TDEF_FE(t_channel_list_default),
+ ?TDEF_FE(t_channel_list_configured),
+ ?TDEF_FE(t_capacity),
+ ?TDEF_FE(t_concurrency),
+ ?TDEF_FE(t_stringify),
+ ?TDEF_FE(t_ignore_db),
+ ?TDEF_FE(t_log_level),
+ ?TDEF_FE(t_allowed_window)
+ ]
+ }.
+
+t_channel_list_default(_) ->
+ ?assertEqual(
+ ["ratio_dbs", "slack_dbs", "upgrade_dbs"],
+ db_channels()
+ ),
+ ?assertEqual(
+ ["ratio_views", "slack_views", "upgrade_views"],
+ view_channels()
+ ),
+ ?assertEqual(
+ ["index_cleanup"],
+ cleanup_channels()
+ ).
+
+t_channel_list_configured(_) ->
+ meck_chans("db_channels", "foo_dbs"),
+ ?assertEqual(
+ ["foo_dbs", "ratio_dbs", "slack_dbs", "upgrade_dbs"],
+ db_channels()
+ ),
+ meck_chans("view_channels", "foo_views"),
+ ?assertEqual(
+ ["foo_views", "ratio_views", "slack_views", "upgrade_views"],
+ view_channels()
+ ),
+ meck_chans("cleanup_channels", "foo_cleanup"),
+ ?assertEqual(
+ ["foo_cleanup", "index_cleanup"],
+ cleanup_channels()
+ ),
+ meck_chans("db_channels", undefined),
+ ?assertEqual(
+ ["ratio_dbs", "slack_dbs", "upgrade_dbs"],
+ db_channels()
+ ),
+ meck_chans("db_channels", ""),
+ ?assertEqual(
+ ["ratio_dbs", "slack_dbs", "upgrade_dbs"],
+ db_channels()
+ ).
+
+t_capacity(_) ->
+ ?assertEqual(9999, capacity("foo")),
+ meck:expect(config, get, fun("smoosh.foo", "capacity", _) -> "2" end),
+ ?assertEqual(2, capacity("foo")).
+
+t_concurrency(_) ->
+ ?assertEqual(1, concurrency("foo")),
+ meck:expect(config, get, fun("smoosh.foo", "concurrency", _) -> "2" end),
+ ?assertEqual(2, concurrency("foo")).
+
+t_stringify(_) ->
+ Flat = fun(L) -> lists:flatten(L) end,
+ ?assertEqual("x index_cleanup", Flat(stringify({?INDEX_CLEANUP, <<"x">>}))),
+ ?assertEqual("x y", Flat(stringify({<<"x">>, <<"y">>}))),
+ ?assertEqual("x", Flat(stringify(<<"x">>))).
+
+t_ignore_db(_) ->
+ ?assertNot(ignore_db(<<"x">>)),
+ ?assertNot(ignore_db({<<"x">>, <<"z">>})),
+ ?assertNot(ignore_db({?INDEX_CLEANUP, <<"x">>})),
+ meck:expect(config, get, fun("smoosh.ignore", "x", _) -> "true" end),
+ ?assert(ignore_db(<<"x">>)),
+ ?assert(ignore_db({<<"x">>, <<"z">>})),
+ ?assert(ignore_db({?INDEX_CLEANUP, <<"x">>})).
+
+t_log_level(_) ->
+ ?assertEqual(notice, log_level("compaction_log", "notice")),
+ meck:expect(config, get, fun("smoosh", "compaction_log", _) -> "error" end),
+ ?assertEqual(error, log_level("compaction_log", "notice")).
+
+t_allowed_window(_) ->
+ ?assert(in_allowed_window("baz")),
+
+ meck_from_to("blah", undefined),
+ ?assert(in_allowed_window("foo")),
+
+ meck_from_to("X:Y", "9.5:Z"),
+ ?assert(in_allowed_window("foo")),
+
+ meck_time(3, 30),
+
+ meck_from_to("02:30", "04:30"),
+ ?assert(in_allowed_window("foo")),
+
+ meck_from_to("3:35", "3:50"),
+ ?assertNot(in_allowed_window("foo")),
+
+ meck_from_to("1:20", "3:29"),
+ ?assertNot(in_allowed_window("foo")),
+
+ meck_from_to("23:30", "3:52"),
+ ?assert(in_allowed_window("foo")),
+
+ meck_from_to("23:30", "3:29"),
+ ?assertNot(in_allowed_window("foo")).
+
+meck_time(H, M) ->
+ meck:expect(calendar, universal_time, 0, {{2000, 10, 10}, {H, M, 42}}).
+
+meck_from_to(From, To) ->
+ meck:expect(config, get, fun
+ ("smoosh.foo", "from", _) -> From;
+ ("smoosh.foo", "to", _) -> To
+ end).
+
+meck_chans(Type, Channels) ->
+ meck:expect(config, get, fun("smoosh", T, _) when T =:= Type -> Channels end).
+
+-endif.
diff --git a/src/smoosh/test/exunit/scheduling_window_test.exs b/src/smoosh/test/exunit/scheduling_window_test.exs
deleted file mode 100644
index 9da4a3150..000000000
--- a/src/smoosh/test/exunit/scheduling_window_test.exs
+++ /dev/null
@@ -1,79 +0,0 @@
-defmodule SmooshSchedulingWindowTest do
- use Couch.Test.ExUnit.Case
-
- setup_all(context) do
- test_ctx = :test_util.start_couch([])
-
- on_exit(fn ->
- :config.delete('smoosh.test_channel', 'from')
- :config.delete('smoosh.test_channel', 'to')
- :test_util.stop_couch(test_ctx)
- end)
-
- context
- end
-
- test "in_allowed_window returns true by default", _context do
- assert :smoosh_utils.in_allowed_window('nonexistent_channel') == true
- end
-
- test "in_allowed_window ignores bad input", _context do
- :config.set('smoosh.test_channel', 'from', 'midnight', false)
- :config.set('smoosh.test_channel', 'to', 'infinity', false)
- assert :smoosh_utils.in_allowed_window('test_channel') == true
- end
-
- test "in_allowed_window returns false when now < from < to", _context do
- now = DateTime.utc_now()
- from = DateTime.add(now, 18_000)
- to = DateTime.add(now, 36_000)
- :config.set('smoosh.test_channel', 'from', '#{from.hour}:#{from.minute}', false)
- :config.set('smoosh.test_channel', 'to', '#{to.hour}:#{to.minute}', false)
- assert :smoosh_utils.in_allowed_window('test_channel') == false
- end
-
- test "in_allowed_window returns true when from < now < to", _context do
- now = DateTime.utc_now()
- from = DateTime.add(now, -18_000)
- to = DateTime.add(now, 18_000)
- :config.set('smoosh.test_channel', 'from', '#{from.hour}:#{from.minute}', false)
- :config.set('smoosh.test_channel', 'to', '#{to.hour}:#{to.minute}', false)
- assert :smoosh_utils.in_allowed_window('test_channel') == true
- end
-
- test "in_allowed_window returns false when from < to < now", _context do
- now = DateTime.utc_now()
- from = DateTime.add(now, -36_000)
- to = DateTime.add(now, -18_000)
- :config.set('smoosh.test_channel', 'from', '#{from.hour}:#{from.minute}', false)
- :config.set('smoosh.test_channel', 'to', '#{to.hour}:#{to.minute}', false)
- assert :smoosh_utils.in_allowed_window('test_channel') == false
- end
-
- test "in_allowed_window returns true when to < from < now", _context do
- now = DateTime.utc_now()
- from = DateTime.add(now, -18_000)
- to = DateTime.add(now, -36_000)
- :config.set('smoosh.test_channel', 'from', '#{from.hour}:#{from.minute}', false)
- :config.set('smoosh.test_channel', 'to', '#{to.hour}:#{to.minute}', false)
- assert :smoosh_utils.in_allowed_window('test_channel') == true
- end
-
- test "in_allowed_window returns false when to < now < from", _context do
- now = DateTime.utc_now()
- from = DateTime.add(now, 18_000)
- to = DateTime.add(now, -18_000)
- :config.set('smoosh.test_channel', 'from', '#{from.hour}:#{from.minute}', false)
- :config.set('smoosh.test_channel', 'to', '#{to.hour}:#{to.minute}', false)
- assert :smoosh_utils.in_allowed_window('test_channel') == false
- end
-
- test "in_allowed_window returns true when now < to < from", _context do
- now = DateTime.utc_now()
- from = DateTime.add(now, 36_000)
- to = DateTime.add(now, 18_000)
- :config.set('smoosh.test_channel', 'from', '#{from.hour}:#{from.minute}', false)
- :config.set('smoosh.test_channel', 'to', '#{to.hour}:#{to.minute}', false)
- assert :smoosh_utils.in_allowed_window('test_channel') == true
- end
-end
diff --git a/src/smoosh/test/exunit/test_helper.exs b/src/smoosh/test/exunit/test_helper.exs
deleted file mode 100644
index 314050085..000000000
--- a/src/smoosh/test/exunit/test_helper.exs
+++ /dev/null
@@ -1,2 +0,0 @@
-ExUnit.configure(formatters: [JUnitFormatter, ExUnit.CLIFormatter])
-ExUnit.start()