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

enhance: add client interceptor support #900

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

Conversation

madogar
Copy link

@madogar madogar commented May 13, 2024

No description provided.

@sre-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: madogar
To complete the pull request process, please assign yhmo after the PR has been reviewed.
You can assign the PR to them by writing /assign @yhmo in a comment when ready.

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

@sre-ci-robot
Copy link

Welcome @madogar! It looks like this is your first PR to milvus-io/milvus-sdk-java 🎉

Signed-off-by: Shreesha Srinath Madogaran <smadogaran@salesforce.com>
@yhmo
Copy link
Contributor

yhmo commented May 15, 2024

@madogar
We don't intend to expose direct interceptor input to users.

Java SDK follows the python sdk logic. In python sdk, the interceptor is used to pass db_name and authorization.
https://github.com/milvus-io/pymilvus/blob/3198bd2614446cd3b4266751394d6d16dcb6a8ef/pymilvus/client/grpc_handler.py#L236
https://github.com/milvus-io/pymilvus/blob/3198bd2614446cd3b4266751394d6d16dcb6a8ef/pymilvus/client/grpc_handler.py#L239

The python sdk doesn't provide direct interceptor for users, either. We only expose parameters that we can support.

@madogar
Copy link
Author

madogar commented May 15, 2024

@yhmo
My bad, forgot to set the context. Here is the server side change to set client_request_id in logs:
milvus-io/milvus#32264

As of today there is no way to set the param from java SDK, hence this change where would implement an interceptor like below and set it at client level:
`public class TraceIdClientInterceptor implements ClientInterceptor {
@OverRide
public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(MethodDescriptor<ReqT, RespT> method, CallOptions callOptions, Channel next) {

    return new ForwardingClientCall
        .SimpleForwardingClientCall<ReqT, RespT>(next.newCall(method, callOptions)) {
        @Override
        public void start(ClientCall.Listener<RespT> responseListener, Metadata headers) {
            headers.put(Metadata.Key.of("client_request_id", Metadata.ASCII_STRING_MARSHALLER), "999-" + UUID.randomUUID().toString());
            super.start(responseListener, headers);
        }
    };
}

}`

The biggest challenge is setting this (unique value)param for every gRPC call.

@yhmo
Copy link
Contributor

yhmo commented May 20, 2024

@madogar
This pr is to set a client ID for the connection, and pass the client ID by ClientInterceptor.
In my opinion, I mean no need to expose ClientInterceptor to users.
Just like the database name:

if (StringUtils.isNotEmpty(connectParam.getDatabaseName())) {
            metadata.put(Metadata.Key.of("dbname", Metadata.ASCII_STRING_MARSHALLER), connectParam.getDatabaseName());
}

List<ClientInterceptor> clientInterceptors = new ArrayList<>();
clientInterceptors.add(MetadataUtils.newAttachHeadersInterceptor(metadata));

We can add a method to the ConnectParam like this:

if (StringUtils.isNotEmpty(connectParam.getClientID())) {
            metadata.put(Metadata.Key.of("client_request_id", Metadata.ASCII_STRING_MARSHALLER), connectParam.getClientID());
}

List<ClientInterceptor> clientInterceptors = new ArrayList<>();
clientInterceptors.add(MetadataUtils.newAttachHeadersInterceptor(metadata));

The ConnectParam.withClientInterceptors() directly exposes the grpc class ClientInterceptor to users, which is not a good design. It might be out of control if users pass ClientInterceptor freely to the server.

Just let the user input an ID by ConnectParam.withClientID(String cliecntID), and internally encapsulate the ID with a ClientInterceptor.

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

Successfully merging this pull request may close these issues.

None yet

3 participants