couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Davis <paul.joseph.da...@gmail.com>
Subject Re: New Externals: Implemented with tests
Date Wed, 27 Oct 2010 03:22:43 GMT
On Mon, Oct 18, 2010 at 1:55 PM, Adam Kocoloski <kocolosk@apache.org> wrote:
> On Oct 18, 2010, at 1:23 PM, Paul Davis wrote:
>
>> On Mon, Oct 18, 2010 at 12:19 PM, Adam Kocoloski <kocolosk@apache.org> wrote:
>>> Hi Paul, nice work.  A few small points regarding couch_os_daemons:
>>>
>>> 1) couch_os_daemons:stop() will actually restart couch_os_daemons, won't it?
The child is specified as 'permanent', so even if it stops with reason normal the couch_secondary_services
supervisor will start it again, I think.
>>>
>>
>> I think technically because of how the daemons section is implemented,
>> but AFAIK, the couch_daemons would never call that. I thought I'd
>> copied that from the couch_query_servers pattern, but
>> couch_query_servers appears to have no method for actually being
>> stopped.
>>
>> I'm not entirely certain on what the best approach is. There's nothing
>> that actually tries to stop the daemon, so maybe just deleting that
>> method would be the best bet.
>
> Sounds good to me.  Either that or replace with supervisor:terminate_child(couch_secondary_services,
couch_os_daemons) if you really want a way to stop it.
>
>>
>>> 2) Why do you make the table private?  I think protected would be a better choice.
 Then handle_call(daemon_info, ...) can just return the table handle, and the client can
do whatever inspection is desired.  It would be more useful than the simple list in my opinion.
>>>
>>
>> daemon_info is purely a hack to allow the tests to make asserts on
>> internal state. In a perfect world I'd conditionally compile that only
>> when compiling test code, but we don't have any of the build machinery
>> for doing that. Using a protected table isn't out of the question, I
>> just never meant for it to be used normally outside the gen_server
>> process. Though perhaps it might be useful in the future if someone
>> wants to have an HTTP endpoint akin to _active_tasks to see what
>> daemons are alive?
>
> Yep, something like that, or else just for people who like to go poking around in the
VM while it's running :)  I think it would be good for debugging down the line.
>
>>
>>> 3) If you give couch_config:register a pointer to an exported function rather
than an anonymous function it becomes much easier to hot-upgrade couch_os_daemons w/o killing
couch_config down the line.
>>
>> You mean as simple as exporting something like "conifg_notify/2" and
>> then passing
>> "fun config_notify/2" to couch_config:register? If so that's simple
>> enough I can make the change this evening.
>
> Passing "fun couch_os_daemons:config_notify/2" is fine from the perspective of hot-loading.
 "fun config_notify/2" is just syntactic sugar for fun(A,B) -> config_notify(A,B) end,
so that's no good.
>
> http://www.erlang.org/doc/reference_manual/expressions.html#id74236
>
> Best, Adam
>
>

Just pushed out the new code.

1. couch_os_daemons:stop/0 was deleted as it didn't serve any purpose.
2. couch_os_daemons:daemon_info/0 now returns a ets table id.
2.5 to make it easier on me, I added couch_os_daemons:info/1 which
takes an Options list that only looks if the atom "table" is present
which returns the table as a list.
3. The config change notifications is now an exported function.

New code is up at [1]. I also rebased to current trunk to make sure
that's still kosher.


Chris said he's not against adding this for 1.1 on IRC. If no one else
speaks up soonish I'm gonna push this in before long. For the record,
Chris's questions and my answers were something like:

Q. Does it change current behavior?
A. No, previous external behaviour is 100% untouched

Q. Does it introduce any sort of security concerns?
A. Not by default. To take advantage of new features an admin must
make changes to the couchdb configuration to enable the new bits.

I think there was a third question, but it eludes me currently.

HTH,
Paul Davis

[1] http://github.com/davisp/couchdb/commits/new_externals/

Mime
View raw message