Fix incorrect depdencies (RIAK-2128)

[lehoff]

Done for 2.1.1 but should work for all Riak versions.

[angrycub]

Why add lager as a dependency, when it is listed in the README as a progressive enhancement avenue?

[lehoff]

I must be reading the wrong version of the README file because I don't have that reference to being "a progressive enhancement avenue". Which branch is that on?

In general it is not good for one application to rely on others to bring in applications.
My preferred solution here would be to include lager as a dependency and then add a configuration option to decide if lager should be used.

My rationale is that if we forget that we should check for lager being present then we will not detect a problem before runtime, so we have to remember that in the function get_limits/0 (https://github.com/basho/cluster_info/blob/develop/src/cluster_info.erl#L331) we check if lager is present before doing calls and then remember to do check again.
I'd much rather make this explicit.

If we want to keep the dynamic behaviour I think we should change the check https://github.com/basho/cluster_info/blob/develop/src/cluster_info.erl#L334 to:

case is_application_running(lager) of 
...

is_application_running(App) ->
    lists:keymember(App, 1, application:which_applications()).

If lager is merely loaded then code:which(lager_trunc_io) will return and then a call will be made to lager when it is not running. Calling application:which_applications/0 ensures that lager is running.

[angrycub]

@lehoff, I totally agree that reworking the check to use which_applications() is a better version of the progressive enhancement behavior. I think that it's better to not strictly require lager as a dependency of the project itself, but to take advantage of it if we see it. I think the main intent of cluster_info was to be included in a project as a dependency (which would explain the use of lager as an enhancement). It's just a bonus that it can run as a standalone application. This choice enables someone to not have to bring lager into the mix just to add cluster_info to their application.

I totally +1 your suggested change in the check though! That shape would allow for other progressive enhancements depending on the applications running in the VM without having to make them strict dependencies.

[angrycub]

Just reread your comment after posting mine, and realized that you might be talking about a build target that included lager for testing purposes, in which case, I can agree to that as well.

My goal is to make sure that any additional dependencies are absolutely essential to the core functionality, able to be configured in or out at build time with no assumptions being made about their presence in any particular end product (all calls would need to be guarded).

I can certainly see a benefit to having a way to include the applications for eunit testing the "enhanced" behaviors.

[slfritchie]

My opinions, as the original author but no longer sole maintainer.

• It would be nice to the rest of the open source world if later were optional. We Bashonians use lager for almost everything, but not everyone does.
• I agree that the dynamic check for the lager formatting functions would be helped by @lehoff's suggestion.

[lehoff]

There is one major problem with use-if-present: it totally ruins OTP releases when it comes to upgrades and downgrades.

A secondary problem is that dependency management issues is then something that happens at runtime. If the installed version of an use-if-present app does not work with the code written then there will be no alarm bells going off before runtime.

And using lager in cluster_info would not force users of cluster_info to use lager in the rest of their application. We get edown in through the backdoor in numerous of our dependencies as it is today.

[lehoff]

Just had a very good discussion with @angrycub about cluster_info and here is what we suggest.

Given that cluster_info is a forensic type of application it would be better if we moved riak_core_cinfo_core out of riak_core, riak_kv and riak_ee all together.
The same should be done for riak_kv_cinfo, riak_pipe_cinfo and riak_repl_cinfo.

We could create a riak_cluster_info application that encompasses all the cinfo files and only activate the relevant ones based on what kind of system the cluster_info application is pointed at.

This would make the use-if-present behaviour a natural thing to have. cluster_info could be as clever as allowed by running applications on the system that it is investigating.

cluster_info could also be modified to run on a hidden node while doing its investigation. Thereby separating it from the VM that it is inspecting. This is a safer way of digging into a running system.

And having cluster_info on the side also allows us to innovate faster based on requirements from our customers.

lager should not be made a dependency of cluster_info, but it should be clearly documented which applications cluster_info is looking for and will take advantage of when present.

If this idea gets traction this issue/PR should be ignored and then we should have a Jira issue describing the refactoring of the cinfo files into their own app.

[slfritchie]

Hi, Torben & Charlie, sorry about the delay. Your proposal sounds fine with me, 👍

[jonmeredith]

Maybe I'm not understanding, I thought the value of having the cinfo files
in the repos themselves was that the cluster info became composable as you
used more repositories.

If somebody made fooapp that used riak_core+foo then would they need to
fork and modify the repository containing the riak cinfo to remove 'kv'
stuff and add 'foo' stuff.

The hidden node stuff sounds safer, anything that runs node-side via RPC is
going to have all of the same risks of memory/lock consumption.

On Mon, Aug 31, 2015 at 2:38 PM Torben Hoffmann [email protected]
wrote:

Just had a very good discussion with @angrycub
https://github.com/angrycub about cluster_info and here is what we
suggest.

Given that cluster_info is a forensic type of application it would be
better if we moved riak_core_cinfo_core out of riak_core, riak_kv and
riak_ee all together.
The same should be done for riak_kv_cinfo, riak_pipe_cinfo and
riak_repl_cinfo.

We could create a riak_cluster_info application that encompasses all the
cinfo files and only activate the relevant ones based on what kind of
system the cluster_info application is pointed at.

This would make the use-if-present behaviour a natural thing to have.
cluster_info could be as clever as allowed by running applications on the
system that it is investigating.

cluster_info could also be modified to run on a hidden node while doing
its investigation. Thereby separating it from the VM that it is inspecting.
This is a safer way of digging into a running system.

And having cluster_info on the side also allows us to innovate faster
based on requirements from our customers.

lager should not be made a dependency of cluster_info, but it should be
clearly documented which applications cluster_info is looking for and
will take advantage of when present.

If this idea gets traction this issue/PR should be ignored and then we
should have a Jira issue describing the refactoring of the cinfo files into
their own app.

—
Reply to this email directly or view it on GitHub
#24 (comment).