Simplify MAM configuration #1112

kzemek · 2016-12-08T16:19:54Z

Configuration is simplified by introducing a new module, mod_mam_meta, that only parses its arguments and pulls in relevant MAM modules using deps/2 callback introduced in #1103. This way a unified configuration may be introduced without disrupting existing modules and providing full backwards compatibility with old configs.

gadgetci · 2016-12-08T16:19:59Z

apps/ejabberd/src/ejabberd_config.erl

-      fun({Module, _Args}) ->
-              gen_mod:stop_module(Host, Module)
-      end, Modules);
+    lists:foreach(fun({Mod, _}) -> gen_mod:stop_module(Host, Mod) end, Modules);
 handle_local_hosts_config_del({{Key,_}, _} =El) ->


According to Elvis:

Missing space right "," on line 1079

gadgetci · 2016-12-08T16:20:00Z

apps/ejabberd/src/ejabberd_config.erl

@@ -1114,8 +1108,7 @@ handle_local_hosts_config_change({{auth, Host}, OldVals, _}) ->
 handle_local_hosts_config_change({{ldap, Host}, _OldConfig, NewConfig}) ->
    ok = ejabberd_hooks:run_fold(host_config_update, Host, ok, [Host, ldap, NewConfig]);
 handle_local_hosts_config_change({{modules,Host}, OldModules, NewModules}) ->
-    Res = compare_modules(OldModules, NewModules),
-    reload_modules(Host, Res);
+    gen_mod_deps:replace_modules(Host, OldModules, NewModules);
 handle_local_hosts_config_change({{Key,_Host},_Old,_New} = El) ->


According to Elvis:

Missing space right "," on line 1112

gadgetci · 2016-12-08T16:20:01Z

apps/ejabberd/src/ejabberd_config.erl

-      fun({Module, Args}) ->
-              gen_mod:start_module(Host, Module, Args)
-      end, Modules);
+    gen_mod_deps:start_modules(Host, Modules);
 handle_local_hosts_config_add({{Key,_Host}, _} = El) ->


According to Elvis:

Missing space right "," on line 1054

gadgetci · 2016-12-08T16:20:02Z

apps/ejabberd/src/ejabberd_config.erl

@@ -1114,8 +1108,7 @@ handle_local_hosts_config_change({{auth, Host}, OldVals, _}) ->
 handle_local_hosts_config_change({{ldap, Host}, _OldConfig, NewConfig}) ->
    ok = ejabberd_hooks:run_fold(host_config_update, Host, ok, [Host, ldap, NewConfig]);
 handle_local_hosts_config_change({{modules,Host}, OldModules, NewModules}) ->


According to Elvis:

Missing space right "," on line 1110

gadgetci · 2016-12-08T16:20:03Z

apps/ejabberd/src/ejabberd_config.erl

@@ -1114,8 +1108,7 @@ handle_local_hosts_config_change({{auth, Host}, OldVals, _}) ->
 handle_local_hosts_config_change({{ldap, Host}, _OldConfig, NewConfig}) ->
    ok = ejabberd_hooks:run_fold(host_config_update, Host, ok, [Host, ldap, NewConfig]);
 handle_local_hosts_config_change({{modules,Host}, OldModules, NewModules}) ->
-    Res = compare_modules(OldModules, NewModules),
-    reload_modules(Host, Res);
+    gen_mod_deps:replace_modules(Host, OldModules, NewModules);
 handle_local_hosts_config_change({{Key,_Host},_Old,_New} = El) ->


According to Elvis:

Missing space right "," on line 1112

gadgetci · 2016-12-08T16:20:04Z

apps/ejabberd/src/ejabberd_config.erl

@@ -1114,8 +1108,7 @@ handle_local_hosts_config_change({{auth, Host}, OldVals, _}) ->
 handle_local_hosts_config_change({{ldap, Host}, _OldConfig, NewConfig}) ->
    ok = ejabberd_hooks:run_fold(host_config_update, Host, ok, [Host, ldap, NewConfig]);
 handle_local_hosts_config_change({{modules,Host}, OldModules, NewModules}) ->
-    Res = compare_modules(OldModules, NewModules),
-    reload_modules(Host, Res);
+    gen_mod_deps:replace_modules(Host, OldModules, NewModules);
 handle_local_hosts_config_change({{Key,_Host},_Old,_New} = El) ->


According to Elvis:

Missing space right "," on line 1112

fenek · 2016-12-08T20:47:33Z

doc/modules/mod_mam.md

