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

close httpClient when closing simpleIngestManager #931

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sfc-gh-xhuang
Copy link
Contributor

Merging pr from @nirutgupta

#930

The CloseableHttpResponse is closed with try catch (ref), but the client itself is not getting closed, in case if exception is thrown here. This is causing connection leaks, timeout errors and decrease in throughput.

Added the fix to close the httpClient in SimpleIngestManager close() method.

Will add ingestManager.close() here post this change.

@sfc-gh-japatel
Copy link
Collaborator

sfc-gh-japatel commented Jan 24, 2025

Are we okay doing this way (cloning the PR and opening again), should we need CLA signed from author? @sfc-gh-xhuang

@sfc-gh-xhuang
Copy link
Contributor Author

@sfc-gh-japatel Confluent has a CLA already

@sfc-gh-tzhang
Copy link
Contributor

This is Snowpipe, we will need Snowpipe folks to review the change

@sfc-gh-xhuang
Copy link
Contributor Author

@sfc-gh-tzhang I've asked Snowpipe team to take a look for approval but it seems the failing test is a streaming test?

@sfc-gh-tzhang
Copy link
Contributor

@sfc-gh-tzhang I've asked Snowpipe team to take a look for approval but it seems the failing test is a streaming test?

Yes, the failure can be ignored, I have asked Alec to take a look since his PR has the same failure, does look like a real issue

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