Masterslave algorithm #234
Conversation
masterslave.q
Outdated
| details:{`procname`starttimeUTC`ismaster!(.proc.procname;.masterslave.start;1b)} | ||
|
|
||
| //get details of procname provided | ||
| getdetails:{[processname] (first exec w from .servers.SERVERS where procname like processname) ".masterslave.details[]"} |
There was a problem hiding this comment.
When doing IPC can you avoid strings here, i.e. use
(.masterslave.details;)
This should be an async request, but we'll do that later.
I think this should do something on the remote side too i.e. a getdetails call from A->B causes both sides of the negotation to know about each other. So when B receives this call, it should use the details passed in from A to update it's own status
masterslave.q
Outdated
|
|
||
| //update .masterslave.statustable with other proc details and update ismaster col to determine which process is master | ||
| addmember:{[processname] | ||
| update ismaster:0b from (`.masterslave.statustable upsert .masterslave.getdetails[processname],(enlist `handle)!(enlist first exec w from .servers.SERVERS where procname like processname)) where starttimeUTC<>min starttimeUTC} |
There was a problem hiding this comment.
Can you split this out across multilines / separate statements. Kinda hard to read
masterslave.q
Outdated
|
|
||
|
|
||
| // the list of processes to connect to | ||
| .servers.CONNECTIONS:`lb |
There was a problem hiding this comment.
This should just be appended to the list of connections, and do it within an init[] function. Also just use .proc.proctype rather than `lb
masterslave.q
Outdated
|
|
||
| // custom function, this is invoked when a new outbound connection is created | ||
| // to be customised to invoke negotiation of processes | ||
| .servers.connectcustom:{.lg.o[`connect;"found new connection"]; show x} |
There was a problem hiding this comment.
Ok so need to implement this. When it receives a new connection from the discovery service or on a retry, it needs to see if there are any procs of the same proctype that it needs to communicate with to find out who is master.
This definition also needs to include any prior definitions (i.e. to not override previous definitions)
.servers.connectcustom:{[f;x] ... your code in here ...; f@x }@[value;`.servers.connectcustom;{{}}]
This should also only be done in the init function
Use this same approach for .z.pc (capture the previous definition, add your own)
masterslave.q
Outdated
| start:.z.p | ||
|
|
||
| //set details dict with own details | ||
| details:{`procname`starttimeUTC`ismaster!(.proc.procname;.masterslave.start;1b)} |
There was a problem hiding this comment.
I think probably for now we can drop the ismaster value
We can just pass the name and the start time, and then both sides of the negotiation can work out which is the master (which might be neither of them)
masterslave.q
Outdated
| .masterslave.checkifmaster[]; | ||
| }; | ||
| // create connections | ||
| .servers.startup[] |
There was a problem hiding this comment.
Having .servers.startup[] in here is fine for testing purposes, but in final version this shouldn't be in here, i.e. someone who loads this should do
.ms.init[]
and then call
.servers.startup[] later in their code
Then init function should also be within the .masterslave namespace
masterslave.q
Outdated
|
|
||
| \d .masterslave | ||
|
|
||
| //table scehma of connected lb processes with ismaster status | ||
| statustable:([handle:`int$()] procname:`symbol$();starttimeUTC:`timestamp$();ismaster:`boolean$()) | ||
| statustable:([handle:`int$()] procname:();starttimeUTC:`timestamp$();ismaster:`boolean$()); |
There was a problem hiding this comment.
why did you change procname to ()? symbol is better.
There was a problem hiding this comment.
I was getting type errors because I kept switching between symbol and string procnames.
Will change so there will only be sym procnames
masterslave.q
Outdated
|
|
||
| //is the process itself the master | ||
| ammaster:{exec ismaster from .masterslave.statustable where handle=0} | ||
| ammaster:{exec ismaster from .masterslave.statustable where handle=0}; |
There was a problem hiding this comment.
This is probably returning a list, should return an atom
masterslave.q
Outdated
| .servers.startup[]; | ||
| .masterslave.addmember each string exec procname from .servers.SERVERS where proctype like "lb", not null w | ||
| } | ||
| (.masterslave.addmember')[string exec procname from .servers.SERVERS where proctype like "lb", not null w]; |
There was a problem hiding this comment.
Don't hardcode the proctype in here. Should just be
where proctype=.proc.proctype
.i.e proctype is the same as the local process
masterslave.q
Outdated
|
|
||
| //.z.pc call | ||
| //.z.pc call | ||
| .z.pc:{.masterslave.pc[x y;y]}.z.pc; |
There was a problem hiding this comment.
need to wrap this in an error trap (in case .z.pc isn't initially defined) i.e.
.z.pc:{.masterslave.pc[x y;y]}@[value;`.z.pc;{{[x]}}]
Also as per comment above, better to have .masterslave.pc only take one param, and then .z.pc is
.z.pc:{x y; .masterslave.pc[y]}@[value;`.z.pc;{{[x]}}]
.z.pc should only be defined within init[]
masterslave.q
Outdated
| //tell other alive processes to renegotiate who the master is | ||
|
|
||
| //tell other alive processes to renegotiate who the master is | ||
| pc:{[result;W] |
There was a problem hiding this comment.
pc should probably only take one param here, W
masterslave.q
Outdated
| // custom function, this is invoked when a new outbound connection is created | ||
| // to be customised to invoke negotiation of processes | ||
| .servers.connectcustom:{.lg.o[`connect;"found new connection"]; | ||
| .masterslave.checkifmaster[]; |
There was a problem hiding this comment.
If we get a new connection in, then we only want to get the details for that single new connection. WE don't want to re-run the whole thing for all connections, which I think is what is happening here.
masterslave.q
Outdated
| //remove dropped connection from statustable | ||
| //tell other alive processes to renegotiate who the master is | ||
| pc:{[W] | ||
| .masterslave.deletedropped[W]; |
There was a problem hiding this comment.
You need a check in here to not do this if W=0
When the process starts up in the background it will do a close event on the 0 handle, which we don't want to remove from the table.
masterslave.q
Outdated
|
|
||
| // custom function, this is invoked when a new outbound connection is created | ||
| // to be customised to invoke negotiation of processes | ||
| .servers.connectcustom:{[f;x] .masterslave.checkifmaster[]; |
There was a problem hiding this comment.
I think this will work, but there is still too much work going on here (too much IPC)
Basically .servers.connectcustom will get invoked when a new connection is made, and it will be passed in (as the x argument in this case) the new connections. So, you only need this process to negotiate with those new processes, not all the old ones too. In this version, if there are 10 connections, and a new one arrives, then the way you have it every process will make 11 outbound requests, whereas really they only need to be making one
There was a problem hiding this comment.
Is f the function to be called on the new connection handlex?
And instead of running .ms.checkifmaster when a new connection is made,
I should call .ms.addmember with the new connection procname?
There was a problem hiding this comment.
Is f the function to be called on the new connection handlex?
Yes. f is the pre-existing definition of .servers.connectcustom (which may be nothing)
I should call .ms.addmember with the new connection procname
I think so
masterslave.q
Outdated
|
|
||
| //get details of procname provided | ||
| getdetails:{[processname] | ||
| (first exec w from .servers.SERVERS where procname like raze string processname,not null w) (`.masterslave.details;[]) |
There was a problem hiding this comment.
why are you doing raze string processname here?
Also why a like rather than an = if it is a symbol?
(same comment for line below)
There was a problem hiding this comment.
will change it to =, I think it was cause I was originally using strings
masterslave.q
Outdated
| //update .masterslave.statustable with other proc details and update ismaster col to determine which process is master | ||
| addmember:{[processname] | ||
| `.masterslave.statustable upsert .masterslave.getdetails[processname], | ||
| (`handle`ismaster)!(first exec w from .servers.SERVERS where procname like processname,not null w;1b) |
There was a problem hiding this comment.
Is this return value used for anything?
There was a problem hiding this comment.
as in when it returns `.masterslave.statustable?
This func is just to upsert process details dictionary into the .ms.statustable, I'm not aware of any other return value?
|
Lots of terminology to update in here. |
Master slave Algorithm
Algorithm to determine which slave process is the master process.
Each process starts up thinking it's the master and as it connects to other processes of the same type, it negotiates. Process making outbound connection initiates negotiation.
Checklist