Arvados

11644-mounts-api @ 42f5f3a29761ac9a943530157da376c798a4ac6d

Lucas Di Pentima wrote:

• File services/keepstore/volume.go
  • Lines 329-333: Can you please explain to me what’s happening there? (just curious, I suppose it's doing some kind of introspection, right? If so, what happens with mnt.DeviceID if DeviceID() isn't there?)

It's a type check on an unnamed interface, equivalent to

type DeviceIDer interface {
        DeviceID() string
}

if v, ok := v.(DeviceIDer); ok {
        mnt.DeviceID = v.DeviceID
}

If the volume doesn't support that interface (i.e., doesn't have a DeviceID() func) then mnt.DeviceID will still be "", which (presumably) keep-balance will have to interpret as "no device id available".

...but this actually only made sense while some volume types didn't support DeviceID(). It would be simpler to just make DeviceID() part of the Volume interface. So I've done that.

• Line 322: Why declare a method with an “anonymous receiver”, isn’t the same a declaring it as a normal function?

It's just a way of keeping it in the VolumeMount namespace. The calling syntax is a bit weird but it's more or less equivalent to a class or static method.

• File services/keepstore/volume_unix.go
  • The utility command /sbin/blkid could be used to get the mounted device UUID (it works running it as a normal user, as tested on Debian Jessie)

On my debian 8 box, that only works after filling the cache by running blkid as root. Before that, it prints empty results.

The blkid man page says to use lsblk instead, which always prints empty UUIDs unless running as root.

(Aside: I was tempted to parse /proc/self/mountinfo instead of invoking findmnt -- that way we wouldn't have to fork any processes at all -- but that seems like premature optimization at this point.)

• File services/keepstore/handlers.go
  • Lines 65, 66: Those routes could be grouped with the one at Line 55? Should we maintain a consistent URL naming scheme and use /mounts/{uuid}/index/{prefix:…} instead?

I went with "blocks" because "blocks/" is a web convention for getting an index of blocks, while "index" doesn't say what it's getting an index of.

But on the subject of web conventions... /mounts/{uuid}/blocks/?prefix={prefix} would probably be even better. That way /mounts/{uuid}/blocks/{locator} can be reserved for a more obvious purpose, i.e., fetch {locator}'s data from the specified mount.

• Couldn’t IndexHandler and MountBlocks be unified? If not, shouldn’t be better to name MountBlocks as MountBlocksHandler to keep naming consistency? (the same with rtr.Mount)

Yes to both. Done.

I was thinking that it would be better to move in the opposite direction and call the handlers Index and Mounts and Get, but now that you mention it, it does seem better to stay consistent until we change them all at once.

11644-mounts-api @ 370036888e75b509ebf84ba8337273f7b9146f37

Applied in changeset arvados|commit:d121e087ad1b4e91f869dbd57534c6d6ce51d19d.