-If you haven't chosen any of the above, skip the next part.
+* **backend** (atom, default: `odbc`) - Database backend to use. `odbc`, `riak` and `cassandra` are supported.
+* **add_archived_element** (atom, default: `false`) - Add `<archived/>` element from MAM v0.2.
+* **is_complete_message** (module, default: `mod_mam_utils`) - Name of a module implementing [`is_complete_message/3` callback](#is_complete_message) that determines if the message should be archived.


Maybe let's refactor this to is_archivable_message, because complete doesn't really makes sense IMO.

michalwski · 2016-12-12T11:08:43Z

apps/ejabberd/src/mod_mam_meta.erl

+    Deps = add_dep(mod_mam_odbc_user, [Type], Deps1),
+
+    lists:foldl(
+      fun


Extracting this fun to a function will most probably improve readability here.

gadgetci · 2016-12-12T11:58:13Z

apps/ejabberd/src/mod_mam_muc.erl

-
+call_is_archivable_message(Module, Dir, Packet) ->
+    M = mod_mam_muc_params:is_archivable_message(),
+    M:is_archivable_message(Module, Dir, Packet).


According to Elvis:

Remove the dynamic function call on line 985. Only modules that define callbacks should make dynamic calls.

gadgetci · 2016-12-12T11:58:13Z

apps/ejabberd/src/mod_mam.erl

-    M:is_complete_message(Module, Dir, Packet).
+is_archivable_message(Module, Dir, Packet) ->
+    M = mod_mam_params:is_archivable_message(),
+    M:is_archivable_message(Module, Dir, Packet).


According to Elvis:

Remove the dynamic function call on line 1015. Only modules that define callbacks should make dynamic calls.

gadgetci · 2016-12-12T13:00:43Z

apps/ejabberd/src/mod_mam_muc.erl

-
+call_is_archivable_message(Module, Dir, Packet) ->
+    M = mod_mam_muc_params:is_archivable_message(),
+    M:is_archivable_message(Module, Dir, Packet).


According to Elvis:

Remove the dynamic function call on line 987. Only modules that define callbacks should make dynamic calls.

gadgetci · 2016-12-12T15:34:57Z

apps/ejabberd/src/mod_mam_muc.erl

-
+call_is_archivable_message(Module, Dir, Packet) ->
+    M = mod_mam_muc_params:is_archivable_message(),
+    M:is_archivable_message(Module, Dir, Packet).


According to Elvis:

Remove the dynamic function call on line 995. Only modules that define callbacks should make dynamic calls.

gadgetci · 2016-12-12T15:37:43Z

apps/ejabberd/src/mod_mam_muc.erl

-    M = mod_mam_muc_params:is_complete_message(),
-    M:is_complete_message(Module, Dir, Packet).
-
+call_is_archivable_message(Module, Dir, Packet) ->


According to Compiler:

Warning: function call_is_archivable_message/3 is unused

Configuration is simplified by introducing a new module, mod_mam_meta, that only parses its arguments and pulls in relevant MAM modules using deps/2 callback. This way a unified configuration may be introduced without disrupting existing modules and providing full backwards compatibility with old configs.

gadgetci reviewed Dec 8, 2016

View reviewed changes

kzemek changed the title ~~Simplify mam configuration~~ Simplify MAM configuration Dec 8, 2016

fenek reviewed Dec 8, 2016

View reviewed changes

michalwski reviewed Dec 12, 2016

View reviewed changes

kzemek force-pushed the simplify-mam-configuration branch from 6833e15 to f0b55d8 Compare December 12, 2016 11:58

gadgetci reviewed Dec 12, 2016

View reviewed changes

kzemek added the ready label Dec 12, 2016

kzemek force-pushed the simplify-mam-configuration branch from f0b55d8 to 1556966 Compare December 12, 2016 13:00

gadgetci reviewed Dec 12, 2016

View reviewed changes

kzemek force-pushed the simplify-mam-configuration branch from 1556966 to a56c567 Compare December 12, 2016 15:34

gadgetci reviewed Dec 12, 2016

View reviewed changes

kzemek force-pushed the simplify-mam-configuration branch from a56c567 to 6d75ad6 Compare December 12, 2016 15:36

gadgetci reviewed Dec 12, 2016

View reviewed changes

kzemek added 4 commits December 13, 2016 15:33

Refactor MAM option is_complete_message to is_archivable_message.

cfd58c3

Add new MAM configuration to example ejabberd.cfg .

a0f9cca

Improve mod_mam_meta code style.

73704e2

kzemek force-pushed the simplify-mam-configuration branch from 6d75ad6 to 73704e2 Compare December 13, 2016 14:35

michalwski merged commit e7bd692 into master Dec 14, 2016

michalwski deleted the simplify-mam-configuration branch December 14, 2016 08:13

kzemek mentioned this pull request Dec 22, 2016

Simplified MAM configuration #673

Closed

This was referenced Jan 20, 2017

MongooseIM 2.0.1 #1160

Merged

MongooseIM 2.0.1 #1164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify MAM configuration #1112

Simplify MAM configuration #1112

kzemek commented Dec 8, 2016 •

edited

Loading

gadgetci Dec 8, 2016

gadgetci Dec 8, 2016

gadgetci Dec 8, 2016

gadgetci Dec 8, 2016

gadgetci Dec 8, 2016

gadgetci Dec 8, 2016

fenek Dec 8, 2016

michalwski Dec 12, 2016

gadgetci Dec 12, 2016

gadgetci Dec 12, 2016

gadgetci Dec 12, 2016

gadgetci Dec 12, 2016

gadgetci Dec 12, 2016

Simplify MAM configuration #1112

Simplify MAM configuration #1112

Conversation

kzemek commented Dec 8, 2016 • edited Loading

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

kzemek commented Dec 8, 2016 •

edited

Loading