On Oct 18, 2010, at 1:23 PM, Paul Davis wrote: > On Mon, Oct 18, 2010 at 12:19 PM, Adam Kocoloski <[email protected]> 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
