Arvados

Reviewing 4ab25f3

1. In keep PutBlock(), shouldn't it attempt to do some sort of load balancing across disks, instead of writing to the first available volume? E.g. preferentially writing to whichever volume is the least full (by percentage)
2. volume_unix Read(), what happens if f.Read(buf) returns a short read? It should loop until EOF.
3. volume_unix Read(), it returns the entire 'buf' on error. It should probably return nil in the 'buf' position.
4. Go doesn't have better path manipulation functions than Sprintf? (e.g. something like os.path.join() in Python)
5. Should index requests be I/O queued as well? If there are a lot of files scanning the entire directory tree could interfere with other I/O
6. SSDs don't have seek time overhead, does it make sense to allow parallel read/write on SSD?
7. What is the reasoning for having IsFull() record the "full" state with a bogus symlink? Is statfs() an expensive call? And why a symlink and not a regular file?
8. Empty "return" statements are weird

Peter Amstutz wrote:

Reviewing 4ab25f3

In keep PutBlock(), shouldn't it attempt to do some sort of load balancing across disks, instead of writing to the first available volume? E.g. preferentially writing to whichever volume is the least full (by percentage)

Yes, good point. We threw around some ideas for what kind of strategies we could use here but didn't settle on any, so I didn't get around to changing it.

I think I like having it round-robin through the volumes -- that should be simpler to implement, it's more likely to reduce disk contention, and it should keep them pretty well balanced for space. Objections?

volume_unix Read(), what happens if f.Read(buf) returns a short read? It should loop until EOF.

Good point. Done.

volume_unix Read(), it returns the entire 'buf' on error. It should probably return nil in the 'buf' position.

I'd like to let the caller decide what (if anything) to do with the buffer in the event of an error. The convention of returning a value and also an error means that it's pretty unambiguous when there is and isn't an error, and there's a very strong expectation that if an error is returned, the caller will check it and do something with it. It seems useful to be able to report an error but also return whatever data we were able to read.

Go doesn't have better path manipulation functions than Sprintf? (e.g. something like os.path.join() in Python)

There is, actually: filepath.Join. Fixed.

Should index requests be I/O queued as well? If there are a lot of files scanning the entire directory tree could interfere with other I/O

No, the story description for #2620 specifically excludes /index from operations that need to honor an I/O lock. (If we find in practice that /index seems to invoke disk contention then of course we can revisit that.)

SSDs don't have seek time overhead, does it make sense to allow parallel read/write on SSD?

Sure, the admin can just run keep with -serialize=false in that case. I think that at this stage, this feels like a policy decision that is better left to admin control.

What is the reasoning for having IsFull() record the "full" state with a bogus symlink? Is statfs() an expensive call? And why a symlink and not a regular file?

This is for parity with the current Warehouse implementation, which uses a symlink this way to mark a full Keep volume (presumably a symlink rather than a file so as not to risk writing to a full disk). I am not actually sure this degree of feature parity is necessary, but IIRC Ward wants Keep to support this feature specifically to make sure that new Keep can work as a drop-in replacement for Warehouse. I'll be happy to drop it if/when we agree that it's unnecessary.

Empty "return" statements are weird

Agreed!

Just pushed a change (57b46f9) to replace the KeepVolumes global array with a VolumeManager interface.

See volume.go for a description of the interface and the RRVolumeManager implementation. Then the changes to keep.go and keep_test.go should make sense. IMHO this is a lot cleaner now, and is hopefully easier to read and understand too.