Extract record batch structure #314
Conversation
There was a problem hiding this comment.
Thanks for taking the time to do this refactor!
One ask I'll have is we're about to merge #306, which is going to affect the compression part of this work. Would you be able to adapt the code to account for these changes?
|
Sure. I'll redo it after the merge of #306 |
|
@VictorDenisov we've made a couple more optimizations that we'll have to integrate here as well: |
VictorDenisov
force-pushed
the
transactions/extract_record_batch
branch
from
bdeef33 to
02f9541
Compare
VictorDenisov
force-pushed
the
transactions/extract_record_batch
branch
from
02f9541 to
eaba0a1
Compare
|
@achille-roussel I updated the PR for the latest code. |
recordbatch.go
Outdated
| size int32 | ||
| } | ||
|
|
||
| func (r *recordBatch) init() (err error) { |
There was a problem hiding this comment.
It looks like it's never valid to use a recordBatch without calling init, how about making it a constructor instead?
func newRecordBatch(codec CompressionCodec, msgs []Message) *recordBatch {
...
}One challenge is managing the error returned by compressRecordBatch, maybe the recordBatch type can hold the error value like we do for writeBuffer?
There was a problem hiding this comment.
Yeah, I was choosing between this option and the option that I implemented. My motivation was to not duplicate the fields in the record batch and in the declaration of the method. I don't have any strong attachment to either solution. Would you prefer your variant?
There was a problem hiding this comment.
Since a recordBatch value is never fully initialized without calling init I think it would make the code harder to use wrong to have the construction contained in a single function 👍
|
Thanks for your contribution! |
@achille-roussel @stevevls Would you be willing to merge this refactoring?
As I was working on adding transaction support to batches I realized that constructing batches deep in the invocation stack is very tedious as all their parameters should be passed to V3 and V7 functions.
I extracted record batch structure and now it's constructed one level above the writeProduceRequest functions. This way if we need to add arguments to recordBatches we can do it without touching signatures of writeProduceRequest functions.