Skip to content
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

Add QuestDB output component #115

Merged
merged 9 commits into from
Sep 18, 2024
Merged

Conversation

sklarsa
Copy link
Contributor

@sklarsa sklarsa commented Sep 13, 2024

Adds an output component for writing to QuestDB tables using ILP (Influx Line Protocol) as implemented by the official QuestDB Go Client.

@gregfurman gregfurman self-requested a review September 13, 2024 15:40
Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this! All is looking good just some minor clarifications and suggestions.

Also, have you used QuestDB against our existing SQL components? Would be curious if it works out-the-box it and would be happy to extend them if need to do accomodate QuestDB!

Otherwise, your tests are passing on my local but I haven't (yet) tested this against a running QuestDB instance. Will do so soon 🥇

internal/impl/questdb/output.go Outdated Show resolved Hide resolved
internal/impl/questdb/output.go Show resolved Hide resolved
internal/impl/questdb/output.go Outdated Show resolved Hide resolved
internal/impl/questdb/output.go Outdated Show resolved Hide resolved
internal/impl/questdb/output.go Show resolved Hide resolved
Comment on lines +215 to +216
// todo: is this the correct interpretation of max-in-flight?
qdb.WithMaxSenders(mif)(w.pool)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The max_in_flight field is specifically for internal Bento messages. This WithMaxSenders looks like it's referring to the number of QuestDB Senders which I believe are different (from QuestDB dotnet docs):

Sender is single-threaded, and uses a single connection to the database.
If you want to send in parallel, you can use multiple senders and standard async tasking.

cc @jem-davies Thoughts on this?

Reminds me that we could probably add a StreamTestOptMaxInFlight test to the integration tests. Will comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick update here: not sure about the above. Have asked Jem to take a look.

What other value would make sense for the sender count?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pool defaults to a maximum of 64 senders. Ideally, we would use a sender per bento thread/goroutine, but it's not clear to me how many concurrent executions of WriteBatch can happen at a single time. I'm guessing max_in_flight is a maximum message count then?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can take a look tomorrow - just busy at the moment

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's a maximum message count. Happy with approving and seeing how performance looks, and iterating on this at another point

internal/impl/questdb/output.go Show resolved Hide resolved
internal/impl/questdb/output.go Outdated Show resolved Hide resolved
sklarsa and others added 4 commits September 16, 2024 13:41
Co-authored-by: Greg Furman <31275503+gregfurman@users.noreply.github.com>
Co-authored-by: Greg Furman <31275503+gregfurman@users.noreply.github.com>
@sklarsa
Copy link
Contributor Author

sklarsa commented Sep 16, 2024

Also, have you used QuestDB against our existing SQL components? Would be curious if it works out-the-box it and would be happy to extend them if need to do accomodate QuestDB!

I have not. We implement pgwire and should be compatible with standard postgres clients. I'll give that a shot this week, although we really only recommend using SQL for data extraction and querying, not ingestion.

@gregfurman
Copy link
Collaborator

Lastly, can you add the docs for the component? Running make docs should suffice 🙂

Once that's added I'll approve and we can ship this bad-boy

@sklarsa
Copy link
Contributor Author

sklarsa commented Sep 17, 2024

Lastly, can you add the docs for the component? Running make docs should suffice 🙂

Done! Thank you :-)

Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just fixed the formatting of that description string.

@gregfurman gregfurman merged commit 431e8b5 into warpstreamlabs:main Sep 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants