Fix naming asymmetry in ovis_log_register()/ovis_log_destroy()

[morrone]

There is a naming asymmetry in the ovis log API:

• ovis_log_register()
• ovis_log_destroy()

We should rename ovis_log_destroy() to something like ovis_log_deregister().

[tom95858]

@nichamon, this sounds fine to me, what do you think?

[baallan]

Could we make some consistency like _new/_delete all across ldms in a v5 transition? the 4.x releases already have this and 'renames for prettiness' break other peoples development trees.

[tom95858]

Renaming functions does not affect compatibility. We can change these names whenever we choose. Github is everyone's development tree.

[baallan]

Renaming plugin api functions immediately breaks new plugin development branches which run anywhere from 3 months to a year in lifespan and frustrates new developers. While I'm a big fan of consistent naming practices, we should batch disruptive changes like this that don't actually provide any new functionality to major release boundaries.

[tom95858]

"Whenever we choose" may include criteria for batching as you suggest, so developers only have to rebase once. I'm simply pointing out that this does not/should not affect wire compatibility with earlier releases.

[tom95858]

@morrone, @baallan please propose the changes you prefer. Currently we have a mix of add/remove, add/delete, new/free, new/delete, etc... I think @morrone has suggested that he does not prefer the use of new/del as these terms have implied meanings in the C++ namespace.

[nichamon]

@nichamon, this sounds fine to me, what do you think?

I agree with @morrone to change ovis_log_destroy() to ovis_log_unregister(). I'll create a pull request when I have the patch.

For the other function names, I'll wait for a proposal regarding what function names people think should be changed and what the new names should be.

[nichamon]

@tom95858 @morrone I looked into the code. Renaming ovis_log_destroy() to ovis_log_unregister() will affect all plugins because they call ovis_log_destroy() in term(). I think I'll wait for the final decision on what we want to do in #1344, in the case that we decide to change the load interface. That will also affect all plugins.

[tom95858]

@nichamon I think we should consider having a base_sampler_term() function that has this function in it + some other base sampler cleanup. Same thing with setup, then we can "normalize" the log naming and termination logic.

[morrone]

@nichamon I think we should consider having a base_sampler_term() function that has this function in it + some other base sampler cleanup. Same thing with setup, then we can "normalize" the log naming and termination logic.

I think the base_sampler_* stuff is still a bit too far from being general-purpose to start hiding more important things inside it.

[tom95858]

@nichamon I think we should consider having a base_sampler_term() function that has this function in it + some other base sampler cleanup. Same thing with setup, then we can "normalize" the log naming and termination logic.

I think the base_sampler_* stuff is still a bit too far from being general-purpose to start hiding more important things inside it.

I don't think we're trying to hide anything, but if we change some common functionality that everyone shares if we want to tweak that capability I would prefer not to have to change every single sampler.

For clarity, what I'm talking about is that the plugin's term() function calls the base_sampler_term() function, but the term() function is not removed altogether.

[tom95858]

@morrone, @nichamon let's change it to ovis_log_degregister() as @morrone suggested. The only question is do we want to change it in v4.4.5? Probably yes?

[baallan]

We have quite a bit of university sampler development work off the 4.4 branch. if adding ovis_log_deregister to the 4.4 branch, ovis_log_destroy should be macro'd to create a warning to the developer to update to the new name (and substitute the new one) so that development code based on 4.4 doesn't break.

[morrone]

I'd suggest that API chances are more appropriate for OVIS-4/main than b4.4.

[nichamon]

I agreed with @morrone .