481266 - Provide a way to perform actions on remote clients

Status: RESOLVED FIXED

(Cloud Services :: General, defect, P1)

Description

[Ed Lee :Mardak]

This is needed for bug 480057 to make all remote clients like the local client.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Here's an initial stab where commands are stored in a hash where the key is a command and the value is an array of command arguments.

{ command: [[arg], [arg]], command2: [[]] }

However, now that I implemented it this way, it won't be possible to order the commands. But it does allow ignoring repeated commands of the same args.

Originally in bug 480057, we proposed something similar:

[ { action: command, args: [[arg], [arg]] } ]

But that also falls into the problem of not being able to easily adjust the command order (array of args).

I'll probably rewrite this to be more like..

[ { command: [arg] }, { command2: [] } ]

Where new commands are pushed on the end.

Do we still want to filter out duplicate commands though? Only if they would be adjacent?

Comment 2

[Dan Mills [:thunder]]

It's hard to say definitively when it's so open-ended.

I think you should have the format use an array, but code that reads it in can decide to ignore certain commands.  It's more than uniquifying--for example, if you had a command to wipe a collection, then a command to wipe the whole server, you can just ignore the former.

Comment 3

[Ed Lee :Mardak]

Attachment: v2

Switched to an array of hashes that have a command and an args.

We'll have the thing processing the commands filter out more complex "action dupes". Might get tricky down the line if we have [reset, doSomething, resetAll].. Are we allowed to coalesce the resets. I guess it depends on |doSomething| ;)

Comment 4

[Dan Mills [:thunder]]

Comment on attachment 365310 [details] [diff] [review]
v2

>+    let packCommand = function(args, desc) ({
>+      args: args,
>+      desc: desc,
>+    });
>+
>+    return {
>+      resetAll: packCommand(0, "Clear temporary local data for all engines"),
>+      resetEngine: packCommand(1, "Clear temporary local data for engine"),
>+      wipeAll: packCommand(0, "Delete all client data for all engines"),
>+      wipeEngine: packCommand(1, "Delete all client data for engine"),
>+    };

nit: isn't this:

