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

Remove the check which compares Mnesia directory with the value set in the app env when deleting Mnesia files #1904

Merged
merged 1 commit into from
Jun 7, 2018

Conversation

arkgil
Copy link
Contributor

@arkgil arkgil commented Jun 5, 2018

This patch removes the check if Mnesia directory reported by mnesia:system_info(directory) is the same as application:get_env(mnesia, dir) when deleting Mnesia files.

The check sometimes resulted in unexpected errors. Users set Mnesia directory path using application env mnesia.dir. This path might be a relative path (it might be even an atom!). When Mnesia starts, it takes this value, converts it to absolute path and stored it in initial config. This is what mnesia:system_info(directory) returns, thus the check failed in simple situations when someone sets mnesia.dir to relative path. The check only succeeded in cases when mnesia.dir path is a normalized, absolute path, or when Mnesia is stopped, in which caes mnesia:system_info(directory) falls back to application env's values.

@erszcz
Copy link
Member

erszcz commented Jun 5, 2018

I'm OK with removing the check.

Originally, the check was introduced since (not knowing it) I didn't want to program for the implementation of mnesia:system_info(directory). Since we now know it falls back to application:get_env, it seems that we are pretty safe.

Eventually, we might just log the situation on a high verbosity level instead of throwing an error, so that a human operator knows about a potential misconfiguration.

This patch removes the part of code which raises an error if Mnesia
directory reported by `mnesia:system_info(directory)` is the same as
`application:get_env(mnesia, dir)` when deleting Mnesia files. From
now on only a warning message is emitted.

The check sometimes resulted in unexpected errors. Users set Mnesia
directory path using application env `mnesia.dir`. This path might be a
relative path (it might be even an atom!). When Mnesia starts, it takes
this value, converts it to absolute path and stored it in initial
config. This is what `mnesia:system_info(directory)` returns, thus the
check failed in simple situations when someone sets `mnesia.dir` to
relative path. The check only succeeded in cases when `mnesia.dir` path
is a normalized, absolute path.
@arkgil arkgil force-pushed the mnesia-dir-inconsistent-err-fix branch from 360ff82 to cb29dd4 Compare June 6, 2018 12:30
@codecov
Copy link

codecov bot commented Jun 6, 2018

Codecov Report

Merging #1904 into master will increase coverage by 0.99%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1904      +/-   ##
==========================================
+ Coverage   72.63%   73.63%   +0.99%     
==========================================
  Files         307      310       +3     
  Lines       28140    30863    +2723     
==========================================
+ Hits        20440    22725    +2285     
- Misses       7700     8138     +438
Impacted Files Coverage Δ
src/mongoose_cluster.erl 72.97% <0%> (-1%) ⬇️
src/cassandra/mongoose_cassandra_sup.erl 64.28% <0%> (-32.02%) ⬇️
src/mod_shared_roster_ldap.erl 37% <0%> (-21.67%) ⬇️
src/ejabberd_auth_ldap.erl 39.66% <0%> (-19.84%) ⬇️
src/mod_mam_meta.erl 84.52% <0%> (-10.56%) ⬇️
src/mongoose_subhosts.erl 86.66% <0%> (-6.67%) ⬇️
src/ejabberd_sup.erl 94.11% <0%> (-5.89%) ⬇️
src/mod_muc_light_utils.erl 84.21% <0%> (-4.22%) ⬇️
src/ejabberd_app.erl 81.74% <0%> (-3.32%) ⬇️
src/mod_offline.erl 74.41% <0%> (-3.24%) ⬇️
... and 44 more

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 33db205...cb29dd4. Read the comment docs.

@arkgil
Copy link
Contributor Author

arkgil commented Jun 6, 2018

@erszcz I've added a warning log message if directories are not the same. I think that's a sweet spot between raising an error and not checking it at all.

Copy link
Member

@erszcz erszcz left a comment

Choose a reason for hiding this comment

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

Looks good to me!

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

@arcusfelis arcusfelis merged commit 006f096 into master Jun 7, 2018
@fenek fenek added this to the 3.0.0++ milestone Jun 21, 2018
@fenek fenek deleted the mnesia-dir-inconsistent-err-fix branch August 23, 2018 15:29
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.

4 participants