Arvados

7121-fix-deadlock at 94e1489

Whoops, missed committing a new file. Now at d2db86b

• log “collision” or “corrupt” errors so that we can have a log processor later on that looks for those and cleans up. (Can be in volume_unix.go)

• volume_unix.go: This comment “Compare returns nil if Get(loc) would return the same content as …” : Do you mean “expect” instead of “cmp” at the beginning of second line of this comment? Even though you use “cmp” in the method implementation, I think we need to use the given input parameter name in the comment.

• handlers.go: CompareAndTouch method — If any other errors other than collisionOrCorrupt, we are continuing and trying other volumes. Should we log and continue in the collisionOrCorrupt cases also?

• There are still a few “serialize” references, especially in keepstore help message. Since you renamed “serialize” to “locker”, can you please update these remaining references as well?

• Since the file mutex_test.go does not actually have any “tests” itself, it is a bit confusing. Can it be named “mutex_test_helper.go” instead? Or, better yet, add a test or two in it to make it complete.

• volume.go : the comment for Get: Is the caller urged to put the returned slice into buffer pool to ensure cache is refreshed? if so, can you add that at the end of that comment.

• volume.go : the comment for Compare is a bit confusing around “Confirm Get() would return buf. If so, return nil”. Can we say something like “Confirm Get(loc) would return a buf with same content as data …” ?

• In collision_test.go, can you please add one more test with buf starting with anything other than "foo" resulting in DiskHashError?

Thanks.

Radhika Chippada wrote:

• log “collision” or “corrupt” errors so that we can have a log processor later on that looks for those and cleans up. (Can be in volume_unix.go)

Added logs in CompareAndTouch() -- this way Volume implementors can just return errors, and one set of "what's worth logging, and how" logic can live in the handlers.

• volume_unix.go: This comment “Compare returns nil if Get(loc) would return the same content as …” : Do you mean “expect” instead of “cmp” at the beginning of second line of this comment? Even though you use “cmp” in the method implementation, I think we need to use the given input parameter name in the comment.

Yes, I forgot to update the comment when updating the signature. Fixed.

• handlers.go: CompareAndTouch method — If any other errors other than collisionOrCorrupt, we are continuing and trying other volumes. Should we log and continue in the collisionOrCorrupt cases also?

In the "corrupt" case we log and continue; in the "collision" case we have to stop, to avoid getting into a state where two volumes have files with the same hash but different data. In such cases it's better to tell the current client "can't store that" (and continue giving the earlier clients the same data they wrote) than to tell the current client "OK" and start giving both old & new clients sometimes-old and sometimes-new data when they ask for it.

• There are still a few “serialize” references, especially in keepstore help message. Since you renamed “serialize” to “locker”, can you please update these remaining references as well?

I think "-serialize" is still the correct term for the command line flag (and argument to TempUnixVolume). If there were other kinds of lockers to specify, we could ask the caller to say things like "-locker=mutex" on the command line, but I think -serialize is still suitable...

• Since the file mutex_test.go does not actually have any “tests” itself, it is a bit confusing. Can it be named “mutex_test_helper.go” instead? Or, better yet, add a test or two in it to make it complete.

Fair enough. Changed to mock_mutex_for_test.go. (I want to make sure it's not accidentally exported, and *_test.go seems to be the only recognized pattern indicating to the Go tools "only used when running tests for this package".)

• volume.go : the comment for Get: Is the caller urged to put the returned slice into buffer pool to ensure cache is refreshed? if so, can you add that at the end of that comment.

Added rationale: "(Otherwise, the buffer pool will be depleted and eventually -- when all available buffers are used and not returned -- operations will reach deadlock.)"

• volume.go : the comment for Compare is a bit confusing around “Confirm Get() would return buf. If so, return nil”. Can we say something like “Confirm Get(loc) would return a buf with same content as data …” ?

Yes, makes sense. Updated.

• In collision_test.go, can you please add one more test with buf starting with anything other than "foo" resulting in DiskHashError?

Ah, good call. Added three new tests with equal length but mismatched data in buf1, buf2, and rdr respectively.

Now at 23cc118

Thanks for clarifying and updating. LGTM.