>+    return {
>+      resetAll: {args: 0, desc: "Clear temporary local data for all engines"}
...

more or less just as brief, and easier to read?

Otherwise looks good.  Just needs the part where it applies them.

Comment 5

[Ed Lee :Mardak]

(In reply to comment #4)
> more or less just as brief, and easier to read?
I suppose the inline object is more compact.. but the function makes sure the "command object" is consistent. Also, if we add in more properties, we'll need to copy/paste the name to each line in addition to adding the new value.

Comment 6

[Dan Mills [:thunder]]

Okay, fair enough.

Comment 7

[Ed Lee :Mardak]

I just switched it over to be..

get _commands() ({
  resetAll: { args: 0, "Clear ..." },
}),

But I just though of a different way! >:D

get _commands() ([
  ["resetAll", 0, "Clear temporary local data for all engines"],
  ["resetEngine", 1, "Clear temporary local data for engine"]
].reduce(function(c, e) (c[e[0]] = { args: e[1], desc: e[2] }) && c, {})),

:D !!

Comment 8

[Dan Mills [:thunder]]

> get _commands() ([
>   ["resetAll", 0, "Clear temporary local data for all engines"],
>   ["resetEngine", 1, "Clear temporary local data for engine"]
> ].reduce(function(c, e) (c[e[0]] = { args: e[1], desc: e[2] }) && c, {})),
> 
> :D !!

Sir,

You almost make JS look like Perl ;-)

Comment 9

[Dan Mills [:thunder]]

It's fine, btw :) go ahead and change it.

Comment 10

[Ed Lee :Mardak]

Attachment: v2.1

r? for an extra change to ClientStore.prototype.

I noticed ClientStore_createRecord when investigating bug 482513 -- records not getting to the server.

I'm not running into a problem yet, but it seems like there would be a problem if we created a new record for the local client just based on the name and type.

Comment 11

[Dan Mills [:thunder]]

Comment on attachment 366753 [details] [diff] [review]
v2.1


>   createRecord: function ClientStore_createRecord(id) {
>     let record = new ClientRecord();
>     record.id = id;
>-    if (id == Clients.clientID)
>-      record.payload = {name: Clients.clientName, type: Clients.clientType};
>-    else
>-      record.payload = this._clients[id];
>+    record.payload = this._clients[id];
>+
>+    // For the local client, update the name and type with the current value
>+    if (id == Clients.clientID) {
>+      record.payload.name = Clients.clientName;
>+      record.payload.type = Clients.clientType;
>+    }
>+

This doesn't seem right.  We don't save the local client's record to the metadata file (which this._clients represents), so if id == Clients.clientID, then this._clients[id] == undefined.

Even if there were some bug and this._clients[Clients.clientID] existed, we wouldn't want to use that data, createRecord should always fully generate a fresh payload when id == Clients.clientID.

I think I see the problem you're running into, though.  You're generating a new client record for a remote client with extra stuff (commands), then on the receiving end you are looking to save that?

I don't think you need to save the commands to disk.  Extract the command info and process it right away / save it for later, then mark Clients.clientID for upload, so you overwrite the server record with a fresh one (without any commands in it).

Comment 12

[Ed Lee :Mardak]

(In reply to comment #11)
> This doesn't seem right.  We don't save the local client's record to the
> metadata file (which this._clients represents), so if id == Clients.clientID,
> then this._clients[id] == undefined.
When getting this.clients, it creates an entry for the local client:

get clients() {
  this._clients[Clients.clientID] = this.createRecord(Clients.clientID).payload;

So it might be null, but then we could just || {} inside createRecord

> createRecord should always fully generate a fresh payload
I don't see how the patch wouldn't do it any differently from now. Right now we check if it's the local client and then add "name" and "type" to the payload. The patch takes the existing payload and updates the "name" and "type" as before, but now it'll carry over any commands as well.

> I don't think you need to save the commands to disk.
I suppose that's true. Commands should disappear after reaching its target. But if we ever need to persist state in the client record, we would need to fix this.

Comment 13

[Dan Mills [:thunder]]

(In reply to comment #12)

> > I don't think you need to save the commands to disk.
> I suppose that's true. Commands should disappear after reaching its target. But
> if we ever need to persist state in the client record, we would need to fix
> this.

I think this is the real issue: currently clientData.js doesn't allow you to store arbitrary data in there, you have to modify ClientStore for that.

I don't see a particular problem with that right now, and it's outside the scope of this bug imo (since you shouldn't be storing the commands locally anyway).

Comment 14

[Ed Lee :Mardak]

I need to make that change in clientData because otherwise when I try getting the local client record, it just creates a record with only a name and type.

Comment 15

[Ed Lee :Mardak]

Attachment: v2.2

With better handling of non-existant _clients[id].

Note that _clients[id] will be created when applyIncoming calls update/create.

Comment 16

[Dan Mills [:thunder]]

(In reply to comment #14)
> I need to make that change in clientData because otherwise when I try getting
> the local client record, it just creates a record with only a name and type.

I don't understand, what else do you need there?

Comment 17

[Dan Mills [:thunder]]

(In reply to comment #15)
> Created an attachment (id=366889) [details]
> v2.2
> 
> With better handling of non-existant _clients[id].
> 
> Note that _clients[id] will be created when applyIncoming calls update/create.

Yeah, I think that's a bug, we should be checking if id == Clients.clientID and if so extracting info and setting it in the canonical location (e.g., prefs for name/type).

Comment 18

[Dan Mills [:thunder]]

Comment on attachment 366889 [details] [diff] [review]
v2.2

As I explained on irc, the clients hunk is suboptimal, but it's ok for M5.  I'll file a followup bug for fixing that.

Comment 19

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/0de1b766f8f9