Solr
  1. Solr
  2. SOLR-6625

Add interceptor API for outgoing calls through HttpSolrClient

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.3, 6.0
    • Component/s: SolrJ
    • Labels:
      None

      Description

      Some of our setups use Solr in a SPNego/kerberos setup (we've done this by adding our own filters to the web.xml). We have an issue in that SPNego requires a negotiation step, but some HttpSolrServer requests are not repeatable, notably the PUT/POST requests. So, what happens is, HttpSolrServer sends the requests, the server responds with a negotiation request, and the request fails because the request is not repeatable. We've modified our code to send a repeatable request beforehand in these cases.

      It would be nicer if HttpSolrServer provided a pre/post callback when it was making an httpclient request. This would allow administrators to make changes to the request for authentication purposes, and would allow users to make per-request changes to the httpclient calls (i.e. modify httpclient requestconfig to modify the timeout on a per-request basis).

      1. SOLR-6625_interceptor.patch
        34 kB
        Ishan Chattopadhyaya
      2. SOLR-6625_interceptor.patch
        41 kB
        Ishan Chattopadhyaya
      3. SOLR-6625_interceptor.patch
        62 kB
        Ishan Chattopadhyaya
      4. SOLR-6625_interceptor.patch
        61 kB
        Ishan Chattopadhyaya
      5. SOLR-6625_interceptor.patch
        39 kB
        Ishan Chattopadhyaya
      6. SOLR-6625_interceptor.patch
        39 kB
        Ishan Chattopadhyaya
      7. SOLR-6625_interceptor.patch
        40 kB
        Ishan Chattopadhyaya
      8. SOLR-6625_interceptor.patch
        48 kB
        Ishan Chattopadhyaya
      9. SOLR-6625_interceptor.patch
        49 kB
        Ishan Chattopadhyaya
      10. SOLR-6625_interceptor.patch
        8 kB
        Ishan Chattopadhyaya
      11. SOLR-6625_r1654079.patch
        54 kB
        Per Steffensen
      12. SOLR-6625_r1654079.patch
        49 kB
        Per Steffensen
      13. SOLR-6625_SolrReqPropogate.patch
        71 kB
        Noble Paul
      14. SOLR-6625.patch
        37 kB
        Noble Paul
      15. SOLR-6625.patch
        37 kB
        Noble Paul
      16. SOLR-6625.patch
        53 kB
        Gregory Chanan
      17. SOLR-6625.patch
        53 kB
        Gregory Chanan
      18. SOLR-6625.patch
        52 kB
        Gregory Chanan
      19. SOLR-6625.patch
        51 kB
        Gregory Chanan
      20. SOLR-6625.patch
        20 kB
        Gregory Chanan
      21. SOLR-6625.patch
        15 kB
        Gregory Chanan
      22. SOLR-6625-revert.patch
        14 kB
        Ishan Chattopadhyaya
      23. SOLR-6625-testfailure.log
        49 kB
        Anshum Gupta
      24. SOLR-6625-testfix.patch
        2 kB
        Ishan Chattopadhyaya
      25. SOLR-6625-testfix.patch
        1 kB
        Ishan Chattopadhyaya
      26. SOLR-6625-testfix.patch
        0.6 kB
        Ishan Chattopadhyaya
      27. SOLR-6625-testfix.patch
        2 kB
        Ishan Chattopadhyaya
      28. SOLR-6625-testfix.patch
        1 kB
        Ishan Chattopadhyaya

        Issue Links

          Activity

          Hide
          Gregory Chanan added a comment -

          Here's a patch with a couple of tests:

          • Verifies that the http methods are as expected in BasicHttpSolrServer
          • Adds a test to BasicHttpSolrServer that tests modifying the URI of the request before it is sent out

          NOTE: this patch assumes SOLR-6723 is applied, because it modifies a test case from there.

          Show
          Gregory Chanan added a comment - Here's a patch with a couple of tests: Verifies that the http methods are as expected in BasicHttpSolrServer Adds a test to BasicHttpSolrServer that tests modifying the URI of the request before it is sent out NOTE: this patch assumes SOLR-6723 is applied, because it modifies a test case from there.
          Hide
          Mark Miller added a comment -

          +1, looks good.

          Might want to consider making that interface an abstract class in case we want to add postRequest with back compat or something, but nothing I'm stuck on.

          Show
          Mark Miller added a comment - +1, looks good. Might want to consider making that interface an abstract class in case we want to add postRequest with back compat or something, but nothing I'm stuck on.
          Hide
          Gregory Chanan added a comment -

          Might want to consider making that interface an abstract class in case we want to add postRequest with back compat or something, but nothing I'm stuck on.

          Good suggestion, I'll do that on commit.

          Show
          Gregory Chanan added a comment - Might want to consider making that interface an abstract class in case we want to add postRequest with back compat or something, but nothing I'm stuck on. Good suggestion, I'll do that on commit.
          Hide
          Per Steffensen added a comment -

          SOLR-4470 supports repeatable POST request by using BufferedHttpEntity. Look at the changes in HttpSolrServer - search for credentialsButNonPreemptive in latest patch https://issues.apache.org/jira/secure/attachment/12634740/SOLR-4470.patch

          Show
          Per Steffensen added a comment - SOLR-4470 supports repeatable POST request by using BufferedHttpEntity. Look at the changes in HttpSolrServer - search for credentialsButNonPreemptive in latest patch https://issues.apache.org/jira/secure/attachment/12634740/SOLR-4470.patch
          Hide
          Gregory Chanan added a comment -

          Thanks for pointing that out Per Steffensen!

          One thing I wanted to avoid with this patch is putting authentication-type specific details in HttpSolrServer. SOLR-4470 has a little logic there that is basic-auth specific; if we support a range of authentication types this could get complex. Perhaps the best thing is to combine approaches here?

          i.e. it seems from my reading of SOLR-4470 that if you were to implement the basic-auth specific code via a callback you'd need:
          1) to be passed the SolrRequest (so you could check getPreemptiveAuthentication)
          2) the ability to return an HttpContext for the HttpSolrServer to use

          1) certainly doesn't seem like a problem, and 2) we could do via the return value of preRequest or via another method in the callback, e.g applyContextForRequestPre.

          As for the suggestion of using a BufferedHttpEntity rather than the OPTIONS approach I describe above, that certainly may be an improvement. I'd have to look more into it, but in either case, it seems it can be implemented via the callback.

          Thoughts?

          Show
          Gregory Chanan added a comment - Thanks for pointing that out Per Steffensen ! One thing I wanted to avoid with this patch is putting authentication-type specific details in HttpSolrServer. SOLR-4470 has a little logic there that is basic-auth specific; if we support a range of authentication types this could get complex. Perhaps the best thing is to combine approaches here? i.e. it seems from my reading of SOLR-4470 that if you were to implement the basic-auth specific code via a callback you'd need: 1) to be passed the SolrRequest (so you could check getPreemptiveAuthentication) 2) the ability to return an HttpContext for the HttpSolrServer to use 1) certainly doesn't seem like a problem, and 2) we could do via the return value of preRequest or via another method in the callback, e.g applyContextForRequestPre. As for the suggestion of using a BufferedHttpEntity rather than the OPTIONS approach I describe above, that certainly may be an improvement. I'd have to look more into it, but in either case, it seems it can be implemented via the callback. Thoughts?
          Hide
          Gregory Chanan added a comment -

          Here's another version of the patch around what I described from the previous post.

          Changes:

          Show
          Gregory Chanan added a comment - Here's another version of the patch around what I described from the previous post. Changes: Is more explicit about the when the callback fires, it's called preExecuteRequest now to indicate it's called before HttpClient.execute Passes the solrRequest via the callback now, so an implementation can call getPreemptiveAuthentication or similar as implemented in SOLR-4470 Callback can return an HttpContext to use. HttpContext implementation in HttpSolrServer is borrowed from SOLR-4470 . HttpContext seems sufficient as a return value; I was originally thinking an extensible class. But the only execute params that take an HttpUriRequest (from http://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/client/HttpClient.html#execute%28org.apache.http.client.methods.HttpUriRequest,%20org.apache.http.client.ResponseHandler,%20org.apache.http.protocol.HttpContext%29 ) are the request, the response handler, and the context. We can already modify the request via the callback, and solr has its own response handler functionality, so I'm not sure of a case where we'd need to modify HttpClient's responseHandler. Adds a test to BasicHttpSolrServerTests that exercises providing a context by adding some cookies.
          Hide
          Per Steffensen added a comment -

          One thing I wanted to avoid with this patch is putting authentication-type specific details in HttpSolrServer. SOLR-4470 has a little logic there that is basic-auth specific

          Actually SOLR-4470 aims at introducing a framework for any authentication-type, and then (for now) implement basic-auth using this framework. It is prepared for adding new authentication types. See AuthCredentials carrying any kind of AbstractAuthMethod - currently only AbstractAuthMethod-implementation is BasicHttpAuth. Adding a new authentication type should basically be about adding a new AbstractAuthMethod-implementation. But sorry, I do not remember to many details. But what I do know, is that we have been using SOLR-4470 solution now in production for a long time, without any problems at all.

          As for the suggestion of using a BufferedHttpEntity rather than the OPTIONS approach I describe above, that certainly may be an improvement.

          I do not know if it is an improvement compared to your approach. I just implemented in a way that worked. Supporting non-preemptive authenticating POST-requests was not the main focus of SOLR-4470, so I just quickly did it in the way that I found it could be done - without considering performance or anything

          Show
          Per Steffensen added a comment - One thing I wanted to avoid with this patch is putting authentication-type specific details in HttpSolrServer. SOLR-4470 has a little logic there that is basic-auth specific Actually SOLR-4470 aims at introducing a framework for any authentication-type, and then (for now) implement basic-auth using this framework. It is prepared for adding new authentication types. See AuthCredentials carrying any kind of AbstractAuthMethod - currently only AbstractAuthMethod -implementation is BasicHttpAuth . Adding a new authentication type should basically be about adding a new AbstractAuthMethod -implementation. But sorry, I do not remember to many details. But what I do know, is that we have been using SOLR-4470 solution now in production for a long time, without any problems at all. As for the suggestion of using a BufferedHttpEntity rather than the OPTIONS approach I describe above, that certainly may be an improvement. I do not know if it is an improvement compared to your approach. I just implemented in a way that worked. Supporting non-preemptive authenticating POST-requests was not the main focus of SOLR-4470 , so I just quickly did it in the way that I found it could be done - without considering performance or anything
          Hide
          Gregory Chanan added a comment -

          Actually SOLR-4470 aims at introducing a framework for any authentication-type, and then (for now) implement basic-auth using this framework

          Ah, I see, I misinterpreted the SOLR-4470 code in HttpSolrServer – it uses BasicAuthCache and BasicScheme which I thought were in reference to basic auth, but they are really just default implementations.

          What I'm really arguing – and it's my fault I didn't make it clear with example code – is that the authentication type may affect how you want the http requests to look, beyond just the credentials. For example, I'm using an authentication filter based off of Hadoop's AuthenticationFilter (https://github.com/apache/hadoop/blob/7250b0bf914a55d0fa4802834de7f1909f1b0d6b/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/server/AuthenticationFilter.java). That filter does SPNego negotiation on the first request, but sets a cookie you can use to avoid the negotiation on subsequent requests. So, I wouldn't want the the SOLR-4470 implementation where I buffer up every request; I only want to do that on the first request to the server on the connection.

          From seeing the SOLR-4470 code, though, it looks like I was thinking about this incorrectly. Instead of the HttpClientCallback being a function of the HttpSolrServer, it's really a function of the AuthCredentials implementation. So, the default implementation would just be the credentialsButNonPreemptive/getHttpContextForRequest code you have in HttpSolrServer in SOLR-4470, but other AuthCredentials implementations could override. Does that sound right to you, Per Steffensen?

          I do not know if it is an improvement compared to your approach. I just implemented in a way that worked. Supporting non-preemptive authenticating POST-requests was not the main focus of SOLR-4470, so I just quickly did it in the way that I found it could be done - without considering performance or anything

          Cool, I'll investigate in another jira.

          Show
          Gregory Chanan added a comment - Actually SOLR-4470 aims at introducing a framework for any authentication-type, and then (for now) implement basic-auth using this framework Ah, I see, I misinterpreted the SOLR-4470 code in HttpSolrServer – it uses BasicAuthCache and BasicScheme which I thought were in reference to basic auth, but they are really just default implementations. What I'm really arguing – and it's my fault I didn't make it clear with example code – is that the authentication type may affect how you want the http requests to look, beyond just the credentials. For example, I'm using an authentication filter based off of Hadoop's AuthenticationFilter ( https://github.com/apache/hadoop/blob/7250b0bf914a55d0fa4802834de7f1909f1b0d6b/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/server/AuthenticationFilter.java ). That filter does SPNego negotiation on the first request, but sets a cookie you can use to avoid the negotiation on subsequent requests. So, I wouldn't want the the SOLR-4470 implementation where I buffer up every request; I only want to do that on the first request to the server on the connection. From seeing the SOLR-4470 code, though, it looks like I was thinking about this incorrectly. Instead of the HttpClientCallback being a function of the HttpSolrServer, it's really a function of the AuthCredentials implementation. So, the default implementation would just be the credentialsButNonPreemptive/getHttpContextForRequest code you have in HttpSolrServer in SOLR-4470 , but other AuthCredentials implementations could override. Does that sound right to you, Per Steffensen ? I do not know if it is an improvement compared to your approach. I just implemented in a way that worked. Supporting non-preemptive authenticating POST-requests was not the main focus of SOLR-4470 , so I just quickly did it in the way that I found it could be done - without considering performance or anything Cool, I'll investigate in another jira.
          Hide
          Gregory Chanan added a comment -

          One other note: if we buy my argument above, that each authentication implementation may want to implement an HttpClientCallback in order to perform some authentication-specific work, we need it in every place where uses HttpClient. From scanning the source code, it looks like we'd need to add something to ConcurrentUpdateSolrServer and SolrDispatchFilter and possibly SolrCLI.

          Instead of the HttpClientCallback being a function of the HttpSolrServer, it's really a function of the AuthCredentials implementation

          I should mention, perhaps AuthCredentials is not a good name here if it's credentials + callback. Maybe AuthenticationMethod or AuthenticationType or something.

          Show
          Gregory Chanan added a comment - One other note: if we buy my argument above, that each authentication implementation may want to implement an HttpClientCallback in order to perform some authentication-specific work, we need it in every place where uses HttpClient. From scanning the source code, it looks like we'd need to add something to ConcurrentUpdateSolrServer and SolrDispatchFilter and possibly SolrCLI. Instead of the HttpClientCallback being a function of the HttpSolrServer, it's really a function of the AuthCredentials implementation I should mention, perhaps AuthCredentials is not a good name here if it's credentials + callback. Maybe AuthenticationMethod or AuthenticationType or something.
          Hide
          Per Steffensen added a comment -

          Sorry I do not have the time to go into details. But from quick reading, and as I remember the SOLR-4470 implementation, most/all of what you say sounds reasonable.

          Please note that SOLR-4470 hasnt been committed to the Apache Solr code-base. I provided it long time ago, and just recently Jan Høydahl and I updated it to fit tip of trunk, and I know Jan intended to try to push it to the code-base. Do not know what happened after that.

          Please also note that in SOLR-4470 I tried to prepare for additional authentication types, but it is hard to make it 100% right when you do not know the nature of the actual types being implemented in the future. Essence is that AuthCredentials should carry [information about] the authentications to be "used" for the request(s). How to "use" them is an implementation-detail of the specific authentication type (implementing AbstractAuthMethod), and it may require a little rearranging of code to implement authentication type #2. Basing it on a general callback feature sound like good idea. I believe in "never design for the future", but if I didnt at least try to sketch the idea in a "framework", there is a big risk than the next authentication type would be implemented in a completely different way. I also believe in "separation of concerns", so I would really like the "authentication concern" to be handled in one single place - AuthCredentials was my attempt to make such "a place".

          Show
          Per Steffensen added a comment - Sorry I do not have the time to go into details. But from quick reading, and as I remember the SOLR-4470 implementation, most/all of what you say sounds reasonable. Please note that SOLR-4470 hasnt been committed to the Apache Solr code-base. I provided it long time ago, and just recently Jan Høydahl and I updated it to fit tip of trunk, and I know Jan intended to try to push it to the code-base. Do not know what happened after that. Please also note that in SOLR-4470 I tried to prepare for additional authentication types, but it is hard to make it 100% right when you do not know the nature of the actual types being implemented in the future. Essence is that AuthCredentials should carry [information about] the authentications to be "used" for the request(s). How to "use" them is an implementation-detail of the specific authentication type (implementing AbstractAuthMethod ), and it may require a little rearranging of code to implement authentication type #2. Basing it on a general callback feature sound like good idea. I believe in "never design for the future", but if I didnt at least try to sketch the idea in a "framework", there is a big risk than the next authentication type would be implemented in a completely different way. I also believe in "separation of concerns", so I would really like the "authentication concern" to be handled in one single place - AuthCredentials was my attempt to make such "a place".
          Hide
          Gregory Chanan added a comment -

          Here's a new version of the patch, along the lines of what Per and I discussed above.

          Changes from previous patch:

          • Now works across HttpClient invocations, not just in HttpSolrServer. So raw HttpClient calls from the SolrDispatchFilter, ConcurrentUpdateSolrServer, SolrCLI are handled as well. There are unit tests for all the above except SolrCLI (for which I couldn't find a general test for)
          • Can be configured similar to the ZKCredentials. So the server side is configured via solr.xml, client side is configured via system properties.
          • We now pass a SolrRequest to the callback, to match the semantics of SOLR-4470. So it should be easy to incorporate SOLR-4470 at a later date.
          • Because we pass a SolrRequest to the callback, for the raw httpclient calls we generate a SolrRequest that matches the underlying call. These raw calls should probably use HttpSolrServer anyway – this is a step in that direction. This changed required adding support for more http methods, since the forwarded requests can send HEAD and DELETE, for example.
          Show
          Gregory Chanan added a comment - Here's a new version of the patch, along the lines of what Per and I discussed above. Changes from previous patch: Now works across HttpClient invocations, not just in HttpSolrServer. So raw HttpClient calls from the SolrDispatchFilter, ConcurrentUpdateSolrServer, SolrCLI are handled as well. There are unit tests for all the above except SolrCLI (for which I couldn't find a general test for) Can be configured similar to the ZKCredentials. So the server side is configured via solr.xml, client side is configured via system properties. We now pass a SolrRequest to the callback, to match the semantics of SOLR-4470 . So it should be easy to incorporate SOLR-4470 at a later date. Because we pass a SolrRequest to the callback, for the raw httpclient calls we generate a SolrRequest that matches the underlying call. These raw calls should probably use HttpSolrServer anyway – this is a step in that direction. This changed required adding support for more http methods, since the forwarded requests can send HEAD and DELETE, for example.
          Hide
          Gregory Chanan added a comment -

          Minor update, ensures the loader is passed to the UpdateShardHandler from CoreContainer.

          Show
          Gregory Chanan added a comment - Minor update, ensures the loader is passed to the UpdateShardHandler from CoreContainer.
          Hide
          Gregory Chanan added a comment -

          Adds a small test for the previous change, i.e. that the distributed requests actually get fed through the callback.

          Show
          Gregory Chanan added a comment - Adds a small test for the previous change, i.e. that the distributed requests actually get fed through the callback.
          Hide
          Gregory Chanan added a comment - - edited

          Put up a review request: https://reviews.apache.org/r/28826

          New patch allows the callback to throw an IOException; this allows the calling code to do something reasonable in error cases, i.e. if the callback makes an HttpRequest, the code in SolrDispatchFilter will print a good error message about it failing when trying to forward requests if it actually fails.

          The only part I'm hesitant about here is the code to generate a SolrRequest if one doesn't exist (in SolrCLI, SolrDipsatchFilter). This seems like a decent amount of complexity for little gain compared to just passing null. For my case, in the SolrDispatchFilter, I need to get some information from the SolrQueryRequest or HttpServletRequest (basically, the authenticated user that's available in the HttpServletRequest.getAttribute or SolrQueryRequest.getContext() or SolrQueryRequest.getParams()). But passing in either the SolrQueryRequest or HttpServletRequest doesn't seem like a good idea, because those are both really server-side data structures and the HttpClientRequestCallback is client side. We could have a different interface for the server-side stuff, but that doesn't seem less complex than generating a new SolrRequest. One alternative would be to pass a SolrParams or Map (i.e. SolrQueryRequest.getContext()) as another parameter, call it context, and make it clear from the documentation it is only useful on forwarded requests and can be ignored for clients. That would allow us to remove the code for HttpHead/HttpDelete support in SolrRequests.

          Anyway, that's my thinking right now, any feedback would be appreciated.

          Show
          Gregory Chanan added a comment - - edited Put up a review request: https://reviews.apache.org/r/28826 New patch allows the callback to throw an IOException; this allows the calling code to do something reasonable in error cases, i.e. if the callback makes an HttpRequest, the code in SolrDispatchFilter will print a good error message about it failing when trying to forward requests if it actually fails. The only part I'm hesitant about here is the code to generate a SolrRequest if one doesn't exist (in SolrCLI, SolrDipsatchFilter). This seems like a decent amount of complexity for little gain compared to just passing null. For my case, in the SolrDispatchFilter, I need to get some information from the SolrQueryRequest or HttpServletRequest (basically, the authenticated user that's available in the HttpServletRequest.getAttribute or SolrQueryRequest.getContext() or SolrQueryRequest.getParams()). But passing in either the SolrQueryRequest or HttpServletRequest doesn't seem like a good idea, because those are both really server-side data structures and the HttpClientRequestCallback is client side. We could have a different interface for the server-side stuff, but that doesn't seem less complex than generating a new SolrRequest. One alternative would be to pass a SolrParams or Map (i.e. SolrQueryRequest.getContext()) as another parameter, call it context, and make it clear from the documentation it is only useful on forwarded requests and can be ignored for clients. That would allow us to remove the code for HttpHead/HttpDelete support in SolrRequests. Anyway, that's my thinking right now, any feedback would be appreciated.
          Hide
          Mark Miller added a comment -

          I think the last option is best.

          Perhaps we could add this info to the current context info map on the SolrRequest, and pass that context to the call back.

          Show
          Mark Miller added a comment - I think the last option is best. Perhaps we could add this info to the current context info map on the SolrRequest, and pass that context to the call back.
          Hide
          Steve Davids added a comment -

          Is there any reason why we wouldn't just utilize HttpClient's HttpRequestInterceptor and HttpResponseInterceptor? It seems as though if we could just provide an HttpClientFactory that clients can implement/override it should provide enough hooks to have everyone customize HttpClient to their hearts delight.

          Show
          Steve Davids added a comment - Is there any reason why we wouldn't just utilize HttpClient's HttpRequestInterceptor and HttpResponseInterceptor ? It seems as though if we could just provide an HttpClientFactory that clients can implement/override it should provide enough hooks to have everyone customize HttpClient to their hearts delight.
          Hide
          Gregory Chanan added a comment -

          The reason I didn't take that approach is that in some cases there's not enough context to figure out what you need to do. It's essentially equivalent to just passing the HttpRequest to the callback (and would thus be superior if that's all we needed), but consider the following cases:
          1) See SOLR-4470. That makes use of SolrRequest.getPreemptiveAuthentication, so you'd need the actual SolrRequest
          2) See the discussion above about SolrDispatchFilter. In that case, the client needs to get the context of the SolrQueryRequest

          Show
          Gregory Chanan added a comment - The reason I didn't take that approach is that in some cases there's not enough context to figure out what you need to do. It's essentially equivalent to just passing the HttpRequest to the callback (and would thus be superior if that's all we needed), but consider the following cases: 1) See SOLR-4470 . That makes use of SolrRequest.getPreemptiveAuthentication, so you'd need the actual SolrRequest 2) See the discussion above about SolrDispatchFilter. In that case, the client needs to get the context of the SolrQueryRequest
          Hide
          Steve Davids added a comment -

          See SOLR-4470. That makes use of SolrRequest.getPreemptiveAuthentication, so you'd need the actual SolrRequest

          I took a look at the patch but not quite sure any of that is actually necessary. Looking at the "what" detailed in SOLR-4470 they need to be able to lock down Solr Cloud via BasicAuth, you can easily do this via HttpClient's BasicScheme authentication scheme. Likewise you can see all of the various other authentication schemes HttpClient supports (SPNego included). This would seem to do the trick, except for perhaps propagating the authentication from the original request though this shouldn't really be a requirement since the challenge will be static in the web container that you can live with having static credentials in the solr distrib - if it changes deploy new config changes.

          For further information on authentication via HttpClient check out their help page here: http://hc.apache.org/httpcomponents-client-ga/tutorial/html/authentication.html

          See the discussion above about SolrDispatchFilter. In that case, the client needs to get the context of the SolrQueryRequest... For my case, in the SolrDispatchFilter, I need to get some information from the SolrQueryRequest or HttpServletRequest (basically, the authenticated user that's available in the HttpServletRequest.getAttribute or SolrQueryRequest.getContext() or SolrQueryRequest.getParams())

          Are you using these credentials to execute distributed requests? Or would it make sense to have a certain user hit the frontend shard, then that shard will perform the distributed request on behalf of the system's authentication credentials?

          Show
          Steve Davids added a comment - See SOLR-4470 . That makes use of SolrRequest.getPreemptiveAuthentication, so you'd need the actual SolrRequest I took a look at the patch but not quite sure any of that is actually necessary. Looking at the "what" detailed in SOLR-4470 they need to be able to lock down Solr Cloud via BasicAuth, you can easily do this via HttpClient's BasicScheme authentication scheme. Likewise you can see all of the various other authentication schemes HttpClient supports (SPNego included). This would seem to do the trick, except for perhaps propagating the authentication from the original request though this shouldn't really be a requirement since the challenge will be static in the web container that you can live with having static credentials in the solr distrib - if it changes deploy new config changes. For further information on authentication via HttpClient check out their help page here: http://hc.apache.org/httpcomponents-client-ga/tutorial/html/authentication.html See the discussion above about SolrDispatchFilter. In that case, the client needs to get the context of the SolrQueryRequest... For my case, in the SolrDispatchFilter, I need to get some information from the SolrQueryRequest or HttpServletRequest (basically, the authenticated user that's available in the HttpServletRequest.getAttribute or SolrQueryRequest.getContext() or SolrQueryRequest.getParams()) Are you using these credentials to execute distributed requests? Or would it make sense to have a certain user hit the frontend shard, then that shard will perform the distributed request on behalf of the system's authentication credentials?
          Hide
          Gregory Chanan added a comment -

          This would seem to do the trick, except for perhaps propagating the authentication from the original request though this shouldn't really be a requirement since the challenge will be static in the web container that you can live with having static credentials in the solr distrib - if it changes deploy new config changes.

          Right, as far as I understand it, that part is there to propagate the authentication from the original request. Maybe Per Steffensen can chime in with the motivation there rather than always using the static credentials – I'm guessing it is because there is some organization-specific authorization checking expecting the actual user who made the original request.

          This patch is based on what we do with Apache Sentry and is a little different than the SOLR-4470 approach, I'll explain that below. I put the SolrRequest param in the callback because it was easy, we have evidence that other people who implemented authentication find it useful to have access to the SolrRequest, and it is compatible with SOLR-4470.

          Likewise you can see all of the various other authentication schemes HttpClient supports (SPNego included)

          Yep, we are actually using HttpClient's SPNego support – I plan on contributing that in separate patch. One difference is Solr doesn't use repeatable requests, so we need to address that in some way (e.g. use BufferedEntity like SOLR-4470). That part seems like it could be implemented either via this HttpClientRequestCallback or an HttpRequestInterceptor.

          Are you using these credentials to execute distributed requests? Or would it make sense to have a certain user hit the frontend shard, then that shard will perform the distributed request on behalf of the system's authentication credentials?

          The later is close to correct. In Sentry we have some request handlers that do the checking based on the authenticated user, so if you make a request to a shard and the request is not forwarded, everything works as expected. All solr-to-solr requests are done via internal (solr) credentials; we may have used SOLR-4470 if it were available in the version we were developing against and it was tested with SPNego, but it wasn't and this approach is simpler. So this means in the forwarded case, the request looks like it came from solr and thus you can't do proper authorization. To get around this, we use secure impersonation (http://hadoop.apache.org/docs/r1.2.1/Secure_Impersonation.html) to make the forwarded request look like it came from the original authenticated user. That's what the part about including the context from the SolrQueryRequest is about above.

          Now, you could argue that these concerns should be separated – i.e. there should be some sort of forwarded request callback for handling the authenticated user for that specific case and the HttpRequestInterceptor should handle the http concerns around BufferedEntities and such. I'm not totally against that, but I'd say that the HttpClientRequestCallback is compatible with two independently developed authentication/authorization implementations (Sentry and SOLR-4470), and the separated ForwardedCallback/HttpRequestInterceptor approach doesn't seem simpler. And you can still use HttpRequestInterceptors if you want to write your own HttpClientConfigurer.

          Hopefully that addresses your questions Steve Davids, any feedback is appreciated.

          Show
          Gregory Chanan added a comment - This would seem to do the trick, except for perhaps propagating the authentication from the original request though this shouldn't really be a requirement since the challenge will be static in the web container that you can live with having static credentials in the solr distrib - if it changes deploy new config changes. Right, as far as I understand it, that part is there to propagate the authentication from the original request. Maybe Per Steffensen can chime in with the motivation there rather than always using the static credentials – I'm guessing it is because there is some organization-specific authorization checking expecting the actual user who made the original request. This patch is based on what we do with Apache Sentry and is a little different than the SOLR-4470 approach, I'll explain that below. I put the SolrRequest param in the callback because it was easy, we have evidence that other people who implemented authentication find it useful to have access to the SolrRequest, and it is compatible with SOLR-4470 . Likewise you can see all of the various other authentication schemes HttpClient supports (SPNego included) Yep, we are actually using HttpClient's SPNego support – I plan on contributing that in separate patch. One difference is Solr doesn't use repeatable requests, so we need to address that in some way (e.g. use BufferedEntity like SOLR-4470 ). That part seems like it could be implemented either via this HttpClientRequestCallback or an HttpRequestInterceptor. Are you using these credentials to execute distributed requests? Or would it make sense to have a certain user hit the frontend shard, then that shard will perform the distributed request on behalf of the system's authentication credentials? The later is close to correct. In Sentry we have some request handlers that do the checking based on the authenticated user, so if you make a request to a shard and the request is not forwarded, everything works as expected. All solr-to-solr requests are done via internal (solr) credentials; we may have used SOLR-4470 if it were available in the version we were developing against and it was tested with SPNego, but it wasn't and this approach is simpler. So this means in the forwarded case, the request looks like it came from solr and thus you can't do proper authorization. To get around this, we use secure impersonation ( http://hadoop.apache.org/docs/r1.2.1/Secure_Impersonation.html ) to make the forwarded request look like it came from the original authenticated user. That's what the part about including the context from the SolrQueryRequest is about above. Now, you could argue that these concerns should be separated – i.e. there should be some sort of forwarded request callback for handling the authenticated user for that specific case and the HttpRequestInterceptor should handle the http concerns around BufferedEntities and such. I'm not totally against that, but I'd say that the HttpClientRequestCallback is compatible with two independently developed authentication/authorization implementations (Sentry and SOLR-4470 ), and the separated ForwardedCallback/HttpRequestInterceptor approach doesn't seem simpler. And you can still use HttpRequestInterceptors if you want to write your own HttpClientConfigurer. Hopefully that addresses your questions Steve Davids , any feedback is appreciated.
          Hide
          Per Steffensen added a comment -

          Maybe Per Steffensen can chime in with the motivation there rather than always using the static credentials – I'm guessing it is because there is some organization-specific authorization checking expecting the actual user who made the original request.

          • Every paranoid customers. In general we/they just do not want a user to be able to indirectly (through distributed sub-requests) get a response to a query he is not allowed to fire directly. There is a lot of details to it that I will spare you, but overall we are also 1) using authorization to prevent specific groups of users to do queries (directly or indirectly) that takes certain amounts of resources from the system or 2) to fetch certain fields or any information that can be derived from those fields. Ad 1) By a lot of testing we know which types of queries really takes a lot of resources out of our system, but it is hard to be sure that a query that does not seem to be such a query, does not indirectly fire such queries in distributed sub-requests. Ad 2) Just because an outer request does not specifically ask for a certain field, it is hard to know that the indirect sub-request also does not, even though in practice it is so (except for id an score)
          • Distributed audit logging
          • Etc.

          I realize that the reasons we want the credentials from the outer request forwarded in sub-requests are very thin. I understand if this is not something that you feel is necessary to support in Apache Solr. But if it does not make the solution much more complicated you might just support it. I do not think SOLR-4470 is a complicated solution - most of it, really, is testing.

          Likewise you can see all of the various other authentication schemes HttpClient supports (SPNego included)

          SOLR-4470 does not try to do itself what HttpClient can already do. The HttpClient capabilities can just be used outside or through the authentication-framework introduced in SOLR-4470. SOLR-4470 is mostly about joggling the credentials inside Solr-nodes. When they have to be added to requests, of course use HttpClient capabilities.

          To get around this, we use secure impersonation

          Interesting!

          Show
          Per Steffensen added a comment - Maybe Per Steffensen can chime in with the motivation there rather than always using the static credentials – I'm guessing it is because there is some organization-specific authorization checking expecting the actual user who made the original request. Every paranoid customers. In general we/they just do not want a user to be able to indirectly (through distributed sub-requests) get a response to a query he is not allowed to fire directly. There is a lot of details to it that I will spare you, but overall we are also 1) using authorization to prevent specific groups of users to do queries (directly or indirectly) that takes certain amounts of resources from the system or 2) to fetch certain fields or any information that can be derived from those fields. Ad 1) By a lot of testing we know which types of queries really takes a lot of resources out of our system, but it is hard to be sure that a query that does not seem to be such a query, does not indirectly fire such queries in distributed sub-requests. Ad 2) Just because an outer request does not specifically ask for a certain field, it is hard to know that the indirect sub-request also does not, even though in practice it is so (except for id an score) Distributed audit logging Etc. I realize that the reasons we want the credentials from the outer request forwarded in sub-requests are very thin. I understand if this is not something that you feel is necessary to support in Apache Solr. But if it does not make the solution much more complicated you might just support it. I do not think SOLR-4470 is a complicated solution - most of it, really, is testing. Likewise you can see all of the various other authentication schemes HttpClient supports (SPNego included) SOLR-4470 does not try to do itself what HttpClient can already do. The HttpClient capabilities can just be used outside or through the authentication-framework introduced in SOLR-4470 . SOLR-4470 is mostly about joggling the credentials inside Solr-nodes. When they have to be added to requests, of course use HttpClient capabilities. To get around this, we use secure impersonation Interesting!
          Hide
          Per Steffensen added a comment -

          Had a few hours for a look at the latest patch. Not a lot of time so I might very well have overlooked something, so please bear with me. But from what I got out of it I have the following comments

          It seems you can control the used callbacks in two ways

          • 1) Through solr.xml.
            • Request that SolrDispatchFilter forwards (using remoteQuery)
            • Update requests sent through UpdateShardHandler - DistributedUpdateProcessor, SnapPuller, PeerSync, SyncStrategy ...
            • Overseer requests (through UpdateShardHandler via ZkController)
          • 2) Through VM parameter. Used for every request sent - from Solr nodes and clients or whatever is running in the JVM sending request using HttpSolrServer (or ConcurrentUpdateSolrServer). Believe, for the requests mentioned under 1) above, both callbacks (the one from solr.xml AND the one from VM parameter) will be triggered - is that intentional? (it is fine for me)
            I think, that if you want to support setting the callback through solr.xml it should be used for all requests sent out of a Solr node - including e.g. search requests to shards issued from SearchHandler (by code inspection it seems to me that it will not currently be included here). Regarding this principle of using the solr.xml-callback for ALL requests going out of a Solr node, I would really like to see some testing that makes sure this principle is fulfilled now and that it is not broken in the future (the day after SOLR-6625 is committed and forgotten about, someone adds a new place in the code sending requests going out of Solr nodes and forgets to call the callbacks). I wanted to do the same in SOLR-4470 (making sure that no requests going out of Solr nodes do not have credentials added - not it the code as it is today nor in the code as it changes in the future). I achieved it by adding a protecting layer around every Solr node ever started during testing, and assuming that the test-suite triggers every type of requests going out of Solr nodes, that will basically ensure that no one forgets to propagate credentials when extending the code in the future (those requests will fail and the test will likely fail). I would really like to see something similar here, verifying that solr.xml-callback is triggered for every request going out of a Solr node.

          Maybe you should consider the naming of distribCount and forwardCount in BasicDistributedZk2Test.HttpClientRequestCallbackCounter. They are very confusing to me. distribCount seem to count update-requests (they are kinda forwarded) and forwardCount seem to count everything else including request from e.g. Overseer (they are among those that can not be characterized as forwarded)

          I know you are not keen on cleaning up while adding a new feature (I was told that we have to do cleanup in different JIRAs). Personally I am very much against that, because no one will ever make a JIRA just to clean up (at least, looking at the code-base I see that it does not happen enough). Assuming we want to do a little cleanup (in that code that is touched anyway in this JIRA) I have the following suggestions

          • Change ConcurrentUpdateSolrServer to use HttpSolrServer or at least make them share common code. Then we do not have "remember" to add callback-support in both when we solve this SOLR-6625
          • In general we should do some "Separation of Concerns" here and create a single component X (e.g. SolrNodeOutgoingRequestSender) concerned with sending requests out of Solr nodes. Make X use callback from solr.xml.
            • Let SolrDispatchFilter, UpdateShardHandler and whatever sends requests out of Solr nodes use that component X, or something based on or using it
            • It is strange that Overseer uses UpdateShardHandler - Overseer is not doing updates. Either rename UpdateShardHandler to something that does not signal that it is only used for update-requests, or let Overseer use something else based on component X

          Instead of adding callback-calls numerous places in the code not knowing if some place was forgotten, polluting the code even more and leaving the same kind of problems for others doing similar things in the future, IMHO SOLR-6625 should be solved by refactoring the code so that the actual feature of SOLR-6625 is ridiculously easy to add

          • Separating the general concern of sending request from solr-code (server or client) to one single component Y. (HttpSolrServer or a class containing the code shared between it and ConcurrentUpdateSolrServer is fine for now). It is easy to make a test that ensures that HttpClient.execute only occurs one single place in the (non-test) code
          • Separating the concern of sending requests going out from Solr nodes to one single component X (of course using Y). Make the test-suite verify that Solr-nodes never ever send request bypassing this component Y - maybe inspired by the way it is done in SOLR-4470 (here the test is verifying that Solr-nodes never ever sends requests without adding credentials - potato potato)
          • Now it is ridiculously easy to solve SOLR-6625 (and you have used your strength cleaning up the code instead of polluting it even more): Make X capable of taking a list L of callbacks, and let it call those callbacks plus the VM-parameter based callbacks. Make sure Y passes the solr.xml-callbacks in this list L when it uses X. Testing is almost superfluous, because you know that requests from solr-code are always sent using X (which calls VM-parameter callbacks) and that requests going out of Solr nodes are already sent using Y (which makes sure X calls solr.xml-callbacks). Maybe we few simple tests, e.g. a test that X always uses VM-parameter callbacks, and a test that Y actually makes X use the solr.xml-callbacks.

          Yes, I deliberately said callbackS. I think there should be support for a list of callbacks, so that people using this general feature will be able to do proper separation of concerns if they want to do several conceptually different things using callbacks

          If SOLR-6625 should be able to be used directly for a SOLR-4470 solution, we need to include the "super-request" (the request that indirectly triggered the request about to be fired) in the call to the callback. I understand if you do not want to deal with that now - we can deal with that later.

          I really like the nice general feature you are making here - one vote from me! Looking very much forward to see how it ends.

          Show
          Per Steffensen added a comment - Had a few hours for a look at the latest patch. Not a lot of time so I might very well have overlooked something, so please bear with me. But from what I got out of it I have the following comments It seems you can control the used callbacks in two ways 1) Through solr.xml . Request that SolrDispatchFilter forwards (using remoteQuery ) Update requests sent through UpdateShardHandler - DistributedUpdateProcessor , SnapPuller , PeerSync , SyncStrategy ... Overseer requests (through UpdateShardHandler via ZkController ) 2) Through VM parameter. Used for every request sent - from Solr nodes and clients or whatever is running in the JVM sending request using HttpSolrServer (or ConcurrentUpdateSolrServer ). Believe, for the requests mentioned under 1) above, both callbacks (the one from solr.xml AND the one from VM parameter) will be triggered - is that intentional? (it is fine for me) I think, that if you want to support setting the callback through solr.xml it should be used for all requests sent out of a Solr node - including e.g. search requests to shards issued from SearchHandler (by code inspection it seems to me that it will not currently be included here). Regarding this principle of using the solr.xml -callback for ALL requests going out of a Solr node, I would really like to see some testing that makes sure this principle is fulfilled now and that it is not broken in the future (the day after SOLR-6625 is committed and forgotten about, someone adds a new place in the code sending requests going out of Solr nodes and forgets to call the callbacks). I wanted to do the same in SOLR-4470 (making sure that no requests going out of Solr nodes do not have credentials added - not it the code as it is today nor in the code as it changes in the future). I achieved it by adding a protecting layer around every Solr node ever started during testing, and assuming that the test-suite triggers every type of requests going out of Solr nodes, that will basically ensure that no one forgets to propagate credentials when extending the code in the future (those requests will fail and the test will likely fail). I would really like to see something similar here, verifying that solr.xml -callback is triggered for every request going out of a Solr node. Maybe you should consider the naming of distribCount and forwardCount in BasicDistributedZk2Test.HttpClientRequestCallbackCounter . They are very confusing to me. distribCount seem to count update-requests (they are kinda forwarded) and forwardCount seem to count everything else including request from e.g. Overseer (they are among those that can not be characterized as forwarded) I know you are not keen on cleaning up while adding a new feature (I was told that we have to do cleanup in different JIRAs). Personally I am very much against that, because no one will ever make a JIRA just to clean up (at least, looking at the code-base I see that it does not happen enough). Assuming we want to do a little cleanup (in that code that is touched anyway in this JIRA) I have the following suggestions Change ConcurrentUpdateSolrServer to use HttpSolrServer or at least make them share common code. Then we do not have "remember" to add callback-support in both when we solve this SOLR-6625 In general we should do some "Separation of Concerns" here and create a single component X (e.g. SolrNodeOutgoingRequestSender ) concerned with sending requests out of Solr nodes. Make X use callback from solr.xml . Let SolrDispatchFilter , UpdateShardHandler and whatever sends requests out of Solr nodes use that component X, or something based on or using it It is strange that Overseer uses UpdateShardHandler - Overseer is not doing updates. Either rename UpdateShardHandler to something that does not signal that it is only used for update-requests, or let Overseer use something else based on component X Instead of adding callback-calls numerous places in the code not knowing if some place was forgotten, polluting the code even more and leaving the same kind of problems for others doing similar things in the future, IMHO SOLR-6625 should be solved by refactoring the code so that the actual feature of SOLR-6625 is ridiculously easy to add Separating the general concern of sending request from solr-code (server or client) to one single component Y. ( HttpSolrServer or a class containing the code shared between it and ConcurrentUpdateSolrServer is fine for now). It is easy to make a test that ensures that HttpClient.execute only occurs one single place in the (non-test) code Separating the concern of sending requests going out from Solr nodes to one single component X (of course using Y). Make the test-suite verify that Solr-nodes never ever send request bypassing this component Y - maybe inspired by the way it is done in SOLR-4470 (here the test is verifying that Solr-nodes never ever sends requests without adding credentials - potato potato) Now it is ridiculously easy to solve SOLR-6625 (and you have used your strength cleaning up the code instead of polluting it even more): Make X capable of taking a list L of callbacks, and let it call those callbacks plus the VM-parameter based callbacks. Make sure Y passes the solr.xml -callbacks in this list L when it uses X. Testing is almost superfluous, because you know that requests from solr-code are always sent using X (which calls VM-parameter callbacks) and that requests going out of Solr nodes are already sent using Y (which makes sure X calls solr.xml -callbacks). Maybe we few simple tests, e.g. a test that X always uses VM-parameter callbacks, and a test that Y actually makes X use the solr.xml -callbacks. Yes, I deliberately said callbackS. I think there should be support for a list of callbacks, so that people using this general feature will be able to do proper separation of concerns if they want to do several conceptually different things using callbacks If SOLR-6625 should be able to be used directly for a SOLR-4470 solution, we need to include the "super-request" (the request that indirectly triggered the request about to be fired) in the call to the callback. I understand if you do not want to deal with that now - we can deal with that later. I really like the nice general feature you are making here - one vote from me! Looking very much forward to see how it ends.
          Hide
          Per Steffensen added a comment - - edited

          Updated patch to HEAD of branch_x5. Not a completely trivial task. Did not try to make any of the changes I suggested in previous comments. Just tried to make a 1-to-1 upgrade to HEAD of branch_5x. Please someone verify that I did not miss anything.

          Show
          Per Steffensen added a comment - - edited Updated patch to HEAD of branch_x5. Not a completely trivial task. Did not try to make any of the changes I suggested in previous comments. Just tried to make a 1-to-1 upgrade to HEAD of branch_5x. Please someone verify that I did not miss anything.
          Hide
          Per Steffensen added a comment - - edited

          New patch

          • Support for multiple callbacks (for nicer separation of concerns) - both for sys-prop-callbacks and for solr.xml-callbacks. preExecuteRequest lets callbacks build on contexts from each other - a logical consequence of introducing support for multiple callbacks
          • Solr-nodes now use both sol.xml-callbacks AND sys-prop-callbacks
          • SolrCLI.getJson attempts-variant now actually uses the callbacks given to it (via sys-prop)
          • Cleaning up some of the code-duplication. Separating more of the code related to this issue in HttpClientRequestCallbackUtil
          Show
          Per Steffensen added a comment - - edited New patch Support for multiple callbacks (for nicer separation of concerns) - both for sys-prop-callbacks and for solr.xml-callbacks. preExecuteRequest lets callbacks build on contexts from each other - a logical consequence of introducing support for multiple callbacks Solr-nodes now use both sol.xml-callbacks AND sys-prop-callbacks SolrCLI.getJson attempts-variant now actually uses the callbacks given to it (via sys-prop) Cleaning up some of the code-duplication. Separating more of the code related to this issue in HttpClientRequestCallbackUtil
          Hide
          Ishan Chattopadhyaya added a comment -

          If I'm not missing something, I think we don't need to create our own callback framework in Solr and instead leverage the HttpRequestInterceptor / HttpResponseInterceptor. Please excuse my ignorance if this has been discussed above and I've missed it.

          The reason I didn't take that approach is that in some cases there's not enough context to figure out what you need to do.

          The SolrRequest / SolrQueryRequest object (and whatever else we want) can be passed into the interceptor's context. And hence, any authentication info that needs to be propagated from original request to sub-requests (internode requests) can be part of the interceptor. Please see the attached patch (SOLR-6625_interceptor.patch). It contains a test method in the MiniSolrClusterTest which uses an request interceptor to capture the SolrRequest object.

          Show
          Ishan Chattopadhyaya added a comment - If I'm not missing something, I think we don't need to create our own callback framework in Solr and instead leverage the HttpRequestInterceptor / HttpResponseInterceptor. Please excuse my ignorance if this has been discussed above and I've missed it. The reason I didn't take that approach is that in some cases there's not enough context to figure out what you need to do. The SolrRequest / SolrQueryRequest object (and whatever else we want) can be passed into the interceptor's context. And hence, any authentication info that needs to be propagated from original request to sub-requests (internode requests) can be part of the interceptor. Please see the attached patch ( SOLR-6625 _interceptor.patch). It contains a test method in the MiniSolrClusterTest which uses an request interceptor to capture the SolrRequest object.
          Hide
          Gregory Chanan added a comment -

          Ishan Chattopadhyaya thanks for posting the patch. As far as I can tell, this replaces our own interface/abstract class for callbacks with an HttpRequestInterceptor. That seems reasonable, but there are a few issues discussed on this JIRA that don't seem addressed. A somewhat comprehensive list:

          1. How is configuration handled? Is it the responsibility of the authentication layer to specify the HttpRequestInterceptor to use for all requests?
          2. There is some question about passing in SolrRequests (client) vs SolrQueryRequests (server). Presumably the authentication layer needs to operate on the client side (often the authentication requirements will be the same), but what are you supposed to do with the SolrQueryRequest? You've sort of hidden it by putting it in the context, but that seems fragile; we should have our own Context that makes clear what is available, but if you pull in SolrQueryRequest, then clients need access to server data structures, which isn't ideal. Perhaps the right thing to do is split up client vs server interceptors?
          3. Can the request interceptor do everything our own callback can do? There are sample tests in the patch which add cookies, modify the URL, etc. It would be good to see those tests with the new implementation
          4. How do you check that we actually pass the correct information to the interceptor? Per Steffensen described this above; what if someone adds another httpclient.execute call tomorrow and doesn't pass the SolrRequest object? We'll never know, so relying on the HttpRequestInterceptor didn't really help us (it automatically dispatches but doesn't check that it has all the info it needs to dispatch). It just limits us to an interface we don't control. We need some way of enforcing the property that every call to httpclient has the information we need.

          Show
          Gregory Chanan added a comment - Ishan Chattopadhyaya thanks for posting the patch. As far as I can tell, this replaces our own interface/abstract class for callbacks with an HttpRequestInterceptor. That seems reasonable, but there are a few issues discussed on this JIRA that don't seem addressed. A somewhat comprehensive list: 1. How is configuration handled? Is it the responsibility of the authentication layer to specify the HttpRequestInterceptor to use for all requests? 2. There is some question about passing in SolrRequests (client) vs SolrQueryRequests (server). Presumably the authentication layer needs to operate on the client side (often the authentication requirements will be the same), but what are you supposed to do with the SolrQueryRequest? You've sort of hidden it by putting it in the context, but that seems fragile; we should have our own Context that makes clear what is available, but if you pull in SolrQueryRequest, then clients need access to server data structures, which isn't ideal. Perhaps the right thing to do is split up client vs server interceptors? 3. Can the request interceptor do everything our own callback can do? There are sample tests in the patch which add cookies, modify the URL, etc. It would be good to see those tests with the new implementation 4. How do you check that we actually pass the correct information to the interceptor? Per Steffensen described this above; what if someone adds another httpclient.execute call tomorrow and doesn't pass the SolrRequest object? We'll never know, so relying on the HttpRequestInterceptor didn't really help us (it automatically dispatches but doesn't check that it has all the info it needs to dispatch). It just limits us to an interface we don't control. We need some way of enforcing the property that every call to httpclient has the information we need.
          Hide
          Anshum Gupta added a comment -

          Thanks for the patch Ishan. I like the simplified approach but among other things that Greg pointed out, I'm most concerned about enforcing that every call to httpclient has the complete and correct information.

          Show
          Anshum Gupta added a comment - Thanks for the patch Ishan. I like the simplified approach but among other things that Greg pointed out, I'm most concerned about enforcing that every call to httpclient has the complete and correct information.
          Hide
          Ishan Chattopadhyaya added a comment - - edited

          Thanks for your review, Gregory Chanan and Anshum Gupta. I've added another patch, for your review.

          1. How is configuration handled? Is it the responsibility of the authentication layer to specify the HttpRequestInterceptor to use for all requests?

          My thought was that the authentication (SOLR-7274) and authorization (SOLR-7275) plugins would add their own interceptors, e.g. to pass in original request's user principal or ACL to a subrequest. The work of taking these things, i.e. user principal, ACL or whatever else is generated from authc/authz plugins, from original user request and adding it to SolrRequest/SolrQueryRequest could be dealt in another issue.

          2. There is some question about passing in SolrRequests (client) vs SolrQueryRequests (server). Presumably the authentication layer needs to operate on the client side (often the authentication requirements will be the same), but what are you supposed to do with the SolrQueryRequest? You've sort of hidden it by putting it in the context, but that seems fragile; we should have our own Context that makes clear what is available, but if you pull in SolrQueryRequest, then clients need access to server data structures, which isn't ideal. Perhaps the right thing to do is split up client vs server interceptors?

          In this new patch, I've created two context objects, SolrHttpContext (client, encapsulates SolrRequest) and SolrQueryRequestContext (server, encapsulates SolrQueryRequest). Based on which of the contexts is received, an interceptor can do its thing with these request objects.

          3. Can the request interceptor do everything our own callback can do? There are sample tests in the patch which add cookies, modify the URL, etc. It would be good to see those tests with the new implementation

          I've added similar tests to HttpSolrClient that were in the previous patches by you and Per Steffensen.

          4. How do you check that we actually pass the correct information to the interceptor? Per Steffensen described this above; what if someone adds another httpclient.execute call tomorrow and doesn't pass the SolrRequest object? We'll never know, so relying on the HttpRequestInterceptor didn't really help us (it automatically dispatches but doesn't check that it has all the info it needs to dispatch). It just limits us to an interface we don't control. We need some way of enforcing the property that every call to httpclient has the information we need.

          That's a good point, thanks for bringing it up, since I had missed it. One of the ways I could think of enforcing this was to add an interceptor that always checks if an httpclient.execute() call puts in a SolrHttpContext object, and throws an exception if not. Another way, we can forbid calls to httpclient.execute() without a SolrHttpContext context. In this patch, I've added done of these.

          Show
          Ishan Chattopadhyaya added a comment - - edited Thanks for your review, Gregory Chanan and Anshum Gupta . I've added another patch, for your review. 1. How is configuration handled? Is it the responsibility of the authentication layer to specify the HttpRequestInterceptor to use for all requests? My thought was that the authentication ( SOLR-7274 ) and authorization ( SOLR-7275 ) plugins would add their own interceptors, e.g. to pass in original request's user principal or ACL to a subrequest. The work of taking these things, i.e. user principal, ACL or whatever else is generated from authc/authz plugins, from original user request and adding it to SolrRequest/SolrQueryRequest could be dealt in another issue. 2. There is some question about passing in SolrRequests (client) vs SolrQueryRequests (server). Presumably the authentication layer needs to operate on the client side (often the authentication requirements will be the same), but what are you supposed to do with the SolrQueryRequest? You've sort of hidden it by putting it in the context, but that seems fragile; we should have our own Context that makes clear what is available, but if you pull in SolrQueryRequest, then clients need access to server data structures, which isn't ideal. Perhaps the right thing to do is split up client vs server interceptors? In this new patch, I've created two context objects, SolrHttpContext (client, encapsulates SolrRequest) and SolrQueryRequestContext (server, encapsulates SolrQueryRequest). Based on which of the contexts is received, an interceptor can do its thing with these request objects. 3. Can the request interceptor do everything our own callback can do? There are sample tests in the patch which add cookies, modify the URL, etc. It would be good to see those tests with the new implementation I've added similar tests to HttpSolrClient that were in the previous patches by you and Per Steffensen . 4. How do you check that we actually pass the correct information to the interceptor? Per Steffensen described this above; what if someone adds another httpclient.execute call tomorrow and doesn't pass the SolrRequest object? We'll never know, so relying on the HttpRequestInterceptor didn't really help us (it automatically dispatches but doesn't check that it has all the info it needs to dispatch). It just limits us to an interface we don't control. We need some way of enforcing the property that every call to httpclient has the information we need. That's a good point, thanks for bringing it up, since I had missed it. One of the ways I could think of enforcing this was to add an interceptor that always checks if an httpclient.execute() call puts in a SolrHttpContext object, and throws an exception if not. Another way, we can forbid calls to httpclient.execute() without a SolrHttpContext context. In this patch, I've added done of these.
          Hide
          Noble Paul added a comment -

          I think there are simpler ways to achieve this than banning the execute() method altogether.
          We can ban the creating of new HttpClient object and mandate the use of UpdateShardHandler.getHttpClient() . That can be arapped to set the right context. That way you don't have to make changes to the entire codebase

          Show
          Noble Paul added a comment - I think there are simpler ways to achieve this than banning the execute() method altogether. We can ban the creating of new HttpClient object and mandate the use of UpdateShardHandler.getHttpClient() . That can be arapped to set the right context. That way you don't have to make changes to the entire codebase
          Hide
          Ishan Chattopadhyaya added a comment - - edited

          I think banning creation of HttpClients can't solve the problem of validating that each request made through them have the request object in the context (or something that explicitly means that there was no context available to be passed in, e.g. SolrHttpContext.EMPTY_CONTEXT in the patch). Each call via an HttpClient may need to have a different context (if any), so I don't think a wrapped up HttpClient can be used with the right context.

          That way you don't have to make changes to the entire codebase

          Currently, these httpclient.execute() calls are, in the main source, in:

          1. SolrDispatchFilter (remote query)
          2. CUSC
          3. HttpSolrClient
          4. SolrCLI
          5. JarRepository
            (latter two don't have a request object to send around)

          And then there are tons of them in the tests. I think, for the tests only, we can try what you suggest; i.e. an extended HttpClient that overrides the execute() method to pass in an empty context.

          Show
          Ishan Chattopadhyaya added a comment - - edited I think banning creation of HttpClients can't solve the problem of validating that each request made through them have the request object in the context (or something that explicitly means that there was no context available to be passed in, e.g. SolrHttpContext.EMPTY_CONTEXT in the patch). Each call via an HttpClient may need to have a different context (if any), so I don't think a wrapped up HttpClient can be used with the right context. That way you don't have to make changes to the entire codebase Currently, these httpclient.execute() calls are, in the main source, in: SolrDispatchFilter (remote query) CUSC HttpSolrClient SolrCLI JarRepository (latter two don't have a request object to send around) And then there are tons of them in the tests. I think, for the tests only, we can try what you suggest; i.e. an extended HttpClient that overrides the execute() method to pass in an empty context.
          Hide
          Ishan Chattopadhyaya added a comment - - edited

          Here's an updated patch that does what I described above.
          Gregory Chanan, Anshum Gupta, Noble Paul, Can you please review?

          Show
          Ishan Chattopadhyaya added a comment - - edited Here's an updated patch that does what I described above. Gregory Chanan , Anshum Gupta , Noble Paul , Can you please review?
          Hide
          Noble Paul added a comment -

          Use a ThreadLocal to pass on the Context around and ensure that no one creates HttpClient directly

          Show
          Noble Paul added a comment - Use a ThreadLocal to pass on the Context around and ensure that no one creates HttpClient directly
          Hide
          Ishan Chattopadhyaya added a comment -

          Updated the patch to trunk. I think this is good to go, even without using a ThreadLocal. This should be useful for SOLR-7692.

          Show
          Ishan Chattopadhyaya added a comment - Updated the patch to trunk. I think this is good to go, even without using a ThreadLocal. This should be useful for SOLR-7692 .
          Hide
          Ishan Chattopadhyaya added a comment -

          Minor fix, was causing a test failure.

          Show
          Ishan Chattopadhyaya added a comment - Minor fix, was causing a test failure.
          Hide
          Anshum Gupta added a comment -

          Thanks for updating this Ishan but I wonder how this would work in case of a custom component/handler where the restrictions imposed by checking for context == null and forbidden APIs. It still wouldn't guarantee that the context is used in all cases.

          Also, I'm assuming you didn't run precommit as there was a use of .execute() without the context in the code (mentioned below). If you did, we might be missing something even more.

          I just did a quick review and here's a bit of feedback:

          • SolrQueryRequestContext.java can you remove all the Solr prefixes?
          • HttpClientUtil.interceptors should be made final. Also it's not threadsafe.
          • AbstractFullDistribZkTestBase.createNewSolrClient(int port) have client.setSoTimeout instead of using the HttpClientUtil. Same for TestReplicationHandlerBackup, and TestRestoreCore.
          • BasicHttpSolrClientTest.changeRequestInterceptor has redundant declaration for cookieName and cookieValue.
          • HttpSolrCall.remoteQuery declares a context but doesn't use it in the .execute() call.
          • In SolrJettyTestBase.SolrContextHttpClient.execute methods you don't need to throw ClientProtocolException as it has IOException in the throws spec already. It also has a few unused imports.
          • Unused imports have been introduced in SolrSchemalessExampleTest.
          Show
          Anshum Gupta added a comment - Thanks for updating this Ishan but I wonder how this would work in case of a custom component/handler where the restrictions imposed by checking for context == null and forbidden APIs. It still wouldn't guarantee that the context is used in all cases. Also, I'm assuming you didn't run precommit as there was a use of .execute() without the context in the code (mentioned below). If you did, we might be missing something even more. I just did a quick review and here's a bit of feedback: SolrQueryRequestContext.java can you remove all the Solr prefixes? HttpClientUtil.interceptors should be made final. Also it's not threadsafe. AbstractFullDistribZkTestBase.createNewSolrClient(int port) have client.setSoTimeout instead of using the HttpClientUtil. Same for TestReplicationHandlerBackup, and TestRestoreCore. BasicHttpSolrClientTest.changeRequestInterceptor has redundant declaration for cookieName and cookieValue. HttpSolrCall.remoteQuery declares a context but doesn't use it in the .execute() call. In SolrJettyTestBase.SolrContextHttpClient.execute methods you don't need to throw ClientProtocolException as it has IOException in the throws spec already. It also has a few unused imports. Unused imports have been introduced in SolrSchemalessExampleTest.
          Hide
          Ishan Chattopadhyaya added a comment -

          Hi Anshum, thanks a lot for your review!

          I wonder how this would work in case of a custom component/handler where the restrictions imposed by checking for context == null and forbidden APIs. It still wouldn't guarantee that the context is used in all cases.

          I think even if there is custom code (within Solr in the form of plugins, or outside Solr which uses SolrJ/HttpSolrClient), it would be asserted that an httpclient, created using HttpClientUtil, should always use a context. Do you suggest we do something more for the case of custom code?

          Also, I'm assuming you didn't run precommit as there was a use of .execute() without the context in the code (mentioned below). If you did, we might be missing something even more.

          Right, I hadn't even run the full tests, but caught (in my previous patch) the execute() thing in HttpSolrCall during the full tests. Now, I have run the precommit and it runs fine after fixing the execute() at a few places.

          SolrQueryRequestContext.java can you remove all the Solr prefixes?

          Renamed to QueryRequestContext.

          HttpClientUtil.interceptors should be made final. Also it's not threadsafe.

          Made it final. Made it a synchronized list.

          AbstractFullDistribZkTestBase.createNewSolrClient(int port) have client.setSoTimeout instead of using the HttpClientUtil. Same for TestReplicationHandlerBackup, and TestRestoreCore.

          This is done because HttpSolrClient's httpClient was created outside of HSC, and hence the `client.setDefaultMaxConnectionsPerHost()` call would throw UnsupportedOperationException.

            public void setDefaultMaxConnectionsPerHost(int max) {
              if (internalClient) {
                HttpClientUtil.setMaxConnectionsPerHost(httpClient, max);
              } else {
                throw new UnsupportedOperationException(
                    "Client was created outside of HttpSolrServer");
              }
            }
          

          BasicHttpSolrClientTest.changeRequestInterceptor has redundant declaration for cookieName and cookieValue.

          Removed now.

          HttpSolrCall.remoteQuery declares a context but doesn't use it in the .execute() call.

          This was the "minor fix" in the above comment/updated patch Fixed already.

          In SolrJettyTestBase.SolrContextHttpClient.execute methods you don't need to throw ClientProtocolException as it has IOException in the throws spec already. It also has a few unused imports.

          Removed the ClientProtocolException.

          Unused imports have been introduced in SolrSchemalessExampleTest.

          Removed.

          Show
          Ishan Chattopadhyaya added a comment - Hi Anshum, thanks a lot for your review! I wonder how this would work in case of a custom component/handler where the restrictions imposed by checking for context == null and forbidden APIs. It still wouldn't guarantee that the context is used in all cases. I think even if there is custom code (within Solr in the form of plugins, or outside Solr which uses SolrJ/HttpSolrClient), it would be asserted that an httpclient, created using HttpClientUtil, should always use a context. Do you suggest we do something more for the case of custom code? Also, I'm assuming you didn't run precommit as there was a use of .execute() without the context in the code (mentioned below). If you did, we might be missing something even more. Right, I hadn't even run the full tests, but caught (in my previous patch) the execute() thing in HttpSolrCall during the full tests. Now, I have run the precommit and it runs fine after fixing the execute() at a few places. SolrQueryRequestContext.java can you remove all the Solr prefixes? Renamed to QueryRequestContext. HttpClientUtil.interceptors should be made final. Also it's not threadsafe. Made it final. Made it a synchronized list. AbstractFullDistribZkTestBase.createNewSolrClient(int port) have client.setSoTimeout instead of using the HttpClientUtil. Same for TestReplicationHandlerBackup, and TestRestoreCore. This is done because HttpSolrClient's httpClient was created outside of HSC, and hence the `client.setDefaultMaxConnectionsPerHost()` call would throw UnsupportedOperationException. public void setDefaultMaxConnectionsPerHost(int max) { if (internalClient) { HttpClientUtil.setMaxConnectionsPerHost(httpClient, max); } else { throw new UnsupportedOperationException( "Client was created outside of HttpSolrServer"); } } BasicHttpSolrClientTest.changeRequestInterceptor has redundant declaration for cookieName and cookieValue. Removed now. HttpSolrCall.remoteQuery declares a context but doesn't use it in the .execute() call. This was the "minor fix" in the above comment/updated patch Fixed already. In SolrJettyTestBase.SolrContextHttpClient.execute methods you don't need to throw ClientProtocolException as it has IOException in the throws spec already. It also has a few unused imports. Removed the ClientProtocolException. Unused imports have been introduced in SolrSchemalessExampleTest. Removed.
          Hide
          Ishan Chattopadhyaya added a comment -

          AbstractFullDistribZkTestBase.createNewSolrClient(int port) have client.setSoTimeout instead of using the HttpClientUtil

          I see what you mean. Updated the patch!

          Show
          Ishan Chattopadhyaya added a comment - AbstractFullDistribZkTestBase.createNewSolrClient(int port) have client.setSoTimeout instead of using the HttpClientUtil I see what you mean. Updated the patch!
          Hide
          Noble Paul added a comment -

          This makes the SolrQueryRequest available automatically in all threads in server

          Show
          Noble Paul added a comment - This makes the SolrQueryRequest available automatically in all threads in server
          Hide
          Ishan Chattopadhyaya added a comment -

          Updated the patch, using the SolrRequestInfo from previous patch (thanks Noble Paul). Now, instead of enforcing httpclient.execute(req, context), added a SolrHttpClient (and within it SolrDefaultHttpClient and SolrSystemDefaultHttpClient) that adds the request object from the SolrRequestInfo into an overridden execute() method transparently.

          Noble Paul, in your previous patch (the SolrRequestInfo part), there are a few exceptions/assertions logged:

          6564 ERROR (searcherExecutor-17-thread-1-processing-x:collection1 snapshot.20150730163745674 //tmp//solr.handler.TestRestoreCore_21A69B2D533EDA15-001//tempDir-002 collection1) [    x:collection1] o.a.s.c.SolrCore Previous SolrRequestInfo was not closed!  req=location=/tmp/solr.handler.TestRestoreCore_21A69B2D533EDA15-001/tempDir-002&command=restore
          6564 ERROR (searcherExecutor-17-thread-1-processing-x:collection1 snapshot.20150730163745674 //tmp//solr.handler.TestRestoreCore_21A69B2D533EDA15-001//tempDir-002 collection1) [    x:collection1] o.a.s.c.SolrCore java.lang.AssertionError
              at org.apache.solr.request.SolrRequestInfo.setRequestInfo(SolrRequestInfo.java:58)
              at org.apache.solr.search.SolrIndexSearcher.warm(SolrIndexSearcher.java:2206)
              at org.apache.solr.core.SolrCore$4.call(SolrCore.java:1817)
              at java.util.concurrent.FutureTask.run(FutureTask.java:266)
              at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor$1.run(ExecutorUtil.java:198)
              at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
              at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
              at java.lang.Thread.run(Thread.java:745)
          

          Can you please have look at it? I've included it as is in this current patch, and haven't looked into why this is happening, since the tests are passing anyway.

          Show
          Ishan Chattopadhyaya added a comment - Updated the patch, using the SolrRequestInfo from previous patch (thanks Noble Paul ). Now, instead of enforcing httpclient.execute(req, context), added a SolrHttpClient (and within it SolrDefaultHttpClient and SolrSystemDefaultHttpClient) that adds the request object from the SolrRequestInfo into an overridden execute() method transparently. Noble Paul , in your previous patch (the SolrRequestInfo part), there are a few exceptions/assertions logged: 6564 ERROR (searcherExecutor-17-thread-1-processing-x:collection1 snapshot.20150730163745674 //tmp//solr.handler.TestRestoreCore_21A69B2D533EDA15-001//tempDir-002 collection1) [ x:collection1] o.a.s.c.SolrCore Previous SolrRequestInfo was not closed! req=location=/tmp/solr.handler.TestRestoreCore_21A69B2D533EDA15-001/tempDir-002&command=restore 6564 ERROR (searcherExecutor-17-thread-1-processing-x:collection1 snapshot.20150730163745674 //tmp//solr.handler.TestRestoreCore_21A69B2D533EDA15-001//tempDir-002 collection1) [ x:collection1] o.a.s.c.SolrCore java.lang.AssertionError at org.apache.solr.request.SolrRequestInfo.setRequestInfo(SolrRequestInfo.java:58) at org.apache.solr.search.SolrIndexSearcher.warm(SolrIndexSearcher.java:2206) at org.apache.solr.core.SolrCore$4.call(SolrCore.java:1817) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor$1.run(ExecutorUtil.java:198) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745) Can you please have look at it? I've included it as is in this current patch, and haven't looked into why this is happening, since the tests are passing anyway.
          Hide
          Noble Paul added a comment -

          fixed the test error

          Show
          Noble Paul added a comment - fixed the test error
          Hide
          ASF subversion and git services added a comment -

          Commit 1693432 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1693432 ]

          SOLR-6625: Enable registering interceptors for the calls made using HttpClient and make the
          request object available at the interceptor context

          Show
          ASF subversion and git services added a comment - Commit 1693432 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1693432 ] SOLR-6625 : Enable registering interceptors for the calls made using HttpClient and make the request object available at the interceptor context
          Hide
          ASF subversion and git services added a comment -

          Commit 1693434 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1693434 ]

          SOLR-6625: formatting fixed

          Show
          ASF subversion and git services added a comment - Commit 1693434 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1693434 ] SOLR-6625 : formatting fixed
          Hide
          ASF subversion and git services added a comment -

          Commit 1693442 from Noble Paul in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1693442 ]

          SOLR-6625: Enable registering interceptors for the calls made using HttpClient and make the request object available at the interceptor context

          Show
          ASF subversion and git services added a comment - Commit 1693442 from Noble Paul in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1693442 ] SOLR-6625 : Enable registering interceptors for the calls made using HttpClient and make the request object available at the interceptor context
          Hide
          Steve Rowe added a comment -

          I'm seeing reproducible TestCloudSolrClientConnections failures on trunk and branch_5x on my Jenkins after commits on this issue. Here's the trunk one (http://jenkins.sarowe.net/job/Lucene-Solr-tests-trunk/1152/):

             [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=C37B717A18417443 -Dtests.slow=true -Dtests.locale=bg_BG -Dtests.timezone=SystemV/HST10 -Dtests.asserts=true -Dtests.file.encoding=UTF-8
             [junit4] ERROR   7.12s J1 | TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp <<<
             [junit4]    > Throwable #1: org.apache.solr.common.SolrException: A SolrHttpContext object must be passed in as context. Context: {http.request=org.apache.http.impl.client.RequestWrapper@eddf10a, http.request-config=[expectContinueEnabled=false, proxy=null, localAddress=null, cookieSpec=null, redirectsEnabled=true, relativeRedirectsAllowed=true, maxRedirects=50, circularRedirectsAllowed=false, authenticationEnabled=true, targetPreferredAuthSchemes=null, proxyPreferredAuthSchemes=null, connectionRequestTimeout=0, connectTimeout=0, socketTimeout=0, decompressionEnabled=true], http.auth.proxy-scope=state:UNCHALLENGED;, http.auth.credentials-provider={}, http.scheme-registry=org.apache.http.conn.scheme.SchemeRegistry@4861e7d6, http.cookie-spec=best-match, http.cookie-store=[], http.connection=org.apache.http.impl.conn.ManagedClientConnectionImpl@771b321c, http.auth.target-scope=state:UNCHALLENGED;, http.cookiespec-registry=org.apache.http.cookie.CookieSpecRegistry@1b78f4d2, http.target_host=https://127.0.0.1:51727, http.route={s}->https://127.0.0.1:51727, http.cookie-origin=[(secure)127.0.0.1:51727/solr/admin/collections], http.authscheme-registry=org.apache.http.auth.AuthSchemeRegistry@7acc1316}
             [junit4]    > 	at __randomizedtesting.SeedInfo.seed([C37B717A18417443:D897F2DA4A5D5186]:0)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpClientConfigurer$1.process(HttpClientConfigurer.java:92)
             [junit4]    > 	at org.apache.http.protocol.ImmutableHttpProcessor.process(ImmutableHttpProcessor.java:132)
             [junit4]    > 	at org.apache.http.protocol.HttpRequestExecutor.preProcess(HttpRequestExecutor.java:166)
             [junit4]    > 	at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:485)
             [junit4]    > 	at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:882)
             [junit4]    > 	at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82)
             [junit4]    > 	at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:107)
             [junit4]    > 	at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:55)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.executeMethod(HttpSolrClient.java:465)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:234)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:226)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:376)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:328)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1086)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:856)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:799)
             [junit4]    > 	at org.apache.solr.client.solrj.SolrClient.request(SolrClient.java:1220)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp(TestCloudSolrClientConnections.java:57)
             [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
          

          and the branch_5x one (http://jenkins.sarowe.net/job/Lucene-Solr-tests-5.x-Java8/892/):

             [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=FC64970124605306 -Dtests.slow=true -Dtests.locale=ar_LY -Dtests.timezone=Asia/Vientiane -Dtests.asserts=true -Dtests.file.encoding=UTF-8
             [junit4] ERROR   1.84s J1 | TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp <<<
             [junit4]    > Throwable #1: org.apache.solr.client.solrj.SolrServerException: java.lang.NullPointerException
             [junit4]    > 	at __randomizedtesting.SeedInfo.seed([FC64970124605306:E78814A1767C76C3]:0)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:414)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:328)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1086)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:856)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:799)
             [junit4]    > 	at org.apache.solr.client.solrj.SolrClient.request(SolrClient.java:1220)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp(TestCloudSolrClientConnections.java:57)
             [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
             [junit4]    > Caused by: java.lang.NullPointerException
             [junit4]    > 	at org.apache.solr.client.solrj.impl.BasicHttpSolrClientTest$2.process(BasicHttpSolrClientTest.java:672)
             [junit4]    > 	at org.apache.http.protocol.ImmutableHttpProcessor.process(ImmutableHttpProcessor.java:132)
             [junit4]    > 	at org.apache.http.protocol.HttpRequestExecutor.preProcess(HttpRequestExecutor.java:166)
             [junit4]    > 	at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:485)
             [junit4]    > 	at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:882)
             [junit4]    > 	at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82)
             [junit4]    > 	at org.apache.solr.util.SolrHttpClient$SolrSystemDefaultHttpClient.execute(SolrHttpClient.java:54)
             [junit4]    > 	at org.apache.solr.util.SolrHttpClient$SolrSystemDefaultHttpClient.execute(SolrHttpClient.java:45)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.executeMethod(HttpSolrClient.java:465)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:234)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:226)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:376)
             [junit4]    > 	... 45 more
          

          The above is from Jenkins on Linux, but when I run the repro line on OS X, I get a different exception:

             [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=FC64970124605306 -Dtests.slow=true -Dtests.locale=ar_LY -Dtests.timezone=Asia/Vientiane -Dtests.asserts=true -Dtests.file.encoding=UTF-8
             [junit4] ERROR   5.08s | TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp <<<
             [junit4]    > Throwable #1: org.apache.solr.common.SolrException: A SolrHttpContext object must be passed in as context. Context: {http.request=org.apache.http.impl.client.RequestWrapper@560c17c7, http.request-config=[expectContinueEnabled=false, proxy=null, localAddress=null, cookieSpec=null, redirectsEnabled=true, relativeRedirectsAllowed=true, maxRedirects=50, circularRedirectsAllowed=false, authenticationEnabled=true, targetPreferredAuthSchemes=null, proxyPreferredAuthSchemes=null, connectionRequestTimeout=0, connectTimeout=0, socketTimeout=0, decompressionEnabled=true], http.auth.proxy-scope=state:UNCHALLENGED;, http.auth.credentials-provider={}, http.scheme-registry=org.apache.http.conn.scheme.SchemeRegistry@13917b5, http.cookie-spec=best-match, http.cookie-store=[], http.connection=org.apache.http.impl.conn.ManagedClientConnectionImpl@492e6959, http.auth.target-scope=state:UNCHALLENGED;, http.cookiespec-registry=org.apache.http.cookie.CookieSpecRegistry@39eeeeba, http.target_host=https://127.0.0.1:52502, http.route={s}->https://127.0.0.1:52502, http.cookie-origin=[(secure)127.0.0.1:52502/solr/admin/collections], http.authscheme-registry=org.apache.http.auth.AuthSchemeRegistry@f6a646f}
             [junit4]    > 	at __randomizedtesting.SeedInfo.seed([FC64970124605306:E78814A1767C76C3]:0)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpClientConfigurer$1.process(HttpClientConfigurer.java:92)
             [junit4]    > 	at org.apache.http.protocol.ImmutableHttpProcessor.process(ImmutableHttpProcessor.java:132)
             [junit4]    > 	at org.apache.http.protocol.HttpRequestExecutor.preProcess(HttpRequestExecutor.java:166)
             [junit4]    > 	at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:485)
             [junit4]    > 	at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:882)
             [junit4]    > 	at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82)
             [junit4]    > 	at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:107)
             [junit4]    > 	at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:55)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.executeMethod(HttpSolrClient.java:465)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:234)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:226)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:376)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:328)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1086)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:856)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:799)
             [junit4]    > 	at org.apache.solr.client.solrj.SolrClient.request(SolrClient.java:1220)
             [junit4]    > 	at org.apache.solr.client.solrj.impl.TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp(TestCloudSolrClientConnections.java:57)
             [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
          
          Show
          Steve Rowe added a comment - I'm seeing reproducible TestCloudSolrClientConnections failures on trunk and branch_5x on my Jenkins after commits on this issue. Here's the trunk one ( http://jenkins.sarowe.net/job/Lucene-Solr-tests-trunk/1152/ ): [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=C37B717A18417443 -Dtests.slow=true -Dtests.locale=bg_BG -Dtests.timezone=SystemV/HST10 -Dtests.asserts=true -Dtests.file.encoding=UTF-8 [junit4] ERROR 7.12s J1 | TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp <<< [junit4] > Throwable #1: org.apache.solr.common.SolrException: A SolrHttpContext object must be passed in as context. Context: {http.request=org.apache.http.impl.client.RequestWrapper@eddf10a, http.request-config=[expectContinueEnabled=false, proxy=null, localAddress=null, cookieSpec=null, redirectsEnabled=true, relativeRedirectsAllowed=true, maxRedirects=50, circularRedirectsAllowed=false, authenticationEnabled=true, targetPreferredAuthSchemes=null, proxyPreferredAuthSchemes=null, connectionRequestTimeout=0, connectTimeout=0, socketTimeout=0, decompressionEnabled=true], http.auth.proxy-scope=state:UNCHALLENGED;, http.auth.credentials-provider={}, http.scheme-registry=org.apache.http.conn.scheme.SchemeRegistry@4861e7d6, http.cookie-spec=best-match, http.cookie-store=[], http.connection=org.apache.http.impl.conn.ManagedClientConnectionImpl@771b321c, http.auth.target-scope=state:UNCHALLENGED;, http.cookiespec-registry=org.apache.http.cookie.CookieSpecRegistry@1b78f4d2, http.target_host=https://127.0.0.1:51727, http.route={s}->https://127.0.0.1:51727, http.cookie-origin=[(secure)127.0.0.1:51727/solr/admin/collections], http.authscheme-registry=org.apache.http.auth.AuthSchemeRegistry@7acc1316} [junit4] > at __randomizedtesting.SeedInfo.seed([C37B717A18417443:D897F2DA4A5D5186]:0) [junit4] > at org.apache.solr.client.solrj.impl.HttpClientConfigurer$1.process(HttpClientConfigurer.java:92) [junit4] > at org.apache.http.protocol.ImmutableHttpProcessor.process(ImmutableHttpProcessor.java:132) [junit4] > at org.apache.http.protocol.HttpRequestExecutor.preProcess(HttpRequestExecutor.java:166) [junit4] > at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:485) [junit4] > at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:882) [junit4] > at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82) [junit4] > at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:107) [junit4] > at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:55) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.executeMethod(HttpSolrClient.java:465) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:234) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:226) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:376) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:328) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1086) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:856) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:799) [junit4] > at org.apache.solr.client.solrj.SolrClient.request(SolrClient.java:1220) [junit4] > at org.apache.solr.client.solrj.impl.TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp(TestCloudSolrClientConnections.java:57) [junit4] > at java.lang.Thread.run(Thread.java:745) and the branch_5x one ( http://jenkins.sarowe.net/job/Lucene-Solr-tests-5.x-Java8/892/ ): [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=FC64970124605306 -Dtests.slow=true -Dtests.locale=ar_LY -Dtests.timezone=Asia/Vientiane -Dtests.asserts=true -Dtests.file.encoding=UTF-8 [junit4] ERROR 1.84s J1 | TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp <<< [junit4] > Throwable #1: org.apache.solr.client.solrj.SolrServerException: java.lang.NullPointerException [junit4] > at __randomizedtesting.SeedInfo.seed([FC64970124605306:E78814A1767C76C3]:0) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:414) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:328) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1086) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:856) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:799) [junit4] > at org.apache.solr.client.solrj.SolrClient.request(SolrClient.java:1220) [junit4] > at org.apache.solr.client.solrj.impl.TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp(TestCloudSolrClientConnections.java:57) [junit4] > at java.lang.Thread.run(Thread.java:745) [junit4] > Caused by: java.lang.NullPointerException [junit4] > at org.apache.solr.client.solrj.impl.BasicHttpSolrClientTest$2.process(BasicHttpSolrClientTest.java:672) [junit4] > at org.apache.http.protocol.ImmutableHttpProcessor.process(ImmutableHttpProcessor.java:132) [junit4] > at org.apache.http.protocol.HttpRequestExecutor.preProcess(HttpRequestExecutor.java:166) [junit4] > at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:485) [junit4] > at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:882) [junit4] > at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82) [junit4] > at org.apache.solr.util.SolrHttpClient$SolrSystemDefaultHttpClient.execute(SolrHttpClient.java:54) [junit4] > at org.apache.solr.util.SolrHttpClient$SolrSystemDefaultHttpClient.execute(SolrHttpClient.java:45) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.executeMethod(HttpSolrClient.java:465) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:234) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:226) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:376) [junit4] > ... 45 more The above is from Jenkins on Linux, but when I run the repro line on OS X, I get a different exception: [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=FC64970124605306 -Dtests.slow=true -Dtests.locale=ar_LY -Dtests.timezone=Asia/Vientiane -Dtests.asserts=true -Dtests.file.encoding=UTF-8 [junit4] ERROR 5.08s | TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp <<< [junit4] > Throwable #1: org.apache.solr.common.SolrException: A SolrHttpContext object must be passed in as context. Context: {http.request=org.apache.http.impl.client.RequestWrapper@560c17c7, http.request-config=[expectContinueEnabled=false, proxy=null, localAddress=null, cookieSpec=null, redirectsEnabled=true, relativeRedirectsAllowed=true, maxRedirects=50, circularRedirectsAllowed=false, authenticationEnabled=true, targetPreferredAuthSchemes=null, proxyPreferredAuthSchemes=null, connectionRequestTimeout=0, connectTimeout=0, socketTimeout=0, decompressionEnabled=true], http.auth.proxy-scope=state:UNCHALLENGED;, http.auth.credentials-provider={}, http.scheme-registry=org.apache.http.conn.scheme.SchemeRegistry@13917b5, http.cookie-spec=best-match, http.cookie-store=[], http.connection=org.apache.http.impl.conn.ManagedClientConnectionImpl@492e6959, http.auth.target-scope=state:UNCHALLENGED;, http.cookiespec-registry=org.apache.http.cookie.CookieSpecRegistry@39eeeeba, http.target_host=https://127.0.0.1:52502, http.route={s}->https://127.0.0.1:52502, http.cookie-origin=[(secure)127.0.0.1:52502/solr/admin/collections], http.authscheme-registry=org.apache.http.auth.AuthSchemeRegistry@f6a646f} [junit4] > at __randomizedtesting.SeedInfo.seed([FC64970124605306:E78814A1767C76C3]:0) [junit4] > at org.apache.solr.client.solrj.impl.HttpClientConfigurer$1.process(HttpClientConfigurer.java:92) [junit4] > at org.apache.http.protocol.ImmutableHttpProcessor.process(ImmutableHttpProcessor.java:132) [junit4] > at org.apache.http.protocol.HttpRequestExecutor.preProcess(HttpRequestExecutor.java:166) [junit4] > at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:485) [junit4] > at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:882) [junit4] > at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82) [junit4] > at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:107) [junit4] > at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:55) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.executeMethod(HttpSolrClient.java:465) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:234) [junit4] > at org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:226) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:376) [junit4] > at org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:328) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1086) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:856) [junit4] > at org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:799) [junit4] > at org.apache.solr.client.solrj.SolrClient.request(SolrClient.java:1220) [junit4] > at org.apache.solr.client.solrj.impl.TestCloudSolrClientConnections.testCloudClientCanConnectAfterClusterComesUp(TestCloudSolrClientConnections.java:57) [junit4] > at java.lang.Thread.run(Thread.java:745)
          Hide
          Noble Paul added a comment -

          It was all passing when I tested it. Ishan can you please take a look?

          Show
          Noble Paul added a comment - It was all passing when I tested it. Ishan can you please take a look?
          Hide
          Ishan Chattopadhyaya added a comment -

          Thanks for pointing it out, Steve.
          Sure Noble. I'll take a look. Tests were passing for me at trunk. Will have
          a look at the 5x branch.

          Show
          Ishan Chattopadhyaya added a comment - Thanks for pointing it out, Steve. Sure Noble. I'll take a look. Tests were passing for me at trunk. Will have a look at the 5x branch.
          Hide
          Ishan Chattopadhyaya added a comment - - edited

          Attaching the patch to fix it.

          Show
          Ishan Chattopadhyaya added a comment - - edited Attaching the patch to fix it.
          Hide
          Anshum Gupta added a comment -

          Thanks Ishan. I just reviewed the old commit to understand the new approach.
          I'll commit this.

          Show
          Anshum Gupta added a comment - Thanks Ishan. I just reviewed the old commit to understand the new approach. I'll commit this.
          Hide
          Anshum Gupta added a comment -

          I got an NPE with the patch (log attached).

          Show
          Anshum Gupta added a comment - I got an NPE with the patch (log attached).
          Hide
          Ishan Chattopadhyaya added a comment -

          I've tried that seed and the test (after the last test fix patch) many times, and its passing for me always; i.e. I can't reproduce the NPE after applying the last patch.

          Anyway, looking into it a bit more.

          Show
          Ishan Chattopadhyaya added a comment - I've tried that seed and the test (after the last test fix patch) many times, and its passing for me always; i.e. I can't reproduce the NPE after applying the last patch. Anyway, looking into it a bit more.
          Hide
          Anshum Gupta added a comment -

          I think in LBHttpSolrClient.java (L326)

          HttpSolrClient client = makeSolrClient(serverStr);
          

          for some reason fails to instantiate and return the SolrClient. Could be because of systemDefaultHttpClientClass being not set for HttpClientUtil.createHttpClient to construct the client.

          Noble/Ishan: can you take a look? If there are a lot more test failures @ Jenkins, I suggest it'd be better to Ignore the test while actively working on a fix.

          Show
          Anshum Gupta added a comment - I think in LBHttpSolrClient.java (L326) HttpSolrClient client = makeSolrClient(serverStr); for some reason fails to instantiate and return the SolrClient. Could be because of systemDefaultHttpClientClass being not set for HttpClientUtil.createHttpClient to construct the client. Noble/Ishan: can you take a look? If there are a lot more test failures @ Jenkins, I suggest it'd be better to Ignore the test while actively working on a fix.
          Hide
          ASF subversion and git services added a comment -

          Commit 1693483 from Anshum Gupta in branch 'dev/trunk'
          [ https://svn.apache.org/r1693483 ]

          SOLR-6625: Set HttpClientImpl in SolrTestCaseJ4 for tests

          Show
          ASF subversion and git services added a comment - Commit 1693483 from Anshum Gupta in branch 'dev/trunk' [ https://svn.apache.org/r1693483 ] SOLR-6625 : Set HttpClientImpl in SolrTestCaseJ4 for tests
          Hide
          ASF subversion and git services added a comment -

          Commit 1693484 from Anshum Gupta in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1693484 ]

          SOLR-6625: Set HttpClientImpl in SolrTestCaseJ4 for tests(merge from trunk)

          Show
          ASF subversion and git services added a comment - Commit 1693484 from Anshum Gupta in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1693484 ] SOLR-6625 : Set HttpClientImpl in SolrTestCaseJ4 for tests(merge from trunk)
          Hide
          Anshum Gupta added a comment -

          I've committed the patch as that would be required anyways. It doesn't fix the NPE issue though.

          Show
          Anshum Gupta added a comment - I've committed the patch as that would be required anyways. It doesn't fix the NPE issue though.
          Hide
          Ishan Chattopadhyaya added a comment -

          Removing the interceptors that were added during the BasicHttpSolrClientTest. This was interfering with the other tests, viz. TestSolrCloudClientConnections.

          Show
          Ishan Chattopadhyaya added a comment - Removing the interceptors that were added during the BasicHttpSolrClientTest. This was interfering with the other tests, viz. TestSolrCloudClientConnections.
          Hide
          Anshum Gupta added a comment -

          That looks like it should have been interfering and would solve the problem. I'll commit this.

          Show
          Anshum Gupta added a comment - That looks like it should have been interfering and would solve the problem. I'll commit this.
          Hide
          ASF subversion and git services added a comment -

          Commit 1693497 from Anshum Gupta in branch 'dev/trunk'
          [ https://svn.apache.org/r1693497 ]

          SOLR-6625: Remove RequestInterceptor at the end of the test in BasicHttpSolrClientTest. It was interfering with other tests running the same JVM.

          Show
          ASF subversion and git services added a comment - Commit 1693497 from Anshum Gupta in branch 'dev/trunk' [ https://svn.apache.org/r1693497 ] SOLR-6625 : Remove RequestInterceptor at the end of the test in BasicHttpSolrClientTest. It was interfering with other tests running the same JVM.
          Hide
          ASF subversion and git services added a comment -

          Commit 1693498 from Anshum Gupta in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1693498 ]

          SOLR-6625: Remove RequestInterceptor at the end of the test in BasicHttpSolrClientTest. It was interfering with other tests running the same JVM.(merge from trunk)

          Show
          ASF subversion and git services added a comment - Commit 1693498 from Anshum Gupta in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1693498 ] SOLR-6625 : Remove RequestInterceptor at the end of the test in BasicHttpSolrClientTest. It was interfering with other tests running the same JVM.(merge from trunk)
          Hide
          Steve Rowe added a comment -

          I tried all of the seeds my Jenkins found today and none of them reproduce for me on OS X after Anshum's r1693498 commit.

          Here are the ones for trunk:

          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=F852018ABA4ECDFF -Dtests.slow=true -Dtests.locale=nl -Dtests.timezone=Asia/Gaza -Dtests.asserts=true -Dtests.file.encoding=UTF-8
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=36C935176479F04C -Dtests.slow=true -Dtests.locale=ar_YE -Dtests.timezone=Australia/Adelaide -Dtests.asserts=true -Dtests.file.encoding=UTF-8
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=D555CA223513ED3F -Dtests.slow=true -Dtests.locale=tr -Dtests.timezone=Antarctica/Syowa -Dtests.asserts=true -Dtests.file.encoding=US-ASCII
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=C37B717A18417443 -Dtests.slow=true -Dtests.locale=bg_BG -Dtests.timezone=SystemV/HST10 -Dtests.asserts=true -Dtests.file.encoding=UTF-8
          

          For branch_5x/Java8:

          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=FC64970124605306 -Dtests.slow=true -Dtests.locale=ar_LY -Dtests.timezone=Asia/Vientiane -Dtests.asserts=true -Dtests.file.encoding=UTF-8
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=4A4BDF1F10B61D9 -Dtests.slow=true -Dtests.locale=hu_HU -Dtests.timezone=Pacific/Fiji -Dtests.asserts=true -Dtests.file.encoding=US-ASCII
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=E4714ED097B4D0A7 -Dtests.slow=true -Dtests.locale= -Dtests.timezone=Etc/GMT-5 -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=E0C2EF677FA5EC03 -Dtests.slow=true -Dtests.locale=it_CH -Dtests.timezone=America/New_York -Dtests.asserts=true -Dtests.file.encoding=UTF-8
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=7F8305E670B947FE -Dtests.slow=true -Dtests.locale=fr_CH -Dtests.timezone=Europe/Skopje -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=30D63D1BAD373514 -Dtests.slow=true -Dtests.locale=it -Dtests.timezone=America/Iqaluit -Dtests.asserts=true -Dtests.file.encoding=US-ASCII
          

          And for branch_5x/Java7:

          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=98C4B653C6DE38C9 -Dtests.slow=true -Dtests.locale=it -Dtests.timezone=America/Havana -Dtests.asserts=true -Dtests.file.encoding=US-ASCII
          ant test  -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=27A47BFC57FF783D -Dtests.slow=true -Dtests.locale=en_SG -Dtests.timezone=Pacific/Chatham -Dtests.asserts=true -Dtests.file.encoding=UTF-8
          
          Show
          Steve Rowe added a comment - I tried all of the seeds my Jenkins found today and none of them reproduce for me on OS X after Anshum's r1693498 commit. Here are the ones for trunk: ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=F852018ABA4ECDFF -Dtests.slow=true -Dtests.locale=nl -Dtests.timezone=Asia/Gaza -Dtests.asserts=true -Dtests.file.encoding=UTF-8 ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=36C935176479F04C -Dtests.slow=true -Dtests.locale=ar_YE -Dtests.timezone=Australia/Adelaide -Dtests.asserts=true -Dtests.file.encoding=UTF-8 ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=D555CA223513ED3F -Dtests.slow=true -Dtests.locale=tr -Dtests.timezone=Antarctica/Syowa -Dtests.asserts=true -Dtests.file.encoding=US-ASCII ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=C37B717A18417443 -Dtests.slow=true -Dtests.locale=bg_BG -Dtests.timezone=SystemV/HST10 -Dtests.asserts=true -Dtests.file.encoding=UTF-8 For branch_5x/Java8: ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=FC64970124605306 -Dtests.slow=true -Dtests.locale=ar_LY -Dtests.timezone=Asia/Vientiane -Dtests.asserts=true -Dtests.file.encoding=UTF-8 ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=4A4BDF1F10B61D9 -Dtests.slow=true -Dtests.locale=hu_HU -Dtests.timezone=Pacific/Fiji -Dtests.asserts=true -Dtests.file.encoding=US-ASCII ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=E4714ED097B4D0A7 -Dtests.slow=true -Dtests.locale= -Dtests.timezone=Etc/GMT-5 -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1 ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=E0C2EF677FA5EC03 -Dtests.slow=true -Dtests.locale=it_CH -Dtests.timezone=America/New_York -Dtests.asserts=true -Dtests.file.encoding=UTF-8 ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=7F8305E670B947FE -Dtests.slow=true -Dtests.locale=fr_CH -Dtests.timezone=Europe/Skopje -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1 ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=30D63D1BAD373514 -Dtests.slow=true -Dtests.locale=it -Dtests.timezone=America/Iqaluit -Dtests.asserts=true -Dtests.file.encoding=US-ASCII And for branch_5x/Java7: ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=98C4B653C6DE38C9 -Dtests.slow=true -Dtests.locale=it -Dtests.timezone=America/Havana -Dtests.asserts=true -Dtests.file.encoding=US-ASCII ant test -Dtestcase=TestCloudSolrClientConnections -Dtests.method=testCloudClientCanConnectAfterClusterComesUp -Dtests.seed=27A47BFC57FF783D -Dtests.slow=true -Dtests.locale=en_SG -Dtests.timezone=Pacific/Chatham -Dtests.asserts=true -Dtests.file.encoding=UTF-8
          Hide
          Anshum Gupta added a comment -

          Thanks for confirming that Steve.

          Show
          Anshum Gupta added a comment - Thanks for confirming that Steve.
          Hide
          Yonik Seeley added a comment - - edited

          I haven't checked 5x yet, but I'm having issues on trunk:

          /opt/code/lusolr_clean2/solr$ bin/solr start
          Waiting to see Solr listening on port 8983 [|]  
          Started Solr server on port 8983 (pid=69801). Happy searching!
          
          /opt/code/lusolr_clean2/solr$ bin/solr create -c demo
          ERROR: create failed due to: A SolrHttpContext object must be passed in as context. Context: {http.request=org.apache.http.impl.client.RequestWrapper@4e1d422d, http.request-config=[expectContinueEnabled=false, proxy=null, localAddress=null, cookieSpec=null, redirectsEnabled=false, relativeRedirectsAllowed=true, maxRedirects=50, circularRedirectsAllowed=false, authenticationEnabled=true, targetPreferredAuthSchemes=null, proxyPreferredAuthSchemes=null, connectionRequestTimeout=0, connectTimeout=0, socketTimeout=0, decompressionEnabled=true], http.auth.proxy-scope=state:UNCHALLENGED;, http.auth.credentials-provider={}, http.scheme-registry=org.apache.http.conn.scheme.SchemeRegistry@66480dd7, http.cookie-spec=best-match, http.cookie-store=[], http.connection=org.apache.http.impl.conn.ManagedClientConnectionImpl@52a86356, http.auth.target-scope=state:UNCHALLENGED;, http.cookiespec-registry=org.apache.http.cookie.CookieSpecRegistry@5ce81285, http.target_host=http://localhost:8983, http.route={}->http://localhost:8983, http.cookie-origin=[localhost:8983/solr/admin/info/system], http.authscheme-registry=org.apache.http.auth.AuthSchemeRegistry@78c03f1f}
          

          edit: now confirmed that I'm having this issue on 5x as well

          Show
          Yonik Seeley added a comment - - edited I haven't checked 5x yet, but I'm having issues on trunk: /opt/code/lusolr_clean2/solr$ bin/solr start Waiting to see Solr listening on port 8983 [|] Started Solr server on port 8983 (pid=69801). Happy searching! /opt/code/lusolr_clean2/solr$ bin/solr create -c demo ERROR: create failed due to: A SolrHttpContext object must be passed in as context. Context: {http.request=org.apache.http.impl.client.RequestWrapper@4e1d422d, http.request-config=[expectContinueEnabled= false , proxy= null , localAddress= null , cookieSpec= null , redirectsEnabled= false , relativeRedirectsAllowed= true , maxRedirects=50, circularRedirectsAllowed= false , authenticationEnabled= true , targetPreferredAuthSchemes= null , proxyPreferredAuthSchemes= null , connectionRequestTimeout=0, connectTimeout=0, socketTimeout=0, decompressionEnabled= true ], http.auth.proxy-scope=state:UNCHALLENGED;, http.auth.credentials-provider={}, http.scheme-registry=org.apache.http.conn.scheme.SchemeRegistry@66480dd7, http.cookie-spec=best-match, http.cookie-store=[], http.connection=org.apache.http.impl.conn.ManagedClientConnectionImpl@52a86356, http.auth.target-scope=state:UNCHALLENGED;, http.cookiespec-registry=org.apache.http.cookie.CookieSpecRegistry@5ce81285, http.target_host=http: //localhost:8983, http.route={}->http://localhost:8983, http.cookie-origin=[localhost:8983/solr/admin/info/system], http.authscheme-registry=org.apache.http.auth.AuthSchemeRegistry@78c03f1f} edit: now confirmed that I'm having this issue on 5x as well
          Hide
          Ishan Chattopadhyaya added a comment - - edited

          Thanks for pointing out Yonik Seeley. Here's a patch that should fix this. With no tests catching it, I overlooked; apologies. I'll run the full tests and also manual tests with the bin/solr scripts after this patch and report back if I see any issues.

          The httpClient for SDF was created as a class level member, before the init() was called.

          Edit: My setup was stuck downloading restlet; now that I've copied it from another server, I realize the issue remains. Looking into it...

          Show
          Ishan Chattopadhyaya added a comment - - edited Thanks for pointing out Yonik Seeley . Here's a patch that should fix this. With no tests catching it, I overlooked; apologies. I'll run the full tests and also manual tests with the bin/solr scripts after this patch and report back if I see any issues. The httpClient for SDF was created as a class level member, before the init() was called. Edit: My setup was stuck downloading restlet; now that I've copied it from another server, I realize the issue remains. Looking into it...
          Hide
          Ishan Chattopadhyaya added a comment -

          This patch fixes the bin/solr (SolrCLI) issue.

          Show
          Ishan Chattopadhyaya added a comment - This patch fixes the bin/solr (SolrCLI) issue.
          Hide
          Shalin Shekhar Mangar added a comment -

          Here's a patch that should fix this. With no tests catching it, I overlooked; apologies.

          Can we add a test to catch this stuff?

          Show
          Shalin Shekhar Mangar added a comment - Here's a patch that should fix this. With no tests catching it, I overlooked; apologies. Can we add a test to catch this stuff?
          Hide
          Ishan Chattopadhyaya added a comment - - edited

          Close to 5.3, I am feeling unsure of what else is broken. Since SOLR-7692 can use the serverside request object (SolrRequestObject) directly from SolrRequestInfo, I want to revert the SolrHttpContext and the SolrHttpClient parts from this issue. We'll still continue to have the HttpRequestInterceptor support and the tests.

          Here's a patch to do the same (SOLR-6625-revert.patch). The tests are passing for me.

          If, at a later point, we need client side request object (SolrRequest), we can bring back these changes with more tests. Right now, that seems not necessary.

          Show
          Ishan Chattopadhyaya added a comment - - edited Close to 5.3, I am feeling unsure of what else is broken. Since SOLR-7692 can use the serverside request object (SolrRequestObject) directly from SolrRequestInfo, I want to revert the SolrHttpContext and the SolrHttpClient parts from this issue. We'll still continue to have the HttpRequestInterceptor support and the tests. Here's a patch to do the same ( SOLR-6625 -revert.patch). The tests are passing for me. If, at a later point, we need client side request object (SolrRequest), we can bring back these changes with more tests. Right now, that seems not necessary.
          Hide
          ASF subversion and git services added a comment -

          Commit 1693931 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1693931 ]

          SOLR-6625: Remove the HttpContext which provides our request

          Show
          ASF subversion and git services added a comment - Commit 1693931 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1693931 ] SOLR-6625 : Remove the HttpContext which provides our request
          Hide
          ASF subversion and git services added a comment -

          Commit 1693955 from Noble Paul in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1693955 ]

          SOLR-6625: Remove the HttpContext which provides our request

          Show
          ASF subversion and git services added a comment - Commit 1693955 from Noble Paul in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1693955 ] SOLR-6625 : Remove the HttpContext which provides our request
          Hide
          Shalin Shekhar Mangar added a comment -

          Is this feature complete? Can we close this issue?

          Show
          Shalin Shekhar Mangar added a comment - Is this feature complete? Can we close this issue?
          Hide
          Shalin Shekhar Mangar added a comment -

          Bulk close for 5.3.0 release

          Show
          Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release

            People

            • Assignee:
              Noble Paul
              Reporter:
              Gregory Chanan
            • Votes:
              1 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development