Elasticsearch: Functionality to pass request context in performRequestAsync and retrieve it from the listener's response

Created on 21 Mar 2018  路  20Comments  路  Source: elastic/elasticsearch

Describe the feature: Functionality to pass request context in performRequestAsync and retrieve it from the listener's response

Elasticsearch version (bin/elasticsearch --version): All

JVM version (java -version): All

Description of the problem including expected versus actual behavior: Currently, if I call performRequestAsync() method then I won't be able to pass request context to it to map request and response by retrieving the request context from the response provided by the listener.

There should be support for it so that it becomes feasible to map request and response and execute appropriate logic for the request and response.

:CorFeatureJava High Level REST Client feedback_needed

Most helpful comment

Can you pull in someone from your team who can provide some better explanation rather than saying use more resources?
I think you are not getting it completely, Can you ask someone else to comment on this issue ?

@garvitlnmiit please do not dismiss peoples comments in this way and ask for others to comment. This is an open community where anyone is encouraged to comment and should be able to do so without fear of being dismissed. Not getting a response you hoped for from one developer is not a acceptable reason to dismiss their comments.

I agree with what @jimczi says that there is no reason for us to add this context object. Creating a new listener for each request adds no more overhead then creating the context object that you are asking for.

All 20 comments

Hi @garvitlnmiit ,
can you be more specific regarding your feature request. The title does not describe what you want to achieve or the functionality that you want.
From what I understand from the title you can already provide specific headers in your request, is it not enough ?

Pinging @elastic/es-core-infra

Hi @jimczi Please check now, I have described the issue.

Sorry but I don't understand why this would be useful. Can you describe a use case for this functionality ?

@jimczi

Let's say for a request I call Elasticsearch asynchronously and on receiving the response I want to call another service which takes this response and original request context. For this, I need to know the request context at the time of response.

Request context could be anything which we want to pass on further like order_id, user_id, user_id etc.

Well you can do what you want with the listener since it's just an interface. You can save the context in the listener and use it when onResponse is called ?

But for that, I will have to create a new listener object for each request, isn't it?
Also, doing so will consume more memory.

Well probably, depending on what you do with this context and the action that you takes on the listener. I think it's better to discuss this in the forum first (https://discuss.elastic.co/c/elasticsearch), we reserve github for bugs report and feature request but I think that your request can be handled with the current framework. For these reasons I hope you don't mind if I close this issue, If you open a new topic in the forum I'd be happy to help there.

@jimczi

Using your approach, a new listener object would get created for every request. Don't think this is a scalable approach?

If you know how this can be handled then let me know else re-open the issue.

Creating a new listener object for each request is not an issue, you cannot have millions of queries per second in the same client so I don't see how it could be a scaling issue. Creating a tiny object per request shouldn't affect your throughput.

@jimczi

Still not a satisfactory explanation.

You really don't think the need for having that functionality in the listener itself?

Can you pull in someone from your team who can provide some better explanation rather than saying use more resources?

You really don't think the need for having that functionality in the listener itself?

The listener is an interface that you need to implement, I really don't see why you could not implement this functionality even if you want to reuse the same listener for multiple requests. My answer is not to add more resources but that you can do whatever you want with this listener, we don't enforce anything. Adding a complex object passing to this interface in es is just out of the scope, for special use cases (that I still don't fully understand) you can add the logic on your code.

@jimczi

I think you are not getting it completely, Can you ask someone else to comment on this issue ?

Can you pull in someone from your team who can provide some better explanation rather than saying use more resources?
I think you are not getting it completely, Can you ask someone else to comment on this issue ?

@garvitlnmiit please do not dismiss peoples comments in this way and ask for others to comment. This is an open community where anyone is encouraged to comment and should be able to do so without fear of being dismissed. Not getting a response you hoped for from one developer is not a acceptable reason to dismiss their comments.

I agree with what @jimczi says that there is no reason for us to add this context object. Creating a new listener for each request adds no more overhead then creating the context object that you are asking for.

Ok. I was just looking for another opinion from your team because I could not find the explaination satisfactory.

@garvitlnmiit I do agree with @jimczi here that passing a dedicated listener should do the job just fine. I don't think there is any need to pass additional objects. Having a pattern like this is quite common and is considered best practice:

final AppId id = new AppId("some user input");
ResponseListener responseListener = new ResponseListener() {
    @Override
    public void onSuccess(Response response) {
          // do something with appID
    }

    @Override
    public void onFailure(Exception exception) {
      // do something with appID
    }
};
restClient.performRequestAsync("GET", "/", responseListener);

I hope this can clarify your concerns.

@s1monw I had already implemented this way, the discussion here is about the scalable approach.

@s1monw I had already implemented this way, the discussion here is about the scalable approach.

you mean you are concerned that this additional object per request keeps you from scaling? I wonder if this is premature optimization in your case. The JVM is very good at allocating these object fast and since they are short living they should be collected very quickly. I am not sure I follow what you mean by scalable approach.

@s1monw Yes, you got it right. But still I am not able to convince myself as the number of requests is too much in my case (~ 1million per minute)

I am pretty sure that extra object is not going to cut it. Do you have some numbers of the impact?

Was this page helpful?
0 / 5 - 0 ratings

Related issues

dadoonet picture dadoonet  路  3Comments

clintongormley picture clintongormley  路  3Comments

dawi picture dawi  路  3Comments

malpani picture malpani  路  3Comments

rpalsaxena picture rpalsaxena  路  3Comments