-
Notifications
You must be signed in to change notification settings - Fork 55
Stream inspector for destinations #718
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
Conversation
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 tried it out and it worked, both through a websocket and gRPC, nice work! I think the PR needs some polish but we're definitely on the right track 🙌
Some comments that don't belong anywhere else:
- Stopping Conduit with an inspector is not possible, Conduit blocks until the client closes the connection.
- It's possible to start an inspector on a stopped pipeline, we should probably check the status and return an error if that's the case.
- Similarly it's possible to stop a pipeline and keep an inspector running, I'd also expect the inspector to stop in that case.
- While testing I was able to process up to 120 records per second (I used https://websocketking.com/), that feels a bit slow given that it's running on the same machine, am I expecting too much?
- I don't think we mentioned metrics in the design, but I think it would be nice to at least see the number of open inspector sessions on a connector / processor. WDYT? No need to do it in this PR.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
d2c99b2 to
1a94458
Compare
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.
Mostly just nitpicks, I think this is already looking very nice!
Co-authored-by: Lovro Mažgon <[email protected]>
Co-authored-by: Lovro Mažgon <[email protected]>
Co-authored-by: Lovro Mažgon <[email protected]>
Co-authored-by: Lovro Mažgon <[email protected]>
…itIO/conduit into haris/stream-inspector-destination
This reverts commit 8f40c22.
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.
LGTM 👏
Description
Depends on #732
Fixes #700
Quick checks: