Skip to content

Conversation

prnvbn
Copy link
Contributor

@prnvbn prnvbn commented Sep 28, 2025

Summary

Adds with_on_commit to the clickhouse inserter to hook a function call after a batch is actually committed (for e.g. bumping an OTEL metric tracking number of rows written to the database)

Addresses #306

@CLAassistant
Copy link

CLAassistant commented Sep 28, 2025

CLA assistant check
All committers have signed the CLA.

@slvrtrn slvrtrn requested a review from abonander September 29, 2025 21:02
Copy link
Contributor

@abonander abonander left a comment

Choose a reason for hiding this comment

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

A couple of nits, otherwise looks fine.

@prnvbn
Copy link
Contributor Author

prnvbn commented Oct 2, 2025

@abonander addressed the comments

(P.S. I merged instead of a rebasing because I can see from the commit history that this PR will be squashed and merged)

@prnvbn prnvbn requested a review from abonander October 3, 2025 10:36
@slvrtrn slvrtrn changed the title Add callback function for the inserter feat(inserter): add callback function support Oct 3, 2025
@slvrtrn
Copy link
Contributor

slvrtrn commented Oct 3, 2025

There are some failures with rustfmt and clippy; otherwise, it looks good

@prnvbn
Copy link
Contributor Author

prnvbn commented Oct 3, 2025

There are some failures with rustfmt and clippy; otherwise, it looks good

addressed most of them. suppressed one as per - #307 (comment)

@prnvbn
Copy link
Contributor Author

prnvbn commented Oct 6, 2025

@abonander, please review when you get the chance

@prnvbn
Copy link
Contributor Author

prnvbn commented Oct 8, 2025

bump on this

Copy link
Contributor

@abonander abonander left a comment

Choose a reason for hiding this comment

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

This looks good to me, but it's an open question whether we want to block this on adding a test.

I'm personally on the fence about it since the added functionality is rather trivial, so as long as inserts continue to function correctly (which is covered already) it doesn't seem like it needs a specific test.

But I'd like @slvrtrn's opinion on it before merging.

@slvrtrn
Copy link
Contributor

slvrtrn commented Oct 17, 2025

Let's add more tests as a follow-up.

@slvrtrn slvrtrn merged commit ccabf0c into ClickHouse:main Oct 17, 2025
6 checks passed
vwency pushed a commit to vwency/clickhouse-rs that referenced this pull request Oct 19, 2025
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.

4 participants