Hi Gena,

The intent of the shutdown hook functionality is not to be a general application shutdown hook, but to provide reasonably ways of having logback wait for a some condition before shutting down (not just calling LoggerContext.stop immediately). Logback can provide one or more general mechanisms for performing this action, however if the user's needs differ they could implement their own.

The benefit of utilizing a logback based shutdown hook for cleaning up logback resources is that logback can ensure that shutdown hook is properly removed when the context is reset (which reduces the complexity of using the Runtime hook methods directly). Additionally, the user can utilize existing, tested implementations if one of the Logback provided shutdown hook mechanisms is appropriate for the user's application or scenario.

Just because a shutdown hook implementation is simple (such as a one-liner) does not make the logback managed mechanism invalid or irrelevant. The user still has the choice to implement a hook however they choose - via a predefined, managed hook or a completely custom hook.

I'm not sure how this design is premature optimization. It is a very simple design for flexibility - there is no impact to speed, debuggability, or maintenance. Though some may qualify it as "you aren't going to need it", YAGNI more accurately refers to features which significantly take away from the development of core functionality, testing, documentation, etc or add significant complexity, limit future flexibility, etc. The ShutdownHookAction and associated classes are compact, well-defined, do not detract from the maintainability of logback as a whole, do not add significant complexity, nor limit future flexibility.

To be completely honest, Ceki first suggested using a Joran action about a week ago. Since then I have only spent about 4 hours researching, developing, and testing the ShutdownActionHook solution - which is now 100% complete from a functionality standpoint and 95% complete overall (just pending documentation and some unit testing). The other changes to AsyncAppender had been built into a proof of concept and re-implemented with suggestions from the pull request over a week ago. I have probably spent more time debating the merits of your proposal vs mine than actually researching, developing, testing, and documenting!   :) 

Not that I regret the debate by any means - it has been a healthy design discussion and has helped to clarify other use-cases and mechanisms that I may not have thought of otherwise. That being said, with buy-in from maintainers of Logback on this design (at least to this point), I don't see much point in debating further. It still seems to me that the best solution is the blending of our two proposals - using my structure to provide flexibility and your solution to transition the logback mode. Since my solution is nearly complete and yours may be a ways off, this seems like a natural fit both technically and logistically. That being said, if anyone from Logback team wants to weigh in on how they feel would be best to proceed in light of the proposals and discussion, I am open to proceeding with the result of the group decision.

At the very least, regardless of what proposal or blending of proposals is approved, I believe that the changes already made to the AsyncAppender in the open pull request are still valid since that is what ensures that LoggerContext.stop does not return until AsyncAppender has flushed its queue or reached its configured timeout. From my perspective, it is just the discussion around the shutdown hook implementation components that is in contention, agreed?

Thanks!

Regards,

Mike Reinhold




On Thu, Mar 20, 2014 at 3:03 PM, Gena Makhomed <gmm@csdoc.com> wrote:
On 20.03.2014 17:17, Michael Reinhold wrote:

All that being said, I'm not quite sure why you are resistant to a
framework for implementing shutdown hooks. Is there a particular reason
you object to the user being able to select which shutdown mechanism
should be employed by Logback? I understand that you feel your solution
covers most or all cases, but is it not possible that the end user of
Logback may disagree and prefer to apply their own shutdown procedure or
policy?

Right now what user can do inside logback shutdown hook - is just one action: calling LoggerContext.stop after stopping other app subsystems.

And, if user do graceful shutdown of other subsystems in shutdown hook
called "logback shutdown hook" - in fact they write own java code for
application shutdown hook, but falsely name it "logback shutdown hook"

This mismatch between name of logback shutdown hook and its role
of application shutdown hook is the main reason why I am resistant
to a creating yet one more framework for implementing shutdown hooks.

Second reason: framework for implementing shutdown hooks already exists.
it is Runtime.addShutdownHook() and Runtime.removeShutdownHook() methods

Third reason: if implement my proposed solution in logback,
the code of logback shutdown hook will look like this:

public void run() { loggerContext.switchToFailSafeMode(); }

If user want to call LoggerContext.stop in own application shutdown hook
it still can do it in own APPLICATION shutdown hook without any problems

Fourth reason:

I suppose additional attribute <configuration shutdownHook="false">
(with default value "true") can be added to logback, if some user need
completely turn off build-in logback shutdown hook and want to write
own application shutdown hook, using already existing framework for
implementing shutdown hooks from JVM. But right now this is looks
like premature optimization:

========================================================================

http://c2.com/cgi/wiki?PrematureOptimization

"Programmers waste enormous amounts of time thinking about, or worrying about, the speed of noncritical parts of their programs, and these attempts at efficiency actually have a strong negative impact when debugging and maintenance are considered. We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%."

========================================================================

Fifth reason:

see arguments from page
https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

"Always implement things when you actually need them, never when you just foresee that you need them".

"Until the feature is actually needed, it is difficult to fully define what it should do and to test it. If the new feature is not properly defined and tested, it may not work correctly, even if it eventually is needed".

"Adding the new feature may suggest other new features. If these new features are implemented as well, this may result in a snowball effect towards feature creep".


As a side question, how long would you estimate it would take to modify
logback-core (and/or the other logback components) to implement your
proposed solution?

I am not learn all details of code, but suppose that this is
not very difficult task. May be few weeks, if work in spare time
(need to read all sources of logback and understand internal logic).


Are you currently working on implementing
this switchToFailSafe functionality

Right now - not, because I don't know if this code will be accepted
or rejected, my first try was to ask about such idea - if idea accepted,
as the next step I can try to write patch with this idea implementation.


or are you proposing that someone else from the logback community develop it?

I just proposing idea how to make logback graceful shutdown solution.
If this idea will be accepted and approved, then next step can be done.

If someone else from the logback community want
to code this idea - I have nothing against this.


--
Best regards,
 Gena
_______________________________________________
Logback-user mailing list
Logback-user@qos.ch
http://mailman.qos.ch/mailman/listinfo/logback-user