[syslog-ng] [RFC]: LogThreadedDestDriver - making threaded destinations better

[Balazs Scheidler]

On Wed, 2012-04-25 at 12:42 +0200, Gergely Nagy wrote:
> Hi!
> 
> There have been a number of fixes going into afsql or afmongodb (or
> both) recently, fixing bugs that affected both, for the simple reason
> that when I made afmongodb, I copied parts of the architecture from
> afsql. Now there is the SMTP destination aswell, suffering from the same
> issues, and a Redis destination is in the works aswell.
> 
> Therefore, it is about time to rethink how these drivers that all look
> very similar under the hood could be rebased onto a common foundation,
> something that abstracts away all the common stuff, such as handling the
> worker thread, and the queue method - so all the affected drivers need
> to do, is implement their own tasks, and skip all the copy-paste-ish
> boilerplate they have now.
> 
> To this end, we had a short brainstorming with Bazsi, and I'll try to
> summarize what I remember (with a few later ideas added).
> 
> The goal is to create a new class, based on LogDestDriver, for all those
> destination drivers that use a worker thread to do their blocking
> operations. This new class, which for the time being shall be named
> LogThreadedDestDriver shall implement the following things:
> 
> * It must handle the setup, starting and stopping of the worker thread,
>   with a module-supplied worker function, and that function shall be
>   assisted by class functions to do the dirty work.
> * The queue handling must be safe and free of races (which pretty much
>   means we have to find a way to wrap log_queue_pop_head into a
>   synchronous call)
> * The class must be prepared to work not only with single messages, but
>   in bulk aswell (think SQL's transactions, or the upcoming MongoDB bulk
>   insert)
> * The class must implement its own queue method, downstream modules
>   should not need to care about that.
> * We must be able to wake up the worker thread, even if there is nothing
>   to handle: this is needed for a clean shutdown.
> 
> And that's pretty much it.
> 
> The class would have callbacks for the following things:
> 
> * A pair of callbacks to call during the driver's _init() and _deinit()
>   functions. These can either be implemented as callbacks, and the
>   driver will then use it's own init/deinit and set it in its parent,
>   and call the module-set callbacks from those.
> 
>   Or, the class can provide _init/_deinit methods, and leave it up to
>   the module to implement its own methods, and call the class methods
>   itself.
> 
>   The former makes drivers that do not require extra init/deinit
>   simpler, the latter has less callbacks and looks neater. When
>   implementing, I'll probably go with the latter.

I would prefer OOP-style virtual methods over an additional layer of
callbacks. As long as the init function can simply be invoked by the
subclass.

> 
> * An 'do-the-work' callback, which is called whenever work needs to be
>   performed, and an error callback to fire whenever the do-the-work
>   failed.
> 
>   I'm not entirely sure how best to handle the transaction case here,
>   but that's something that needs to be done around here.
> 
>   One possibility is to allow two kinds of do-the-work callbacks: one
>   for the simple case where no transactions are done (afsmtp and current
>   afmongodb) and one that supports transactions/bulk inserts (afsql and
>   future afmongodb and redis).

Probably separate methods for start_txn and commit_txn and a txn_size or
something. start/commit would do nothing by default. txn_size would
probably be the same as flush_lines.

Also, we should get rid of flush_timeout() and close transactions
whenever the input queue empties, just like Patrick suggested in an
earlier thread. Also the BalaBit syslog-ng team has decided the same for
their 4.2 version. Their code is available on git.balabit.hu, so we can
borrow their implementation.

> 
> Off the top of my head, this should be enough to move the boring parts
> of the current threaded destination drivers into a common class. Once
> the transaction case is figured out, all of our drivers can be
> reimplemented on top of this class, reducing copied and slightly altered
> code by a HUGE margin, and finally making it so that we have a single
> place to fix possible bugs in, instead of 3 slightly different copies of
> the same thing.
> 
> As a side effect, time_reopen and stored/dropped counter handling would
> get moved from individual drivers into this base class, along with
> flush_timeout and flush_lines (if those still exist in 3.4) and a couple
> of other things.
> 
> I plan to have a Proof of Concept patch ready by friday night, we'll see
> how it goes!
> 
> (And shortly after, a redis destination will follow)
> 

This sounds like a plan :)

Thanks for working on this.

-- 
Bazsi