Skip to content

Implement release connection in batch #1062

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

Merged
merged 5 commits into from
Aug 10, 2023

Conversation

EpicStep
Copy link
Contributor

@EpicStep EpicStep commented Aug 7, 2023

With this change, you can append to the batch directly without using helper structures (slice). Earlier this led to an empty pool, although it is not necessary to keep the connection in the batcher for the append operation.

In our implementation we saved about 2GB of memory per pod.

screen

Close #1047

Copy link
Contributor

@jkaflik jkaflik 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 your contribution. I understand the need for this; however not sure if we should expose ReleaseConnection, but do it for the user instead. This might be overhead for short-live batches.

Lets address comments and think if this approach is clean.

Fewer API functions we expose, the more clarity in the usage.

@EpicStep
Copy link
Contributor Author

EpicStep commented Aug 8, 2023

I agree with you. I think we can add options in PrepareBatch (it won't break backwards compatibility).
Example: PrepareBatch(context.Background(), "INSERT INTO some_table", WithReleaseConnection())

@jkaflik
Copy link
Contributor

jkaflik commented Aug 8, 2023

@EpicStep, your proposal sounds great

@EpicStep EpicStep requested a review from jkaflik August 8, 2023 20:58
Copy link
Contributor

@jkaflik jkaflik left a comment

Choose a reason for hiding this comment

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

LGTM. Would you mind adding this feature to README?

@EpicStep
Copy link
Contributor Author

EpicStep commented Aug 9, 2023

@jkaflik Thanks. I added this feature to README.md

@EpicStep
Copy link
Contributor Author

EpicStep commented Aug 9, 2023

When will these changes be released?

@jkaflik
Copy link
Contributor

jkaflik commented Aug 10, 2023

@EpicStep I plan to release it today. In a merge train, your PR is last and has conflicts. It's easy, but it would be great if you push from your fork.

@EpicStep
Copy link
Contributor Author

@jkaflik Yes, I can merge

# Conflicts:
#	clickhouse.go
#	clickhouse_std.go
#	conn_batch.go
@EpicStep
Copy link
Contributor Author

@jkaflik done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Acquire connection in batch only when needed
2 participants