Use a separate mailbox for 'db_updated' messages #40

Merged
merged 3 commits into from Jun 14, 2012

Projects

None yet

3 participants

@bdionne
Contributor
bdionne commented Apr 19, 2012

A continuous changes feed will decay with an active
database as the controller mailbox is shared with the
db updater notifier. This patch spawns a separate
process to receive the 'db_updated' messages.

BugzID:13421

@rnewson rnewson commented on the diff Apr 19, 2012
src/fabric_db_update_listener.erl
@@ -0,0 +1,106 @@
+% Copyright 2010 Cloudant
@rnewson
rnewson Apr 19, 2012 Member

2012, shurely?

@bdionne
bdionne Apr 19, 2012 Contributor

good eye, actually the entire component has this same header

@kocolosk kocolosk commented on an outdated diff May 7, 2012
src/fabric_view_changes.erl
{Timeout, _} = couch_changes:get_changes_timeout(Args, Callback),
+ CallerRef = make_ref(),
+ CallerPid = spawn_link(fun() ->
+ fabric_db_update_listener:go(CallerRef, self(), DbName, Timeout)
+ end),
@kocolosk
kocolosk May 7, 2012 Member

We should use the spawn_link(M, F, A) variant here to better deal with hot code upgrades.

@kocolosk
kocolosk May 7, 2012 Member

Also, I don't think you mean to call self() inside the spawned process, do you?

@kocolosk
Member
kocolosk commented May 7, 2012

I find the identification of fabric_view_changes as a rexi endpoint confusing. It seems to me that fabric_db_update_listener lives only to serve fabric_view_changes. I think we should have a blocking API in fabric_db_update_listener that fabric_view_changes can use to find the current state. I don't really see the need to abuse rexi:sync_reply/1 for this purpose -- the spawned process is local, so the interactions between them needn't use rexi at all.

@bdionne
Contributor
bdionne commented May 8, 2012

I think I see your criticism, but don't quite grok the simpler solution yet. I agree the use of rexi:sync_reply is awkward but I thought this was the point of creating a separate controller and using the rexi machinery. It needs to receive both the db_update messages as well as messages from the fabric_view_changes controller interrogating the state.

I'll think on this some more while I'm out running

@bdionne
Contributor
bdionne commented May 9, 2012

Adam,

This last commit implements a version that removes the rexi:sync_reply. Works the same but it still needs more testing

Bob

Bob Dionne and others added some commits Apr 2, 2012
Bob Dionne Use a separate mailbox for 'db_updated' messages
A continuous changes feed will decay with an active
database as the controller mailbox is shared with the
db updater notifier. This patch spawns a separate
process to receive the 'db_updated' messages.

BugzID:13421
309ef1c
@kocolosk kocolosk Remove an import 0d27d23
@kocolosk kocolosk Tag messages, only send timeout if asked
This changes the message protocol between the db_update_listener client
and coordinator so that replies are tagged with the 'state' atom and
Pid of the coordinator process.  Hopefully it avoids unexpected
messages being treated as the State.  It also avoids sending a timeout
message unless a 'get_state' request has arrived.  Previously we'd send
extra 'timeout' messages and end up somewhat out-of-sync.

Using a gen_fsm for the coordinator would seem to be the way to go here.

BugzID: 13421
f598244
@kocolosk
Member

@bdionne see what you think about f598244. I think tagging the messages is important. The change to only send a timeout message on request seems like the right thing to do, too.

@bdionne
Contributor
bdionne commented Jun 14, 2012

@kocolosk completely agree, tagging is safer and yes this should be an fsm

@kocolosk kocolosk merged commit ab8d4f1 into master Jun 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment