Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8332

factor HttpShardHandler[Factory]'s url shuffling out into a ReplicaListTransformer class

    Details

    • Type: Wish
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4, 7.0
    • Component/s: None
    • Labels:
      None

      Description

      Proposed patch against trunk to follow. No change in behaviour intended. This would be as a step towards SOLR-6730.

      1. SOLR-8332.patch
        19 kB
        Christine Poerschke
      2. SOLR-8332.patch
        13 kB
        Christine Poerschke
      3. SOLR-8332.patch
        13 kB
        Christine Poerschke
      4. SOLR-8332.patch
        13 kB
        Christine Poerschke

        Issue Links

          Activity

          Hide
          cpoerschke Christine Poerschke added a comment -

          Rebased patch against latest master/trunk.

          Show
          cpoerschke Christine Poerschke added a comment - Rebased patch against latest master/trunk.
          Hide
          cpoerschke Christine Poerschke added a comment -

          Attaching updated patch (package line moved to after license header and one unused import removed).

          Noble Paul - if you would have a few moments at some point to review the patch that would be much appreciated. Thank you.

          Show
          cpoerschke Christine Poerschke added a comment - Attaching updated patch (package line moved to after license header and one unused import removed). Noble Paul - if you would have a few moments at some point to review the patch that would be much appreciated. Thank you.
          Hide
          noble.paul Noble Paul added a comment -
          interface ReplicaListTransformer {
          
            public void transform(List<Replica> replicas);
          
            public void transformUrls(List<String> shardUrls);
          
          }
          
          

          As a I look at the functionality what it should do is to choose a few replicas from the available list of replicas. Sometimes, it would like to make a choice based on some input from users. It should not deal with actual urls. it's the responsibility of Solr to map the urls to actual replicas

          So, Lets have a much simpler interface

          interface ReplicaFilter {
              public List<Replica>  filter(List<Replica> allReplicas, SolrQueryRequest req);
          }
          
          Show
          noble.paul Noble Paul added a comment - interface ReplicaListTransformer { public void transform(List<Replica> replicas); public void transformUrls(List< String > shardUrls); } As a I look at the functionality what it should do is to choose a few replicas from the available list of replicas. Sometimes, it would like to make a choice based on some input from users. It should not deal with actual urls. it's the responsibility of Solr to map the urls to actual replicas So, Lets have a much simpler interface interface ReplicaFilter { public List<Replica> filter(List<Replica> allReplicas, SolrQueryRequest req); }
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user cpoerschke opened a pull request:

          https://github.com/apache/lucene-solr/pull/102

          SOLR-8332: factor HttpShardHandler[Factory]'s url shuffling out ...

          ... into a ReplicaListTransformer class

          (switching from patch attachment to pull request for clarity and convenience)

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/bloomberg/lucene-solr master-solr-8332

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/lucene-solr/pull/102.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #102


          commit 55d03abfc3c8c74d24815084c58924bcc270bd0b
          Author: Christine Poerschke <cpoerschke@apache.org>
          Date: 2016-10-25T11:14:17Z

          SOLR-8332: work-in-progress (applied Oct 18th patch to current master)

          commit 87f419d38146a06bc40106f344cdef69987415c0
          Author: Christine Poerschke <cpoerschke@apache.org>
          Date: 2016-10-25T11:47:54Z

          SOLR-8332: revisions incorporating review comments (see ticket log for details)


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user cpoerschke opened a pull request: https://github.com/apache/lucene-solr/pull/102 SOLR-8332 : factor HttpShardHandler [Factory] 's url shuffling out ... ... into a ReplicaListTransformer class (switching from patch attachment to pull request for clarity and convenience) You can merge this pull request into a Git repository by running: $ git pull https://github.com/bloomberg/lucene-solr master-solr-8332 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucene-solr/pull/102.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #102 commit 55d03abfc3c8c74d24815084c58924bcc270bd0b Author: Christine Poerschke <cpoerschke@apache.org> Date: 2016-10-25T11:14:17Z SOLR-8332 : work-in-progress (applied Oct 18th patch to current master) commit 87f419d38146a06bc40106f344cdef69987415c0 Author: Christine Poerschke <cpoerschke@apache.org> Date: 2016-10-25T11:47:54Z SOLR-8332 : revisions incorporating review comments (see ticket log for details)
          Hide
          cpoerschke Christine Poerschke added a comment -

          Thanks Noble for taking a look at the patch. I have made revisions and opened the above #102 pull request with them because in a patch it can sometimes be difficult to see the context of changes.

          ReplicaFilter.filter versus ReplicaListTransformer.transform names.

          • The transformer part of the interface name was quite deliberate to try and be generic about what the transformation might be e.g. it could be removal (i.e. filtering out) of elements or it could just be reordering (e.g. via shuffling) of elements.
          • revision made:
            • javadocs added to ReplicaListTransformer.transform giving filtering and shuffling as example transformations

          There being both transform and transformUrls methods.

          • Yes, i was uneasy about that as well. Usually Replica objects need to be filtered or reordered but HttpShardHandler can also operate on URLs passed in via the ShardsParam.SHARDS parameter.
          • revisions made:
            • transform(List<Replica>) and transformUrls(List<String>) combined into one transform(List<?> choices) method
            • javadocs added to mention about Replica vs. String choices
            • ToyMatchingReplicaListTransformer class (in tests) to demonstrate how List<?> choices can be used

          List methodName(List inputs) versus void methodName(List choices) signature.

          • no revisions made: making the parameter input-and-output seems unproblematic and saves having to allocate an output list to be returned

          transform(List,SolrQueryRequest) versus void transform(List) signature.

          • transform is called multiple times i.e. once for each shard but there is only one transformer object. The transformer's constructor may take a SolrQueryRequest argument (if needed) and that way request param deciphering happens only once per prepDistributed call.
          • revisions made:
            • Added ReplicaListTransformerTest class to demonstrate (with toy ReplicaListTransformer classes) how SolrQueryRequest params (or indeed SolrQueryRequest itself) could be passed to a ReplicaListTransformer upon construction.
          Show
          cpoerschke Christine Poerschke added a comment - Thanks Noble for taking a look at the patch. I have made revisions and opened the above #102 pull request with them because in a patch it can sometimes be difficult to see the context of changes. ReplicaFilter.filter versus ReplicaListTransformer.transform names. The transformer part of the interface name was quite deliberate to try and be generic about what the transformation might be e.g. it could be removal (i.e. filtering out) of elements or it could just be reordering (e.g. via shuffling) of elements. revision made: javadocs added to ReplicaListTransformer.transform giving filtering and shuffling as example transformations There being both transform and transformUrls methods. Yes, i was uneasy about that as well. Usually Replica objects need to be filtered or reordered but HttpShardHandler can also operate on URLs passed in via the ShardsParam.SHARDS parameter. revisions made: transform(List<Replica>) and transformUrls(List<String>) combined into one transform(List<?> choices) method javadocs added to mention about Replica vs. String choices ToyMatchingReplicaListTransformer class (in tests) to demonstrate how List<?> choices can be used List methodName(List inputs) versus void methodName(List choices) signature. no revisions made: making the parameter input-and-output seems unproblematic and saves having to allocate an output list to be returned transform(List,SolrQueryRequest) versus void transform(List) signature. transform is called multiple times i.e. once for each shard but there is only one transformer object. The transformer's constructor may take a SolrQueryRequest argument (if needed) and that way request param deciphering happens only once per prepDistributed call. revisions made: Added ReplicaListTransformerTest class to demonstrate (with toy ReplicaListTransformer classes) how SolrQueryRequest params (or indeed SolrQueryRequest itself) could be passed to a ReplicaListTransformer upon construction.
          Hide
          noble.paul Noble Paul added a comment -

          The transformer's constructor may take a SolrQueryRequest argument (if needed) and that way request param deciphering happens only once per prepDistributed call.

          Creating a new instance of ReplicaListTransformer for each request is wasteful. That is why added the request object as a parameter to each call. This way you can manage with a singleton

          Show
          noble.paul Noble Paul added a comment - The transformer's constructor may take a SolrQueryRequest argument (if needed) and that way request param deciphering happens only once per prepDistributed call. Creating a new instance of ReplicaListTransformer for each request is wasteful. That is why added the request object as a parameter to each call. This way you can manage with a singleton
          Hide
          cpoerschke Christine Poerschke added a comment -

          Creating a new instance of ReplicaListTransformer for each request is wasteful. That is why added the request object as a parameter to each call. This way you can manage with a singleton

          #102 supports use of a singleton where possible e.g. HttpShardHandlerFactory's shuffling transformer:

          private final ReplicaListTransformer shufflingReplicaListTransformer = new ShufflingReplicaListTransformer(r);
          ...
          ReplicaListTransformer getReplicaListTransformer(final SolrQueryRequest req)
          {
            return shufflingReplicaListTransformer;
          }
          

          #102 also supports use of per-request transformers where required (e.g. ReplicaListTransformerTest.java#L96-L111) so that request param deciphering happens only once per prepDistributed call rather than repeatedly in each transform call.

          Show
          cpoerschke Christine Poerschke added a comment - Creating a new instance of ReplicaListTransformer for each request is wasteful. That is why added the request object as a parameter to each call. This way you can manage with a singleton #102 supports use of a singleton where possible e.g. HttpShardHandlerFactory 's shuffling transformer: private final ReplicaListTransformer shufflingReplicaListTransformer = new ShufflingReplicaListTransformer(r); ... ReplicaListTransformer getReplicaListTransformer( final SolrQueryRequest req) { return shufflingReplicaListTransformer; } #102 also supports use of per-request transformers where required (e.g. ReplicaListTransformerTest.java#L96-L111 ) so that request param deciphering happens only once per prepDistributed call rather than repeatedly in each transform call.
          Hide
          cpoerschke Christine Poerschke added a comment -

          If there are no further comments here then I will proceed with committing this change sometime middle of next week.

          Show
          cpoerschke Christine Poerschke added a comment - If there are no further comments here then I will proceed with committing this change sometime middle of next week.
          Hide
          noble.paul Noble Paul added a comment -

          I'm not convinced about

            public void transformUrls(List<String> shardUrls);
          

          it shouldn't be required

          Show
          noble.paul Noble Paul added a comment - I'm not convinced about public void transformUrls(List< String > shardUrls); it shouldn't be required
          Hide
          cpoerschke Christine Poerschke added a comment -

          #102 pull request replaces the patch attached to this ticket and amongst other things it combined transform(List<Replica>) and transformUrls(List<String>) into one transform(List<?> choices) method. For other revisions also included in #102 please see comments above.

          Show
          cpoerschke Christine Poerschke added a comment - #102 pull request replaces the patch attached to this ticket and amongst other things it combined transform(List<Replica>) and transformUrls(List<String>) into one transform(List<?> choices) method. For other revisions also included in #102 please see comments above.
          Hide
          noble.paul Noble Paul added a comment -

          +1

          Show
          noble.paul Noble Paul added a comment - +1
          Show
          cpoerschke Christine Poerschke added a comment - attaching https://patch-diff.githubusercontent.com/raw/apache/lucene-solr/pull/102.diff as SOLR-8332 .patch
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6c25adb11963c9859bb1ccd15e7afdb171c85f48 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6c25adb ]

          SOLR-8332: Factor HttpShardHandler[Factory]'s url shuffling out into a ReplicaListTransformer class.
          (Christine Poerschke, Noble Paul)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6c25adb11963c9859bb1ccd15e7afdb171c85f48 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6c25adb ] SOLR-8332 : Factor HttpShardHandler [Factory] 's url shuffling out into a ReplicaListTransformer class. (Christine Poerschke, Noble Paul)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit aa6a678a80575d8f3876b37688b60c1787732294 in lucene-solr's branch refs/heads/branch_6x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=aa6a678 ]

          SOLR-8332: Factor HttpShardHandler[Factory]'s url shuffling out into a ReplicaListTransformer class.
          (Christine Poerschke, Noble Paul)

          Show
          jira-bot ASF subversion and git services added a comment - Commit aa6a678a80575d8f3876b37688b60c1787732294 in lucene-solr's branch refs/heads/branch_6x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=aa6a678 ] SOLR-8332 : Factor HttpShardHandler [Factory] 's url shuffling out into a ReplicaListTransformer class. (Christine Poerschke, Noble Paul)
          Hide
          cpoerschke Christine Poerschke added a comment -

          Thanks Noble Paul for your input.

          Show
          cpoerschke Christine Poerschke added a comment - Thanks Noble Paul for your input.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user cpoerschke closed the pull request at:

          https://github.com/apache/lucene-solr/pull/102

          Show
          githubbot ASF GitHub Bot added a comment - Github user cpoerschke closed the pull request at: https://github.com/apache/lucene-solr/pull/102
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user cpoerschke commented on the issue:

          https://github.com/apache/lucene-solr/pull/102

          Closing out after committing.

          Show
          githubbot ASF GitHub Bot added a comment - Github user cpoerschke commented on the issue: https://github.com/apache/lucene-solr/pull/102 Closing out after committing.

            People

            • Assignee:
              cpoerschke Christine Poerschke
              Reporter:
              cpoerschke Christine Poerschke
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development