Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[471] - Close underlying HTTP Client on closingConnection#674

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
Varun0157 wants to merge37 commits intodatabricks:main
base:main
Choose a base branch
Loading
fromVarun0157:close-conn

Conversation

@Varun0157
Copy link

@Varun0157Varun0157 commentedAug 9, 2025
edited
Loading

(Addresses#471).

What type of PR is this?

  • Bug Fix

Description

Here, we resolve a few resource leaks with regards to TCP connections remaining open even after theConnection is closed:

  • Currently, on session closure, the thrift backend invokesself._transport.close() which simply releases a connection back to the underlying pool. It does not close the pool itself. This leads to the TCP connection not being closed properly and a danglingCLOSE_WAIT state as pointed out in the above issue.
    Thus, we rename_transport.close() to the more appropriate_transport.release_connection() and implement a new_transport.close() that actually closes the underlying pool.
    We invoke this method on SEA backend session closure as well.
  • Currently, theclose() method of theTelemetryClient does not close the unified HTTP client instantiated by it. We add code to close this as well.

How is this tested?

Related Tickets & Documents

#471

vikrantpuppalaand others added19 commitsAugust 8, 2025 12:58
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@Varun0157Varun0157 marked this pull request as draftAugust 13, 2025 07:31
@Varun0157Varun0157 changed the base branch frommain tohttp-client-refactor-2August 13, 2025 07:31
@Varun0157Varun0157 changed the base branch fromhttp-client-refactor-2 tomainAugust 13, 2025 15:43
@Varun0157Varun0157 marked this pull request as ready for reviewAugust 13, 2025 15:43
@Varun0157
Copy link
Author

@jprakash-db /@jayantsing-db /@vikrantpuppala
This PR addresses#471 and fixes a few other related issues. Can't seem to add reviewers, so pinging you here.
Thanks!

Copy link
Contributor

@vikrantpuppalavikrantpuppala 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 making these fixes! could you update from the latest main (just made significant changes to use a unified http client)

Comment on lines 317 to 318
self._close_session(session_id)
self._http_client.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is a session and http client closed together? I feel it is easier to imagine the backend module as a function module for interacting with SQL EXEC. So, with that perspective, there should be minimal state in this and the backend should receive resources by higher level classes and those higher level classes should take care of closing the resources (for example, the connection/cursor should pass a http client to this and close the http client. Same goes for session: cursor maintains the session obtained functionally through this backend and should take care of closing the session). Please let me know if this doesn't look okay.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with the philosophy overall, but I think passing the HTTP client into the backend from a higher-level object is likely infeasible right now since both backends have distinct, custom HTTP clients that expose different methods and are processed differently. Until we unify them, it makes sense to make the backend responsible for it's own HTTP Client instantiation, as a result of which it should be responsible for closing it as well.

Both HTTP Clients do have largely the same functionality, so unifying them (for the most part) could be a separate PR, at which point we can pass in the HTTP client from theSession class.
Let me know if I should do it in this PR instead.

Note that the pattern of instantiating the required HTTP client in the constructor of the concrete backend is common across Thrift and SEA.

@Varun0157
Copy link
Author

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vikrantpuppalavikrantpuppalaAwaiting requested review from vikrantpuppala

@jayantsing-dbjayantsing-dbAwaiting requested review from jayantsing-db

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Varun0157@vikrantpuppala@jayantsing-db

[8]ページ先頭

©2009-2025 Movatter.jp