Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let reload_cluster not check some config values #1752

Closed
wants to merge 23 commits into from

Conversation

kanes115
Copy link
Contributor

@kanes115 kanes115 commented Mar 5, 2018

Right now we just filter this option out of config when calculating sha with ejabberd_config:filter_out_node_specific_endpoints/1 called in compute_config_file_version/1 and compute_config_version/1. In the future it should probably be more general (if the number of such different-among-nodes options increase).

filter_out_gd_endpoints([Opt | Opts]) ->
[Opt | filter_out_gd_endpoints(Opts)].

delete_path_in_proplist(Plist, [Step]) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fenek, if you know of any function in stdlib that could replace this definition (that is a solution for a very generic problem of nested proplists), let me know - I haven't found one.

@codecov-io
Copy link

codecov-io commented Mar 6, 2018

Codecov Report

Merging #1752 into master will decrease coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1752      +/-   ##
==========================================
- Coverage   74.47%   74.45%   -0.03%     
==========================================
  Files         290      290              
  Lines       26931    26947      +16     
==========================================
+ Hits        20058    20064       +6     
- Misses       6873     6883      +10
Impacted Files Coverage Δ
src/ejabberd_config.erl 64.57% <75%> (+0.57%) ⬆️
...rc/global_distrib/mod_global_distrib_transport.erl 47.05% <0%> (-5.89%) ⬇️
src/mod_mam_muc_odbc_async_pool_writer.erl 70.73% <0%> (-3.26%) ⬇️
...bal_distrib/mod_global_distrib_hosts_refresher.erl 84.44% <0%> (-2.23%) ⬇️
src/global_distrib/mod_global_distrib_utils.erl 78.57% <0%> (-1.03%) ⬇️
src/mod_muc_log.erl 78.2% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43ebd25...54fb436. Read the comment docs.

fenek
fenek previously approved these changes Mar 6, 2018
@fenek
Copy link
Member

fenek commented Mar 6, 2018

Let's confirm that it works on dev machine, as there are no reload_cluster tests at all. They should be added as a separate task.

% between nodes in cluster. Each option is expressed as
% a list of keys in proplist that should be deleted (if such
% path exists). The `root path` for them is a list of modules.
node_specific_module_options() ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, can this path go into ejabberd.cfg itself?
I don't like it to be hardcoded.
It feels extremely installation-specific. For different clients we can have different per-node parameters.

I even think there would be installations with the same endpoints for all nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's not too meta. These hardcoded are the options that MIGHT be different and we don't require them to be the same (but they might be). It is not user's role to define them, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO can be done as a separate task. This PR solves immediate issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kanes115 I agree with @arcusfelis. It wouldn't be too meta and we already have config options only for advanced users, so why not affect the reload_cluster logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I agree

Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for now, because it is a priority ha... fix, but we should go more meta later in a separate PR.
And we need tests for reload_cluster in a separate PR.


filter_out_node_specific_options([]) ->
[];
filter_out_node_specific_options([{local_config, {modules, Host}, Mods} | Opts]) -> % use record
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use record here in pattern matching

Dominik Stanaszek added 2 commits March 20, 2018 11:57
@kanes115 kanes115 changed the title Let reload_cluster not check mod_global_distrib -> connections -> endpoints value Let reload_cluster not check some config values Mar 20, 2018
@fenek fenek added WIP 🚧 and removed ready labels Apr 3, 2018
@fenek fenek dismissed their stale review April 3, 2018 17:49

A customer reported a bug in this branch.

@michalwski
Copy link
Contributor

@kanes115 @fenek can we close this PR? I understand the issue was solved by #1948. Is that right?

@michalwski michalwski closed this Jul 6, 2018
@michalwski michalwski deleted the improve_reload_cluster_v2 branch July 6, 2018 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants