[RFC]: LogThreadedDestDriver - making threaded destinations better
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. * 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). 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) -- |8]
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
For the record, my current work on the LogThreadedDestDriver is available from the feature/3.4/threaded-destdriver branch of my repo: https://github.com/algernon/syslog-ng/commits/feature/3.4/threaded-destdrive... Balazs Scheidler <bazsi@balabit.hu> writes:
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.
I went with a mix, because that allowed me to have the bulk of the code in the class easily, and didn't require the converted drivers to go to great lengths if they wanted to override a particular function.
* 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.
As a first step, I'd refactor the code still keeping flush_lines and the slightly broken queue_pop_head(), convert whatever can be to LogThreadedDestDriver, and go from there. Fixing the queue_pop_head() situation and flush_lines would be an entirely separate project for me, primarly so I don't loose track O:)
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.
Thanks, I'll take a look!
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 :)
If you have a few spare moments, and could have a look at my feature/3.4/threaded-destdriver branch, I'd appreciate any feedback. (You might wish to revert the top patch, "WIP: $something", because that does not compile. I put it there so I can work it over the long weekend, but I was busy with other things and didn't get around to it.) -- |8]
participants (2)
-
Balazs Scheidler
-
Gergely Nagy