Solr
  1. Solr
  2. SOLR-4448

Allow the solr internal load balancer to be more easily pluggable.

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.4, 6.0
    • Component/s: SolrCloud
    • Labels:
      None

      Description

      Widen some access level modifiers to allow the load balancer to be extended and plugged into an HttpShardHandler instance using an extended HttpShardHandlerFactory.

      1. SOLR-4448.patch
        3 kB
        philip hoy
      2. SOLR-4448.patch
        5 kB
        philip hoy

        Issue Links

          Activity

          Hide
          philip hoy added a comment - - edited

          Added a git generated patch.

          Show
          philip hoy added a comment - - edited Added a git generated patch.
          Hide
          Mark Miller added a comment -

          how is the additional filter you can supply to the solr jetty runner related?

          Show
          Mark Miller added a comment - how is the additional filter you can supply to the solr jetty runner related?
          Hide
          philip hoy added a comment - - edited

          My apologies, the jetty runner edit should have been part of the linked jira and was added so that i could simulate slow responses in tests. Attached is the amended patch removing that edit.

          Show
          philip hoy added a comment - - edited My apologies, the jetty runner edit should have been part of the linked jira and was added so that i could simulate slow responses in tests. Attached is the amended patch removing that edit.
          Hide
          philip hoy added a comment -

          I don't know if anyone has had a chance to take a look at this patch. It will make it easier to plug in load balancers with different characteristics.

          Show
          philip hoy added a comment - I don't know if anyone has had a chance to take a look at this patch. It will make it easier to plug in load balancers with different characteristics.
          Hide
          Ryan Ernst added a comment -

          This looks like it at least makes the load balancer pluggable if you are already plugging in your own shard handler factory. I think making it pluggable outside of that context is better for the future, but should be a separate jira.

          One thing I don't currently like (even before your patch) is the fact that the load balancer lives in solrj. It is just an odd place, since this is a core part of how distributed search works.

          I think a better design, even if the existing default implementation stays in solrj, is to have an abstract class in solr core, which is all shard handler factory should know about. This should be really simple (one or two methods for configuration and sending requests). Then the LB in solrj can extend from it, and other load balancer implementations don't need to extend from a class in solrj.

          Regardless of those comments:
          +1 to the patch as is.

          Show
          Ryan Ernst added a comment - This looks like it at least makes the load balancer pluggable if you are already plugging in your own shard handler factory. I think making it pluggable outside of that context is better for the future, but should be a separate jira. One thing I don't currently like (even before your patch) is the fact that the load balancer lives in solrj. It is just an odd place, since this is a core part of how distributed search works. I think a better design, even if the existing default implementation stays in solrj, is to have an abstract class in solr core, which is all shard handler factory should know about. This should be really simple (one or two methods for configuration and sending requests). Then the LB in solrj can extend from it, and other load balancer implementations don't need to extend from a class in solrj. Regardless of those comments: +1 to the patch as is.
          Hide
          Shawn Heisey added a comment - - edited

          One thing I don't currently like (even before your patch) is the fact that the load balancer lives in solrj. It is just an odd place, since this is a core part of how distributed search works.

          There are a number of core pieces of Solr functionality that live in solrj. It's the only way that the solrj client can be kept relatively small.

          One particularly relevant example of core Solr functionality that is found in solrj is NamedList. As an experiment, I went into my IDE and added a character to the class name in NamedList.java from trunk, then saved the change. Most of the org.apache.solr packages suddenly had an error marker.

          The client code requires access to the load balancing capability, so if you move load balancing to the Solr core, solrj will have a new and very large dependency.

          Show
          Shawn Heisey added a comment - - edited One thing I don't currently like (even before your patch) is the fact that the load balancer lives in solrj. It is just an odd place, since this is a core part of how distributed search works. There are a number of core pieces of Solr functionality that live in solrj. It's the only way that the solrj client can be kept relatively small. One particularly relevant example of core Solr functionality that is found in solrj is NamedList. As an experiment, I went into my IDE and added a character to the class name in NamedList.java from trunk, then saved the change. Most of the org.apache.solr packages suddenly had an error marker. The client code requires access to the load balancing capability, so if you move load balancing to the Solr core, solrj will have a new and very large dependency.
          Hide
          Shawn Heisey added a comment -

          Further thoughts about the solrj location:

          The load balancing capability is inherently a client feature. When Solr uses it, it is acting as a client. There's no reason for SolrJ and the Solr core to have different implementations, and there is strong precedence for putting common Solr/SolrJ features into SolrJ. LBHttpSolrServer has been around since Solr 1.4, but prior to 4.0, nothing in Solr depended on it.

          Show
          Shawn Heisey added a comment - Further thoughts about the solrj location: The load balancing capability is inherently a client feature. When Solr uses it, it is acting as a client. There's no reason for SolrJ and the Solr core to have different implementations, and there is strong precedence for putting common Solr/SolrJ features into SolrJ. LBHttpSolrServer has been around since Solr 1.4, but prior to 4.0, nothing in Solr depended on it.
          Hide
          Ryan Ernst added a comment -

          The load balancing capability is inherently a client feature.

          Load balancing is used by distributed search. It happens to also be used for uploading documents, which is a client feature. Clients shouldn't be using this for sending distributed search requests. Solr does that.

          There's no reason for SolrJ and the Solr core to have different implementations, and there is strong precedence for putting common Solr/SolrJ features into SolrJ.

          I don't know much about SolrJ. What I really care about is not having to inherit from the default implementation, but instead having an abstraction.

          That said, I still think this patch is fine as is.

          Show
          Ryan Ernst added a comment - The load balancing capability is inherently a client feature. Load balancing is used by distributed search. It happens to also be used for uploading documents, which is a client feature. Clients shouldn't be using this for sending distributed search requests. Solr does that. There's no reason for SolrJ and the Solr core to have different implementations, and there is strong precedence for putting common Solr/SolrJ features into SolrJ. I don't know much about SolrJ. What I really care about is not having to inherit from the default implementation, but instead having an abstraction. That said, I still think this patch is fine as is.
          Hide
          Shawn Heisey added a comment -

          Ryan Ernst first let me say that I am having this discussion because what you're saying goes against my limited understanding, and by stating what I think and listening to your response, I might learn something. You probably already know the things that I am saying. I might even find that I misunderstood what you were saying and that I agree with you.

          Load balancing is used by distributed search. It happens to also be used for uploading documents, which is a client feature. Clients shouldn't be using this for sending distributed search requests. Solr does that.

          I've just done a non-detailed review of CloudSolrServer. It uses a new LBHttpSolrServer object with a customized URL list for every request. Queries get sent to all replicas, updates only get sent to leaders. A TODO says that currently there is no support in the object for sending updates to the correct leader based on a hashing algorithm.

          Outside of SolrCloud, the LB object makes sense for clients in master-slave replication environments, but only on the query side. Updates have to be directed to the master only. A separate load balancer does give you more flexibility, but not everyone wants to invest the time (or possibly money) required.

          If the client on the server side and the client on the client side need identical functionality, then the existing situation makes sense – one implementation in the org.apache.solr.client.solrj namespace. If we think they'll ever diverge, even a little bit, then having an abstract class in the org.apache.solr.common namespace makes sense, although it should still be in the solrj source tree.

          Show
          Shawn Heisey added a comment - Ryan Ernst first let me say that I am having this discussion because what you're saying goes against my limited understanding, and by stating what I think and listening to your response, I might learn something. You probably already know the things that I am saying. I might even find that I misunderstood what you were saying and that I agree with you. Load balancing is used by distributed search. It happens to also be used for uploading documents, which is a client feature. Clients shouldn't be using this for sending distributed search requests. Solr does that. I've just done a non-detailed review of CloudSolrServer. It uses a new LBHttpSolrServer object with a customized URL list for every request. Queries get sent to all replicas, updates only get sent to leaders. A TODO says that currently there is no support in the object for sending updates to the correct leader based on a hashing algorithm. Outside of SolrCloud, the LB object makes sense for clients in master-slave replication environments, but only on the query side. Updates have to be directed to the master only. A separate load balancer does give you more flexibility, but not everyone wants to invest the time (or possibly money) required. If the client on the server side and the client on the client side need identical functionality, then the existing situation makes sense – one implementation in the org.apache.solr.client.solrj namespace. If we think they'll ever diverge, even a little bit, then having an abstract class in the org.apache.solr.common namespace makes sense, although it should still be in the solrj source tree.
          Hide
          Shawn Heisey added a comment -

          My review of CloudSolrServer obviously wasn't deep enough. I thought it was making a new LB object for every request, which seemed very inefficient. Turns out it was making a new LBHttpSolrServer.Req object. The Req class includes a URL list.

          Show
          Shawn Heisey added a comment - My review of CloudSolrServer obviously wasn't deep enough. I thought it was making a new LB object for every request, which seemed very inefficient. Turns out it was making a new LBHttpSolrServer.Req object. The Req class includes a URL list.
          Hide
          Ryan Ernst added a comment -

          My apologies. I do not use SolrJ and was making some bad assumptions. I see now that a client would use this to round-robin between all the hosts in a cluster for a top level requests, and then solr would also use a different LB (running in solr instead of the client) for distributing requests to slices.

          As I've said here, I'm fine with this staying in SolrJ. I only hope (in the future, not asking for it here) to see a better abstraction for the load balancer.

          Show
          Ryan Ernst added a comment - My apologies. I do not use SolrJ and was making some bad assumptions. I see now that a client would use this to round-robin between all the hosts in a cluster for a top level requests, and then solr would also use a different LB (running in solr instead of the client) for distributing requests to slices. As I've said here, I'm fine with this staying in SolrJ. I only hope (in the future, not asking for it here) to see a better abstraction for the load balancer.
          Hide
          philip hoy added a comment -

          I think a problem with the current LBHttpSolrServer is that it has too many responsibilities. It should perhaps be broken into two classes one in solrj to be used by clients which is designed to balance across a known set of solr servers defined at construction time and one in solr proper that deals with the distributed requests which are dependant on the query itself. Looking at the code very little is actually shared between these two use cases so the need for a base implementation and concerns about the appropriate location are not so much of an issue. If so desired the zombie server handling, the only shared code, could be pulled into an base class or another class entirely.

          Show
          philip hoy added a comment - I think a problem with the current LBHttpSolrServer is that it has too many responsibilities. It should perhaps be broken into two classes one in solrj to be used by clients which is designed to balance across a known set of solr servers defined at construction time and one in solr proper that deals with the distributed requests which are dependant on the query itself. Looking at the code very little is actually shared between these two use cases so the need for a base implementation and concerns about the appropriate location are not so much of an issue. If so desired the zombie server handling, the only shared code, could be pulled into an base class or another class entirely.
          Hide
          philip hoy added a comment -

          How do I best progress this patch, I am happy to make any changes deemed necessary.

          Show
          philip hoy added a comment - How do I best progress this patch, I am happy to make any changes deemed necessary.
          Hide
          Robert Muir added a comment -

          The discussion here got a little off-topic from the current patch.

          Philip's patch just exposes a few things as protected and so on: does anyone have any technical objections to it?

          If not, I'll move forward with it Philip. it would be good to maybe make followup JIRA issues for additional refactoring/discussion.

          Show
          Robert Muir added a comment - The discussion here got a little off-topic from the current patch. Philip's patch just exposes a few things as protected and so on: does anyone have any technical objections to it? If not, I'll move forward with it Philip. it would be good to maybe make followup JIRA issues for additional refactoring/discussion.
          Hide
          Mark Miller added a comment -

          +1 - I see some good ideas for a further issue, but let's commit this patch and spin off a new issue.

          Show
          Mark Miller added a comment - +1 - I see some good ideas for a further issue, but let's commit this patch and spin off a new issue.
          Hide
          Robert Muir added a comment -

          I committed this: Thanks Philip!

          Please feel free to open issues for some of the additional refactoring ideas here!

          Show
          Robert Muir added a comment - I committed this: Thanks Philip! Please feel free to open issues for some of the additional refactoring ideas here!
          Hide
          philip hoy added a comment -

          I have added a jira to cover a potential refactoring to split out of few of the responsibilities currently carried out by the LBHttpSolrServer class.

          Show
          philip hoy added a comment - I have added a jira to cover a potential refactoring to split out of few of the responsibilities currently carried out by the LBHttpSolrServer class.
          Hide
          Steve Rowe added a comment -

          Bulk close resolved 4.4 issues

          Show
          Steve Rowe added a comment - Bulk close resolved 4.4 issues

            People

            • Assignee:
              Unassigned
              Reporter:
              philip hoy
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development