incubator-couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam Kocoloski <kocol...@apache.org>
Subject Re: New Externals: Implemented with tests
Date Mon, 18 Oct 2010 17:55:50 GMT
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


Mime
View raw message