Use gen_mod opts in MAM #1627

kzemek · 2018-01-08T10:43:35Z

This PR removes dynamically compiled mod_mam_prefs and mod_mam_muc_prefs modules in favor of fetching module options with ETS via gen_mod:get_module_opts() and then searching for the right option with lists:keyfind.

kzemek · 2018-01-15T14:02:45Z

Before

After

kzemek · 2018-01-15T14:03:53Z

Closeup on the TTD graph after the main (~2s) peak.

Before

After

kzemek · 2018-01-15T14:06:35Z

Closeup on the TTD graph after the second (~80ms) peak.

Before

After

arcusfelis

I've added comments

arcusfelis · 2018-01-17T15:01:32Z

src/gen_mod.erl

@@ -302,6 +303,20 @@ set_module_opt(Host, Module, Opt, Value) ->
                               {#ejabberd_module.opts, Updated})
    end.

+%% @doc Non-atomic! You have been warned.
+-spec set_module_opts(ejabberd:server(), module(), _Opts) -> boolean().


It's a new function.
Can we have a test that checks that the function works correctly?
Description is nice to have too, i.e. how it works, what is Opts and what is the result.

Does it replace all module opts or appends opts? - it's what I want to know from description

arcusfelis · 2018-01-17T15:06:35Z

test.disabled/ejabberd_tests/tests/mam_SUITE.erl

@@ -813,8 +815,7 @@ init_state(C, muc_light, Config) ->
 init_state(_C, prefs_cases, Config) ->
    Config;
 init_state(_C, policy_violation, Config) ->
-    rpc_apply(mod_mam, set_params,
-              [ [{max_result_limit, 5}] ]),
+    rpc_apply(gen_mod, set_module_opt, [host(), mod_mam, max_result_limit, 5]),


can we put this call into a helper module?

I prefer this to be explicit since there's some magic going on with external option setting.

arcusfelis · 2018-01-17T15:08:23Z

src/mod_mam_utils.erl

@@ -1062,3 +1066,30 @@ error_on_sql_error(_HostOrConn, _Query, Result) ->
 -spec wrapper_id() -> binary().
 wrapper_id() ->
    uuid:uuid_to_string(uuid:get_v4(), binary_standard).
+
+param(Module, Host, Opt, Default) ->


specs for each exported function.

arcusfelis · 2018-01-17T15:09:17Z

src/mod_mam_utils.erl

+add_archived_element(Module, Host) ->
+    param(Module, Host, add_archived_element, false).
+
+%% -spec add_stanzaid_element(Host :: ejabberd:lserver()) -> boolean().


spec commented? why?

arcusfelis · 2018-01-17T15:14:12Z

src/mod_mam_muc.erl


-    Format =
-        io_lib:format(
-          "-module(mod_mam_muc_params).~n"


I like the param module, because I can get all MAM related settings without thinking for default value of each option.
Can we have a function, that returns all possible MAM parameters with current values?

Something like

mod_mam:get_parameters(Host) -> [{add_archived_element, false}, {default_result_limit, 50}, ....]

Same for mod_mam_muc.

I like the param module, because I can get all MAM related settings without thinking for default value of each option.

True, that's something to improve.

Can we have a function, that returns all possible MAM parameters with current values?

What would be the point of such function?

What would be the point of such function?

For checking runtime configuration of the module.
For checking what options are available to override.

I'm hesitant to implement such functionality for MAM modules. If we want more explicit support for defaults, we should add it to gen_mod.

Obviously sans defaults, the runtime config should just be fetched with gen_mod:get_module_opts

arcusfelis · 2018-01-17T15:21:39Z

src/mod_mam_muc.erl

    ?DEBUG("Incoming room packet.", []),
-    IsArchivable = mod_mam_muc_params:is_archivable_message(?MODULE, incoming, Packet),


I can't see now that we are using function that is configuration depended.
I would rather keep mod_mam_muc_params, by making a real mod_mam_muc_params module.
But of course, you can call gen_mod from inside this new module.

mod_mam_utils is supposed to have functions, that would always work the same predictable way, not depending on configuration parameters.

I can't see now that we are using function that is configuration depended.

If not religiously adhering to dependency injection by parameters, when can you see whether called function is or is not configuration dependent?

mod_mam_utils is supposed to have functions, that would always work the same predictable way, not depending on configuration parameters.

Why? I see mod_mam_utils as the place for common utility functions of mod_mam and mod_mam_muc. These new functions are not even the first ones there to call gen_mod:get_module_opt.

utils - functions that do something.
params - functions that are allowed to override.
I don't like god-modules, we would end-up with another c2s or muc_room.

I disagree that this is a way for mod_mam_utils to become a god module, but I agree that a separation into mod_mam_params would be beneficial for code clarity.

arcusfelis · 2018-01-23T13:24:56Z

src/mod_mam_muc.erl

+extra_params_module(Host) ->
+    mod_mam_utils:param(?MODULE, Host, extra_lookup_params, undefined).
+
+max_result_limit(Host) ->


can we add specs for calls like this?

arcusfelis · 2018-01-24T14:43:50Z

test.disabled/ejabberd_tests/tests/mam_SUITE.erl

@@ -603,11 +603,14 @@ end_per_group(G, Config) when G == rsm_all; G == mam_purge; G == nostore;
      Config;
 end_per_group(muc_configurable_archiveid, Config) ->
    ParamsB = proplists:get_value(params_backup, Config),
-    rpc_apply(mod_mam_muc, set_params, [ParamsB]),
+    lists:foreach(fun({Key, Value}) ->


It screams to add gen_mod:set_module_opts/3 function.

arcusfelis · 2018-01-24T14:45:10Z

test.disabled/ejabberd_tests/tests/mam_SUITE.erl

 init_per_group(configurable_archiveid, Config) ->
-    [{params_backup, rpc_apply(mod_mam_params, params, [])} | Config];
+    [{params_backup, rpc_apply(gen_mod, get_module_opts, [host(), mod_mam])} | Config];


the whole backup mech should probably replaces with save/restore params.

fenek

Not all MAM modules have been moved to new folder. Please either move them or cancel the relocation in this PR.

fenek · 2018-01-24T22:44:01Z

src/mam/mod_mam_params.erl

+
+-spec has_full_text_search(Module :: mod_mam | mod_mam_muc, Host :: ejabberd:server()) -> boolean().
+has_full_text_search(Module, Host) ->
+    gen_mod:get_module_opt(Host, Module, full_text_search, true).


Why param/4 is not reused here?

fenek · 2018-01-24T22:54:31Z

src/mam/mod_mam_muc.erl

-    mod_mam_muc_params:add_stanzaid_element().
+-spec is_archivable_message(Host :: ejabberd:lserver(), Dir :: incoming | outgoing,
+                            Packet :: exml:element()) -> boolean().
+is_archivable_message(Host, Dir, Packet) ->


Maybe it should be extracted to mod_mam_utils? I believe it is a duplicate from mod_mam.

@arcusfelis wanted to have param-driven functions out of mod_mam_utils and I sort-of agree. Choosing parameters went to mod_mam_params, actual logic went to mod_mam_utils and a convenience shim connecting them went to mam modules. Duplication is a pity here, but it stems more from duplication of other parts of mod_mam/mod_mam_muc.

OK, I understand. May be a subject of refactoring in the future then, maybe there will be a dedicated module for param-driven MAM utils.

fenek · 2018-01-24T22:55:44Z

src/mam/mod_mam_muc.erl

+is_archivable_message(Host, Dir, Packet) ->
+    {M, F} = mod_mam_params:is_archivable_message_fun(?MODULE, Host),
+    ArchiveChatMarkers = mod_mam_params:archive_chat_markers(?MODULE, Host),
+    erlang:apply(M, F, [?MODULE, Dir, Packet, ArchiveChatMarkers]).


This change implies docs update:
https://github.com/esl/MongooseIM/blob/master/doc/modules/mod_mam.md#options
https://github.com/esl/MongooseIM/blob/master/doc/modules/mod_mam.md#is_archivable_message
https://github.com/esl/MongooseIM/blob/master/doc/modules/mod_mam.md#archiving-chat-markers

How so? No logic has changed

kzemek · 2018-01-25T16:28:45Z

Not all MAM modules have been moved to new folder. Please either move them or cancel the relocation in this PR.

@fenek done

fenek · 2018-01-25T16:34:37Z

@kzemek

Thanks! Only docs update remains. :)

fenek · 2018-01-25T16:44:11Z

Apologies, docs became outdated before this PR!

kzemek added the WIP 🚧 label Jan 8, 2018

kzemek force-pushed the mam_get_opt branch 3 times, most recently from 33a626f to 31b24aa Compare January 15, 2018 12:37

kzemek force-pushed the mam_get_opt branch from 31b24aa to a685bb2 Compare January 15, 2018 14:54

kzemek added waiting-for-review and removed WIP 🚧 labels Jan 15, 2018

kzemek requested a review from fenek January 15, 2018 15:16

arcusfelis requested changes Jan 17, 2018

View reviewed changes

kzemek force-pushed the mam_get_opt branch from 42fd92f to efb1918 Compare January 19, 2018 10:25

fenek force-pushed the master branch from 7ab86d0 to 265e11b Compare January 22, 2018 08:27

fenek force-pushed the mam_get_opt branch from efb1918 to b80dc88 Compare January 22, 2018 16:25

arcusfelis reviewed Jan 23, 2018

View reviewed changes

kzemek added 2 commits January 23, 2018 14:31

Use gen_mod opts in MAM.

9765c6f

Improve visibility of mam options' defaults.

884fd4a

kzemek force-pushed the mam_get_opt branch from b80dc88 to 4250de9 Compare January 23, 2018 14:28

arcusfelis reviewed Jan 24, 2018

View reviewed changes

arcusfelis approved these changes Jan 24, 2018

View reviewed changes

arcusfelis changed the title ~~Use gen_mod opts in MAM.~~ Use gen_mod opts in MAM Jan 24, 2018

fenek requested changes Jan 24, 2018

View reviewed changes

fenek added WIP 🚧 and removed waiting-for-review labels Jan 24, 2018

Move MAM params to new module mod_mam_params.

4b200fe

kzemek force-pushed the mam_get_opt branch from 4250de9 to 4b200fe Compare January 25, 2018 16:15

fenek approved these changes Jan 25, 2018

View reviewed changes

fenek added ready and removed WIP 🚧 labels Jan 25, 2018

fenek added this to the 3.0.0 milestone Jan 25, 2018

fenek merged commit b8326b8 into master Jan 25, 2018

fenek deleted the mam_get_opt branch January 25, 2018 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use gen_mod opts in MAM #1627

Use gen_mod opts in MAM #1627

kzemek commented Jan 8, 2018 •

edited

Loading

kzemek commented Jan 15, 2018

kzemek commented Jan 15, 2018

kzemek commented Jan 15, 2018

arcusfelis left a comment

arcusfelis Jan 17, 2018

arcusfelis Jan 17, 2018

arcusfelis Jan 17, 2018

kzemek Jan 18, 2018

arcusfelis Jan 17, 2018

arcusfelis Jan 17, 2018

arcusfelis Jan 17, 2018

kzemek Jan 17, 2018

arcusfelis Jan 22, 2018

kzemek Jan 23, 2018

kzemek Jan 23, 2018

arcusfelis Jan 17, 2018

kzemek Jan 17, 2018

arcusfelis Jan 22, 2018

kzemek Jan 23, 2018

kzemek Jan 23, 2018

arcusfelis Jan 23, 2018

arcusfelis Jan 24, 2018

arcusfelis Jan 24, 2018

fenek left a comment

fenek Jan 24, 2018

kzemek Jan 25, 2018

fenek Jan 24, 2018

kzemek Jan 25, 2018

fenek Jan 25, 2018

fenek Jan 24, 2018 •

edited

Loading

kzemek Jan 25, 2018

kzemek commented Jan 25, 2018

fenek commented Jan 25, 2018

fenek commented Jan 25, 2018

		?DEBUG("Incoming room packet.", []),
		IsArchivable = mod_mam_muc_params:is_archivable_message(?MODULE, incoming, Packet),

Use gen_mod opts in MAM #1627

Use gen_mod opts in MAM #1627

Conversation

kzemek commented Jan 8, 2018 • edited Loading

kzemek commented Jan 15, 2018

Before

After

kzemek commented Jan 15, 2018

Before

After

kzemek commented Jan 15, 2018

Before

After

arcusfelis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenek Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzemek commented Jan 25, 2018

fenek commented Jan 25, 2018

fenek commented Jan 25, 2018

kzemek commented Jan 8, 2018 •

edited

Loading

fenek Jan 24, 2018 •

edited

Loading