[nbs] background thread writes index records #7885
Conversation
|
#benchmark |
|
@max-hoffman DOLT
|
|
#benchmark |
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
go/store/nbs/journal_writer.go
Outdated
| @@ -94,11 +96,22 @@ func openJournalWriter(ctx context.Context, path string) (wr *journalWriter, exi | |||
| return nil, true, err | |||
| } | |||
|
|
|||
| return &journalWriter{ | |||
| sendEg, ctx := errgroup.WithContext(ctx) | |||
| recvEg, ctx := errgroup.WithContext(ctx) | |||
There was a problem hiding this comment.
This seems like a weird lifecycle change for the ctx that gets passed here. All of the sudden this context needs to be valid for the entire duration of the journalWriter, instead of just for the duration of this call...
go/store/nbs/journal_writer.go
Outdated
| wr.sendEg.Go(func() error { | ||
| select { | ||
| case wr.indexCh <- r: | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Sending these async with unbounded concurrency seems a step too far...what's the point of not enqueuing it onto the buffered channel directly?
There was a problem hiding this comment.
blocking on buffered channel is better? i wasn't really sure, the cleanup that way is definitely easier
go/store/nbs/journal_writer.go
Outdated
| err = writeIndexLookup(wr.indexWriter, l) | ||
| case lookupMeta: | ||
| l.checkSum = wr.batchCrc | ||
| wr.batchCrc = 0 |
There was a problem hiding this comment.
Can this become a local variable of this goroutine func? If not, where else is accessing it and why its current access from that place threadsafe?
go/store/nbs/journal_writer.go
Outdated
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
There seems to a problem with the error handling and communication here. Before this change, an error writing the index would cause the write call to report an error. Now, this write call and future write calls will not report an error, and we will not see an error at Close() as currently written either. Which is, maybe fine, but we will also stop reading from indexChan, and that does not seem fine, since the write path expects to be able to write to indexChan.
You've already taken the write lock on the sender side. An easy way forward is to just set indexChan to nil and report the error in a shared variable indexWriteErr and make future writes return that thing? Potentially restart the goroutine on a future write if you want to get back the old semantics?
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
Additional work is required for integration with DoltgreSQL. |
|
Additional work is required for integration with DoltgreSQL. |
|
@max-hoffman DOLT
|
|
#benchmark |
|
Additional work is required for integration with DoltgreSQL. |
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
#benchmark |
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
|
@max-hoffman DOLT
|
This uses a buffered channel to avoid blocking on indexLookup writes. The main side effect is that lookups can be out of order.