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.

Sign up for GitHub

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

Jump to bottom

reuse grpc channel and clean connections #1446

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

reuse grpc channel and clean connections #1446

wants to merge 1 commit into from

Conversation

filip-halt
Copy link
Contributor

@filip-halt filip-halt commented May 18, 2023 •
edited
Loading

This PR:

  • adds in grpc reuse
  • corrects user and db_name alias checks
  • cleans up and reduces helper functions
  • centralizes connection args logic
  • enforces correct URI, does not attempt to attach default host and port to miss structed uri

@mergify mergify bot added the dco-passed label May 18, 2023
@filip-halt filip-halt force-pushed the grpc_channel_reuse branch 4 times, most recently from 9ac4dee to 17b21e3 Compare May 22, 2023 23:23
@mergify mergify bot added the ci-passed label May 22, 2023
@filip-halt filip-halt force-pushed the grpc_channel_reuse branch from e8d87c4 to 7691ad3 Compare May 22, 2023 23:33
@filip-halt filip-halt force-pushed the grpc_channel_reuse branch from d87c5d1 to 1a215d0 Compare May 23, 2023 00:52
@filip-halt filip-halt requested a review from longjiquan May 23, 2023 00:52
@filip-halt filip-halt force-pushed the grpc_channel_reuse branch 2 times, most recently from 542e54b to 7844400 Compare May 24, 2023 05:20
@filip-halt filip-halt changed the title [WIP] reuse grpc channel [WIP] reuse grpc channel and clean connections May 24, 2023
@filip-halt filip-halt force-pushed the grpc_channel_reuse branch from 7844400 to 0018fef Compare May 24, 2023 05:24
@filip-halt filip-halt changed the title [WIP] reuse grpc channel and clean connections reuse grpc channel and clean connections May 24, 2023
@filip-halt filip-halt force-pushed the grpc_channel_reuse branch from 0018fef to 18fd399 Compare May 24, 2023 05:28
@filip-halt
Copy link
Contributor Author

/assign @czs007

@XuanYang-cn
Copy link
Contributor

/cc @czs007

@sre-ci-robot sre-ci-robot requested a review from czs007 May 24, 2023 05:40
longjiquan
longjiquan requested changes May 29, 2023
View reviewed changes
pymilvus/orm/connections.py Outdated
timeout = t if isinstance(t, (int, float)) else Config.MILVUS_CONN_TIMEOUT
if gh is None:
gh = GrpcHandler(**kwargs)
t = kwargs.get("timeout")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe kwargs.get("timeout", None) is better to avoid crash.

Copy link
Contributor

Choose a reason for hiding this comment

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

kwargs.get("timeout") and kwargs.get("timeout", None) are identical

pymilvus/orm/connections.py
if (
key in self._connected_alias
and connection_details["address"] == kwargs["address"]
and connection_details["user"] == kwargs["user"]
Copy link
Contributor

Choose a reason for hiding this comment

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

why we don't check the password?

Copy link
Contributor

Choose a reason for hiding this comment

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

password won't be cached, so unable to check

pymilvus/orm/connections.py Outdated
return
# Set secure=True if username and password are provided
if len(user) > 0 and len(password) > 0:
kwargs["secure"] = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we relate the tls (secure) with the user and password? In fact, authentication with user/password can be done without tls, see also https://wiki.lfaidata.foundation/display/MIL/MEP+27+--+Support+Basic+Authentication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this. We don't need to set secure=True for user and password authentication. secure=True is for tls secure connections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I believe before we were using this as a method for checking. At the moment if host/api is supplied there isnt a way to check if https correct?

@sre-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: filip-halt
To complete the pull request process, please ask for approval from czs007 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

XuanYang-cn
XuanYang-cn requested changes May 29, 2023
View reviewed changes
pymilvus/orm/connections.py Outdated
return
# Set secure=True if username and password are provided
if len(user) > 0 and len(password) > 0:
kwargs["secure"] = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this. We don't need to set secure=True for user and password authentication. secure=True is for tls secure connections.

@filip-halt filip-halt force-pushed the grpc_channel_reuse branch 3 times, most recently from a43d4f0 to e1fb9c9 Compare May 31, 2023 00:40
@filip-halt filip-halt requested a review from czs007 May 31, 2023 19:14
@filip-halt filip-halt force-pushed the grpc_channel_reuse branch 2 times, most recently from 3206ddb to 6b17472 Compare May 31, 2023 19:20
@filip-halt filip-halt force-pushed the grpc_channel_reuse branch from 6b17472 to 733d1da Compare May 31, 2023 19:32
@mergify mergify bot added needs-dco and removed dco-passed labels Mar 5, 2024
@tvvignesh tvvignesh mentioned this pull request Aug 10, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@longjiquan longjiquan Awaiting requested review from longjiquan

@XuanYang-cn XuanYang-cn Awaiting requested review from XuanYang-cn

@czs007 czs007 Awaiting requested review from czs007

Requested changes must be addressed to merge this pull request.

Assignees

@czs007 czs007

Labels
ci-passed needs-dco size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@filip-halt @XuanYang-cn @sre-ci-robot @longjiquan @czs007