Rebased patch against latest master/trunk.

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.

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);
}

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)

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.

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

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.

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

I'm not convinced about

  public void transformUrls(List<String> shardUrls);

it shouldn't be required

#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.

+1

attaching https://patch-diff.githubusercontent.com/raw/apache/lucene-solr/pull/102.diff as SOLR-8332.patch

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)

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)

Thanks Noble Paul for your input.

Github user cpoerschke closed the pull request at:

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

Github user cpoerschke commented on the issue:

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

Closing out after committing.