[nbs] getMany read planning and parallelize #7731
Conversation
|
#benchmark |
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
max-hoffman
changed the title
|
#benchmark |
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
…ock on journal throughout the getMany work.
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
There was a problem hiding this comment.
This looks good to me, other than the race in getMany. I don't know how to measure the impact on the lock duration here vs. what we used to have before...it's something we can always come back and fix though. I think this is good to go if we fix the race.
Simplest way to fix the race might be to move getManyCompressed impl to a helper function that has a signature something like:
func (s journalChunkSource) getManyCompressed_ext(ctx context.Context, eg *errgroup.Group, reqs []getRecord, found func(context.Context, CompressedChunk) error, stats *Stats) (bool, error) {and then to wrap it in both getMany and getManyCompressed...
| return s.getManyCompressed(ctx, eg, reqs, func(ctx context.Context, cc CompressedChunk) { | ||
| ch, err := cc.ToChunk() | ||
| if err != nil { | ||
| eg.Go(func() error { |
There was a problem hiding this comment.
Unfortunately, I don't think this is race-free. In particular, I don't think it's safe to eg.Go() on an errgroup where eg.Wait() may have already been called.
There was a problem hiding this comment.
Actually, I think I might be wrong here...in particular, errgroup is implemented in terms of a sync.WaitGroup, and I thought all positive delta wg.Add() calls were dangerous after a wg.Wait(), but that's not true...
// Note that calls with a positive delta that occur when the counter is zero
// must happen before a Wait. Calls with a negative delta, or calls with a
// positive delta that start when the counter is greater than zero, may happen
// at any time.
There was a problem hiding this comment.
And by, "I think I might be wrong here"...I'm quite sure I'm wrong and your usage is safe / to spec. Sorry for the noise.
|
We've talked about how this may negatively impact mixed workloads that will now block on reads for longer. Tradeoff being that batch reads that will complete faster, but are less interruptible because the read holds the journal lock for longer. The tradeoff for tablescans is pretty obvious, if we see issues with write throughput after adding this we add changes that make batch reads only hold the journal lock for short periods of time. |
Add read planning for journal getMany so instead of doing randIO we get seqIO. SeqIO has a large effect for disk storage systems. Also parallelize getMany calls. The biggest change here is the semantics around how reading locks the journal file. Reads hold the journal lock for longer, which could lower write throughput in some cases. If we see evidence of this, we can do more work to limit the amount of time batch reads can interruptibly holding the journal lock.