Feature #17698
closed
Parallelize writes inside keepstore when there are writes that request multiple storage classes.
Description
Customer wants to have two storage classes by default, which means always writing to two locations, so keepstore should be able to write both of them at the same time instead of one after another.
Updated by Peter Amstutz over 3 years ago
- Related to Idea #16107: Storage classes added
Updated by Peter Amstutz over 3 years ago
- Target version set to 2021-08-04 sprint
- Assigned To set to Tom Clegg
Updated by Peter Amstutz over 3 years ago
- Target version changed from 2021-08-04 sprint to 2021-08-18 sprint
Updated by Tom Clegg over 3 years ago
- Status changed from New to In Progress
17698-keepstore-concurrent-writes @ e0632f47bb83bda5badccc47cc2d8dbb70d92678 -- developer-run-tests: #2639
This adds concurrency in cases where writing to multiple classes involves writing to multiple volumes.
The new test case confirms that we don't over-replicate by starting more writes when the writes already in flight (should they turn out to be successful) will suffice, but it doesn't actually confirm that any writes are done concurrently.
Updated by Peter Amstutz over 3 years ago
- Target version changed from 2021-08-18 sprint to 2021-09-01 sprint
Updated by Lucas Di Pentima over 3 years ago
Some questions & comments:
- At file
services/keepstore/handlers.go- Line 858:
PutBlock()documentation comments seem to be a bit stale. - Line 840: I think
newPutResult()should be namednewPutProgress()and var naming consistency be maintained, as it makes the code difficult to follow for me. - Line 929: I'm curious as to why the code block doesn't use the
mntvar provided by the for-loop. Is that some special go-ism? - Line 930:
pending.Add()is called withoutmtxprotection, wouldn't that allow more than one goroutine to add the samemnt?
- Line 858:
- At file
services/keepstore/volume.go- Line 404: Is
len(vm.writables)always > 0? If not, I think we'll get a panic on read-only keepstores, correct? If yes, maybe it would be convenient to add some tests forNextWritable()
- Line 404: Is
Updated by Tom Clegg over 3 years ago
Lucas Di Pentima wrote:
- Line 858:
PutBlock()documentation comments seem to be a bit stale.
Indeed. Updated.
- Line 840: I think
newPutResult()should be namednewPutProgress()and var naming consistency be maintained, as it makes the code difficult to follow for me.
Fixed, thanks.
- Line 929: I'm curious as to why the code block doesn't use the
mntvar provided by the for-loop. Is that some special go-ism?
That ensures each goroutine gets its own mnt var indicating the mnt it's supposed to work on -- otherwise they would all share the for loop's var, changing underneath them.
- Line 930:
pending.Add()is called withoutmtxprotection, wouldn't that allow more than one goroutine to add the samemnt?
mtx is locked there -- and for that whole loop, except while calling cond.Wait(). Added a comment to this effect.
- At file
services/keepstore/volume.go
- Line 404: Is
len(vm.writables)always > 0? If not, I think we'll get a panic on read-only keepstores, correct? If yes, maybe it would be convenient to add some tests forNextWritable()
Turns out handlePUT() doesn't even call PutBlock() if there are no writable volumes, but I added a test and fixed NextWritable anyway.
17698-keepstore-concurrent-writes @ 69cc7c89aa51e7aa7265215bfc910eaa457986d8 -- developer-run-tests: #2646
Updated by Lucas Di Pentima over 3 years ago
Thanks for the clarifications, this LGTM, thanks!
Updated by Tom Clegg over 3 years ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|a098a3a28cbf70895668cb052e9676a29ef6f764.