-
Notifications
You must be signed in to change notification settings - Fork 98
Implement monitoring for registry values #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
{-# LANGUAGE RecursiveDo #-} | ||
----------------------------------------------------------------------------- | ||
---- | | ||
---- Module : Control.Distributed.Process.Node.RegistryAgent | ||
---- Copyright : (c) Tweag I/O 2015 | ||
---- License : BSD3 (see the file LICENSE) | ||
---- | ||
---- Maintainer : Tim Watson <watson.timothy@gmail.com> | ||
---- Stability : experimental | ||
---- Portability : non-portable (requires concurrency) | ||
---- | ||
---- This module provides a registry monitoring agent, implemented as a | ||
---- /distributed-process Management Agent/. Every 'node' starts this agent on | ||
---- startup. The agent will monitor every remote process that was added to the | ||
---- local registry, so the node removes the process from the registry when it | ||
---- dies or when a network failure is detected. | ||
---- | ||
------------------------------------------------------------------------------- | ||
|
||
module Control.Distributed.Process.Node.RegistryAgent | ||
( registryMonitorAgent | ||
) where | ||
|
||
import Control.Distributed.Process.Management | ||
import Control.Distributed.Process.Internal.Types | ||
import Control.Distributed.Process.Internal.Primitives | ||
import Data.Foldable (forM_) | ||
import Data.Map (Map) | ||
import qualified Data.Map as Map | ||
|
||
registryMonitorAgentId :: MxAgentId | ||
registryMonitorAgentId = MxAgentId "service.registry.monitoring" | ||
|
||
-- | Registry monitor agent | ||
-- | ||
-- This agent listens for 'MxRegistered' and 'MxUnRegistered' events and tracks | ||
-- all remote 'ProcessId's that are stored in the registry. | ||
-- | ||
-- When a remote process is registered, the agent starts monitoring it until it | ||
-- is unregistered or the monitor notification arrives. | ||
-- | ||
-- The agent keeps the amount of labels associated with each registered remote | ||
-- process. This is necessary so the process is unmonitored only when it has | ||
-- been unregistered from all of the labels. | ||
-- | ||
registryMonitorAgent :: Process ProcessId | ||
registryMonitorAgent = do | ||
nid <- getSelfNode | ||
-- For each process the map associates the 'MonitorRef' used to monitor it and | ||
-- the amount of labels associated with it. | ||
mxAgent registryMonitorAgentId (Map.empty :: Map ProcessId (MonitorRef, Int)) | ||
[ mxSink $ \(ProcessMonitorNotification _ pid _) -> do | ||
mxUpdateLocal (Map.delete pid) | ||
mxReady | ||
, mxSink $ \ev -> do | ||
case ev of | ||
MxRegistered pid _ | ||
| processNodeId pid /= nid -> do | ||
hm <- mxGetLocal | ||
m <- liftMX $ mdo | ||
let (v,m) = Map.insertLookupWithKey (\_ (m',r) _ -> (m',r+1)) | ||
pid (mref,1) hm | ||
mref <- maybe (monitor pid) (return . fst) v | ||
return m | ||
mxSetLocal m | ||
MxUnRegistered pid _ | ||
| processNodeId pid /= nid -> do | ||
hm <- mxGetLocal | ||
forM_ (pid `Map.lookup` hm) $ \(mref, i) -> | ||
let i' = pred i | ||
in if i' == 0 | ||
then do liftMX $ unmonitorAsync mref | ||
mxSetLocal $! pid `Map.delete` hm | ||
else mxSetLocal $ Map.insert pid (mref,i') hm | ||
_ -> return () | ||
mxReady | ||
-- remove async answers from mailbox | ||
, mxSink $ \RegisterReply{} -> mxReady | ||
, mxSink $ \DidUnmonitor{} -> mxReady | ||
] |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any downside to simply having as many
MonitorRef
's as there are registered labels, rather than reference counting? I don't think the memory saving (if any) is worth it, seeing as multiple labels for the sameProcessId
is not the common case. Dropping the reference counting would make the code simpler, I reckon.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that will work, the differences are memory footprint and complexity of the
MxUnRegistered
case, that will be slightly higher (we will need traverse the structure and removeMonitorRef
).MxRegistered
will be simplified, another one will be more or less the same.So I'm open to this change.
@facundominguez, any thoughts here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced the rewriting is worth the trouble. We already have a reasonable PR. Your call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it. The following is untested but compiles:
I expect the memory usage to be pretty much the exactly the same as with reference counting: monitors are cheap, and registering a process under multiple labels is uncommon anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code above do not solve the problem that original version intended to solve, i.e. if process is registered multiple times but unregistered only once then it will be removed from map, but it shouldn't. Also I'm not terribly happy about "leaking" mrefs. I have to check code in order to understand if its safe to do. Also here we have a monitor call for each registered process (I can agree that it's a rare case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops - late at night pasted wrong code snippet. I meant to paste this one:
i.e. as the previous comment was saying, one monitor ref per label, not per process. The only difference with the previous snippet is the key of the map. Everything else is the same. BUT, there is a complication, as you point out. It's just that it's not in the
MxUnregistered
case but in theProcessMonitorNotification
case, which when written out becomes something like:I assume you meant to write
ProcessMonitorNotification
instead ofMxUnregistered
. The code is just as simple, but note the linear operation, which I also assume is what you were referring to. It's the same linear operation that's already here, over the exact same set of labels. Processes shouldn't be overusing the registry to register thousands of processes, so that's just about ok.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about
Monoid m => Map ProcesaId (m MonitorRef)
in that case complexity is inMxUnregister
because delete is linear or logarithmic there. So I think I was correct but thought about different solution in design space.About this code we need to check that reregistration
MxUnregistred
thenMxRegistered
exactly in this order, otherwiseMxRegistered
case is not correct, but I hope that ordering and existence of messages are provided.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if events get dropped then even the current code will have trouble. Events on the management bus shouldn't get dropped.
If we must assume that messages do get reordered (because the Management API documentation claims to provide no guarantees), or dropped or duplicated, then I'm not sure that using the Management API for building this functionality is a great option after all.
As @facundominguez says, not worth agonizing over the implementation of this function, since it's basically correct as-is. Though on the assumption that the Management API does provide some guarantees. @hyperthunk could you confirm what those are precisely?