On Wed, 02 Oct 2013 15:20:33 +0200
Daniel-Constantin Mierla <miconda(a)gmail.com> wrote:
On 10/2/13 8:25 AM, Timo Teras wrote:
On Mon, 30 Sep 2013 14:46:48 +0200
"Olle E. Johansson" <oej(a)edvina.net> wrote:
30 sep 2013 kl. 14:04 skrev Daniel-Constantin
Mierla
<miconda(a)gmail.com>om>:
Hello,
On 9/30/13 12:08 PM, Timo Teras wrote:
> On Mon, 30 Sep 2013 11:48:10 +0200
> Daniel-Constantin Mierla <miconda(a)gmail.com> wrote:
>
>> a short reminder that we are one week before feature freeze for
>> next major release.
> I would like to incorporate the module from:
>
https://github.com/rdboisvert/mohqueue
>
> Could you review it?
>
> I am willing to be maintainer for it if needed.
interesting feature - no need for special review because it is a
stand alone module. You are already registered developer, so you
can review yourself a bit if you didn't do (use) it already - the
review of new modules from new developers is to check quickly if
they use internal memory managers, DB API and/or spot if something
else from existing code can be reused.
Agree, this is a really cool feature. I can
think of a ton of RPC
commands as well as stats/counters I would like to see. We're
looking forward to this!
The author asked me to do the initial merge. He might be
willing to
take over the maintainer ship in few weeks, and we can come back
to this discussion after a bit.
I created 'tteras/mohqueue' branch and pushed the module there
as-is - with LICENSE removed as being redundant now that it is part
of the main tree.
If possible please take a look at it and post any comments to fix
before merge to master, which I plan to do tomorrow.
Robert, I also saw that you had created new branch 'noRTPstop' with
three additional commits - please let me know if those are to be
included too. Let me know also if there's any additional changes
you'd like to have in.
The code indentation style is hard to follow for me,
because the
blocks are not easy visible (for what I used to, probably). I would
prefer a style using tabs, with blocks indenting the inner acctions
(e.g., like in the core and most of the modules, that should be
pretty much Allman or K&R). Anyhow, it is not a must as long as
developer of that code keeps maintaining the code and doesn't need
help from others.
I had the same comment. I'll just run it through indent.
From a quick look at main file in the module:
- fixup_str() can be replaced with fixup_spve_spve(...) - it is a
function that allows static and dynamic string parameters
- fixup_count() is then fixup_spve_null(...)
Ok, good points.
The sql tables definition have to be made in xml and
added to
lib/srdb1/schema/, then the sql script will be generated for all db
connectors via:
make dbschema
So modules/mohqueue/mohqueue.sql should not be part of the module --
sql scripts will be part of utils/kamctl/[mysql|postgres|...]/.
Oh right, forgot this.
I also noted that the locking could be simplified, and use rwlock
primites instead of rewriting it as spinlocks.
-Timo