-
Notifications
You must be signed in to change notification settings - Fork 594
Add gotData and endOfProcess handlers #1564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -32,6 +32,8 @@ type onProcess struct { | |||
progress func(*Progress) | |||
profileInfo func(*ProfileInfo) | |||
profileEvents func([]ProfileEvent) | |||
gotData func() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference of gotData
vs data
? Other than the function signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data
is "internal" handler set by library itself during query start:
Lines 67 to 84 in 46d2298
var ( | |
errors = make(chan error, 1) | |
stream = make(chan *proto.Block, bufferSize) | |
) | |
go func() { | |
onProcess.data = func(b *proto.Block) { | |
stream <- b | |
} | |
err := c.process(ctx, onProcess) | |
if err != nil { | |
c.debugf("[query] process error: %v", err) | |
errors <- err | |
} | |
close(stream) | |
close(errors) | |
release(c, err) | |
}() |
@@ -178,6 +183,9 @@ func (c *connect) handle(ctx context.Context, packet byte, on *onProcess) error | |||
} | |||
if block.Rows() != 0 && on.data != nil { | |||
on.data(block) | |||
if on.gotData != nil { | |||
on.gotData() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on.gotData() | |
on.gotData(block) |
why doesn't gotData take block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like to, I can add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I sent a response in Slack but I'll copy here for the record)
I looked at the code here, and I'm hesitant to make these changes since they feel very application-specific (to our internal app). I want to learn more about what the goal of these hooks are.
Specifically these things stand out:
- Having hooks for
data
andgotData
is confusing, the names need to be clearer. We could renamedata
toblock
andgotData
tofirstBlock
(assuming the goal is to track the first block, or count blocks). Maybe we could renamedata
to some internal function like_data
since it's the only one that is always overridden by the driver. - I saw the code suggestion to add
*Block
togotData
. I think this would cause some data races if the user had access to these outside of the driver, and so we shouldn't expose them. endOfProcess
can be renamedendOfStream
since the server packet is calledEndOfStream
. This depends on whether your goal is to track the end of processing, or the actualEndOfStream
packet.- Do we need
endOfProcess
to know if a query ended? This should already be detectable through the respondingdriver.Rows
return value.
Overall I want to make sure we aren't adding a solution to something really specific for one application. If we add it we need to test/maintain it, and I am suspicious about these lower level hooks. I feel like there's another way to expose these events, perhaps in the driver.Rows
interface.
Summary
Add two new handlers to track the processing:
WithGotData
allows to pass a function will be triggered on every processing data block.WithEndOfProcess
allows to pass a function will be triggered when the whole processing going to be finished.Checklist
Delete items not relevant to your PR: