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

Implement a builder pattern for constructing a Solrj client

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 6.0
    • Fix Version/s: 6.1, master (7.0)
    • Component/s: SolrJ
    • Labels:
      None

      Description

      Currently Solrj clients (e.g. CloudSolrClient) supports multiple constructors as follows,
      public CloudSolrClient(String zkHost)
      public CloudSolrClient(String zkHost, HttpClient httpClient)
      public CloudSolrClient(Collection<String> zkHosts, String chroot)
      public CloudSolrClient(Collection<String> zkHosts, String chroot, HttpClient httpClient)
      public CloudSolrClient(String zkHost, boolean updatesToLeaders)
      public CloudSolrClient(String zkHost, boolean updatesToLeaders, HttpClient httpClient)

      It is kind of problematic while introducing an additional parameters (since we need to introduce additional constructors). Instead it will be helpful to provide SolrClient Builder which can provide either default values or support overriding specific parameter.

      1. SOLR-8097.patch
        221 kB
        Anshum Gupta
      2. SOLR-8097.patch
        222 kB
        Jason Gerlowski
      3. SOLR-8097.patch
        221 kB
        Jason Gerlowski
      4. SOLR-8097.patch
        152 kB
        Jason Gerlowski
      5. SOLR-8097.patch
        152 kB
        Jason Gerlowski
      6. SOLR-8097.patch
        152 kB
        Jason Gerlowski
      7. SOLR-8097.patch
        161 kB
        Jason Gerlowski
      8. SOLR-8097.patch
        162 kB
        Jason Gerlowski
      9. SOLR-8097.patch
        162 kB
        Jason Gerlowski
      10. SOLR-8097.patch
        212 kB
        Jason Gerlowski
      11. SOLR-8097.patch
        86 kB
        Jason Gerlowski
      12. SOLR-8097.patch
        79 kB
        Jason Gerlowski
      13. SOLR-8097.patch
        29 kB
        Jason Gerlowski
      14. SOLR-8097.patch
        15 kB
        Jason Gerlowski

        Issue Links

          Activity

          Hide
          gerlowskija Jason Gerlowski added a comment -

          I'm gonna take a first pass at this.

          Is the goal/hope for this JIRA that the builder will allow us to deprecate/remove the CloudSearchClient ctors? (If so, should I remove them in this patch?) Or is the hope just that this will prevent additional ctors from being needed in the future?

          Show
          gerlowskija Jason Gerlowski added a comment - I'm gonna take a first pass at this. Is the goal/hope for this JIRA that the builder will allow us to deprecate/remove the CloudSearchClient ctors? (If so, should I remove them in this patch?) Or is the hope just that this will prevent additional ctors from being needed in the future?
          Hide
          gerlowskija Jason Gerlowski added a comment -

          My comment above implies some questions about the measures we try to take in ensuring backward-compatibility in SolrJ code. I'm still new to the Solr community, so I'm not quite sure how things are typically done here.

          My assumption up to this point is that core SolrJ APIs (i.e. Java ctor/method signatures) can only be changed by a major release. (This appears to have been confirmed by most of the discussion on the recent "Breaking Java back-compat in Solr" mailing list thread, by my reading of it at least.)

          I'm also a little unclear on what "deprecation" means within the Solr community. Can a deprecation warning appear on a non-major release? How long does a deprecation warning typically stay around before the API can be removed altogether? Anyone have a pointer to an article/wiki where I can read up on this?

          That said, this isn't relevant/necessary to start working on a patch, as long as I don't touch any of the existing ctors. So that's how I'll go forward with things until I hear feedback otherwise. I'll start putting together a purely-additive patch off of trunk later tonight or tomorrow.

          Show
          gerlowskija Jason Gerlowski added a comment - My comment above implies some questions about the measures we try to take in ensuring backward-compatibility in SolrJ code. I'm still new to the Solr community, so I'm not quite sure how things are typically done here. My assumption up to this point is that core SolrJ APIs (i.e. Java ctor/method signatures) can only be changed by a major release. (This appears to have been confirmed by most of the discussion on the recent "Breaking Java back-compat in Solr" mailing list thread, by my reading of it at least.) I'm also a little unclear on what "deprecation" means within the Solr community. Can a deprecation warning appear on a non-major release? How long does a deprecation warning typically stay around before the API can be removed altogether? Anyone have a pointer to an article/wiki where I can read up on this? That said, this isn't relevant/necessary to start working on a patch, as long as I don't touch any of the existing ctors. So that's how I'll go forward with things until I hear feedback otherwise. I'll start putting together a purely-additive patch off of trunk later tonight or tomorrow.
          Hide
          elyograg Shawn Heisey added a comment - - edited

          Code written and compiled with a particular version of SolrJ should work properly if the SolrJ jar is replaced with a newer minor version. This compatibility is not expected if the jar is replaced with a newer major version.

          The general goal is that the Solr Java API will not change in minor versions, but this is not guaranteed. The later discussion related to back-compat talks about some new annotations designed to make it more clear which APIs will be static and which are expected to change. The SolrJ API will always be guarded more closely against change than the rest of Solr, because it is the API that is used most commonly in user code.

          When public APIs do change in a minor version, the old API is generally marked as deprecated, so that existing code will still compile and function properly. The deprecated API is completely removed from the next major version (trunk).

          Show
          elyograg Shawn Heisey added a comment - - edited Code written and compiled with a particular version of SolrJ should work properly if the SolrJ jar is replaced with a newer minor version. This compatibility is not expected if the jar is replaced with a newer major version. The general goal is that the Solr Java API will not change in minor versions, but this is not guaranteed. The later discussion related to back-compat talks about some new annotations designed to make it more clear which APIs will be static and which are expected to change. The SolrJ API will always be guarded more closely against change than the rest of Solr, because it is the API that is used most commonly in user code. When public APIs do change in a minor version, the old API is generally marked as deprecated, so that existing code will still compile and function properly. The deprecated API is completely removed from the next major version (trunk).
          Hide
          gerlowskija Jason Gerlowski added a comment -

          I've attached a first pass at the creation of a CloudSolrClientBuilder class. The attached patch contains the builder class, an added CloudSolrClient ctor that takes all parameters, and some basic tests for the builder and new ctor.

          Some notes on the patch:

          • patch is off of trunk/master. I assume we'd also want it in 5.x, but one thing at a time...
          • created two small helper methods in CloudSolrClient. Maybe I shouldn't've done that, but I found myself copy-pasting the code, so it seemed like the right thing to do at the time. Happy to change if necessary.
          • I didn't change any other code in the project to actually use the new builder. I'm happy to do this, just wasn't sure it was the right thing and wanted to wait on feedback from others.
          • I didn't touch (delete, re-scope, deprecate, etc.) any of the existing ctors. I assume an eventual version of this patch on 5.x should deprecate the existing ctors. Should I also have the ctors be deprecated in the trunk/master version? Or should I do something more aggressive, like deleting them altogether? Happy to do whatever; just wasn't entirely sure what the right move was (even after Shawn Heisey addressed my earlier, less directed questions above...thanks btw).

          All-in-all, the patch still needs a bit of work, but should be a solid start. Looking forward to hearing what people think/want-changed!

          Show
          gerlowskija Jason Gerlowski added a comment - I've attached a first pass at the creation of a CloudSolrClientBuilder class. The attached patch contains the builder class, an added CloudSolrClient ctor that takes all parameters, and some basic tests for the builder and new ctor. Some notes on the patch: patch is off of trunk/master. I assume we'd also want it in 5.x, but one thing at a time... created two small helper methods in CloudSolrClient . Maybe I shouldn't've done that, but I found myself copy-pasting the code, so it seemed like the right thing to do at the time. Happy to change if necessary. I didn't change any other code in the project to actually use the new builder. I'm happy to do this, just wasn't sure it was the right thing and wanted to wait on feedback from others. I didn't touch (delete, re-scope, deprecate, etc.) any of the existing ctors. I assume an eventual version of this patch on 5.x should deprecate the existing ctors. Should I also have the ctors be deprecated in the trunk/master version? Or should I do something more aggressive, like deleting them altogether? Happy to do whatever; just wasn't entirely sure what the right move was (even after Shawn Heisey addressed my earlier, less directed questions above...thanks btw). All-in-all, the patch still needs a bit of work, but should be a solid start. Looking forward to hearing what people think/want-changed!
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Hi all,

          Just wanted to give this issue a little 'bump'. If there's not much interest in this issue, that's fine with me. Not trying to push this over other important things people are working on. Just wanted to make sure it wasn't missed on accident.

          So if you're interested in seeing this go forward and have time to take a look, please review the attached patch, and let me know what you'd like tweaked/improved in the next iteration.

          Show
          gerlowskija Jason Gerlowski added a comment - Hi all, Just wanted to give this issue a little 'bump'. If there's not much interest in this issue, that's fine with me. Not trying to push this over other important things people are working on. Just wanted to make sure it wasn't missed on accident. So if you're interested in seeing this go forward and have time to take a look, please review the attached patch, and let me know what you'd like tweaked/improved in the next iteration.
          Hide
          elyograg Shawn Heisey added a comment -

          I like the idea of going with a builder pattern.

          I would probably start with initial work on HttpSolrClient, work out the kinks there, then do the Concurrent and LB objects, and tackle Cloud last. I would do it this way so that each class can utilize methods from the "lower" class. I don't know if it makes sense to create a "SolrClientBuilder" interface or abstract class, but that can be investigated.

          If the embedded server needs similar treatment, then we should see whether it makes any sense to incorporate changes into the parent class – SolrClient. A builder pattern for the embedded server probably does make sense, but I haven't looked deeper to verify.

          To delay the impact to client programs as long as possible, we should skip branch_5x, and just make the change in the master branch. The old constructors can be removed in master once branch_6x is created.

          Show
          elyograg Shawn Heisey added a comment - I like the idea of going with a builder pattern. I would probably start with initial work on HttpSolrClient, work out the kinks there, then do the Concurrent and LB objects, and tackle Cloud last. I would do it this way so that each class can utilize methods from the "lower" class. I don't know if it makes sense to create a "SolrClientBuilder" interface or abstract class, but that can be investigated. If the embedded server needs similar treatment, then we should see whether it makes any sense to incorporate changes into the parent class – SolrClient. A builder pattern for the embedded server probably does make sense, but I haven't looked deeper to verify. To delay the impact to client programs as long as possible, we should skip branch_5x, and just make the change in the master branch. The old constructors can be removed in master once branch_6x is created.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Hey Shawn, thanks for the review.

          I hadn't realized this JIRA was intended to cover each of the SolrClient implementations. The list of CloudSearchClient ctors in the description narrowed my focus; on a re-read it's clear that was just an example. With that in mind all your feedback makes sense to me.

          I'll expand this patch out to the other SolrClients then, in a way that leverages inheritance where possible. Since I was focused more narrowly before, I'm also not sure what makes the most sense with regards to also covering SolrServer. I'll give your suggestions a shot and check back in with a patch or an update.

          Show
          gerlowskija Jason Gerlowski added a comment - Hey Shawn, thanks for the review. I hadn't realized this JIRA was intended to cover each of the SolrClient implementations. The list of CloudSearchClient ctors in the description narrowed my focus; on a re-read it's clear that was just an example. With that in mind all your feedback makes sense to me. I'll expand this patch out to the other SolrClients then, in a way that leverages inheritance where possible. Since I was focused more narrowly before, I'm also not sure what makes the most sense with regards to also covering SolrServer. I'll give your suggestions a shot and check back in with a patch or an update.
          Hide
          elyograg Shawn Heisey added a comment -

          In master (6.0), most of the *Server variants should be removed, as they were deprecated in 5.0. EmbeddedSolrServer is the exception there. I haven't looked at the code in master yet to verify whether those classes are gone.

          Show
          elyograg Shawn Heisey added a comment - In master (6.0), most of the *Server variants should be removed, as they were deprecated in 5.0. EmbeddedSolrServer is the exception there. I haven't looked at the code in master yet to verify whether those classes are gone.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Looks like the only remaining *Server variants are EmbeddedSolrServer (appears to only be used in tests, and the map-reduce contrib module), and EmbeddedTestSolrServer (lives in the morphline-core contrib module). My guess is that it's probably not worth writing a builder for these, since their use is restricted, and they don't really suffer from the too-many-ctor-args problem that the other SolrClients seem to.

          Show
          gerlowskija Jason Gerlowski added a comment - Looks like the only remaining *Server variants are EmbeddedSolrServer (appears to only be used in tests, and the map-reduce contrib module), and EmbeddedTestSolrServer (lives in the morphline-core contrib module). My guess is that it's probably not worth writing a builder for these, since their use is restricted, and they don't really suffer from the too-many-ctor-args problem that the other SolrClients seem to.
          Hide
          elyograg Shawn Heisey added a comment -

          We get semi-regular questions about EmbeddedSolrServer on the mailing lists and sometimes in the IRC channel. Users ARE using it.

          The reason I thought it could maybe use a builder pattern is that the way to create an instance is kind of arcane and non-obvious ... but I suppose that could be called a feature, because it might discourage people from using it. The embedded server is not recommended for general use, but it does make sense for certain highly specialized use cases.

          Show
          elyograg Shawn Heisey added a comment - We get semi-regular questions about EmbeddedSolrServer on the mailing lists and sometimes in the IRC channel. Users ARE using it. The reason I thought it could maybe use a builder pattern is that the way to create an instance is kind of arcane and non-obvious ... but I suppose that could be called a feature, because it might discourage people from using it. The embedded server is not recommended for general use, but it does make sense for certain highly specialized use cases.
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks for the patch Jason. The patch doesn't apply cleanly on master but I tried looking at the patch through the diff file directly and this is a good direction to move in. Let's cover other Clients too and I'd leave out Embedded for now and tackle that one in the end.

          Show
          anshumg Anshum Gupta added a comment - Thanks for the patch Jason. The patch doesn't apply cleanly on master but I tried looking at the patch through the diff file directly and this is a good direction to move in. Let's cover other Clients too and I'd leave out Embedded for now and tackle that one in the end.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Ok, so after a bit of a delay, I've got an updated patch which attempts to incorporate Shawn and Anshum's input.

          In This Patch

          • builder types for CloudSolrClient, LBHttpSolrClient, HttpSolrClient, and ConcurrentUpdateSolrClient.
          • two abstract types which enabled some sharing of setters: HttpClientBasedSolrClientBuilder, and ResponseParserBasedSolrClientBuilder. I say some , because while the inheritance-chain-to-allow-shared-setters sounded like a great idea, it turned out to be tough in practice. Only one param was present in all builders (HttpClient). Others were more hit-and-miss, which made getting good re-use hard in Java's single-inheritance world.

          Not In This Patch

          • Good Javadocs
          • Much testing.
          • Any deprecation notes.
          • Internal usage of these builder types in SolrJ.

          I plan on eventually adding items in the list above, but I wanted to get some feedback on the builder class structure before continuing. Shawn Heisey, are you happy with the re-use I was able to get from things here? It was your suggestion, so just wanted to make sure I was faithful to your idea and didn't misinterpret what you wanted.

          (Patch is on top of latest trunk.)

          Show
          gerlowskija Jason Gerlowski added a comment - Ok, so after a bit of a delay, I've got an updated patch which attempts to incorporate Shawn and Anshum's input. In This Patch builder types for CloudSolrClient, LBHttpSolrClient, HttpSolrClient, and ConcurrentUpdateSolrClient. two abstract types which enabled some sharing of setters: HttpClientBasedSolrClientBuilder , and ResponseParserBasedSolrClientBuilder . I say some , because while the inheritance-chain-to-allow-shared-setters sounded like a great idea, it turned out to be tough in practice. Only one param was present in all builders (HttpClient). Others were more hit-and-miss, which made getting good re-use hard in Java's single-inheritance world. Not In This Patch Good Javadocs Much testing. Any deprecation notes. Internal usage of these builder types in SolrJ. I plan on eventually adding items in the list above, but I wanted to get some feedback on the builder class structure before continuing. Shawn Heisey , are you happy with the re-use I was able to get from things here? It was your suggestion, so just wanted to make sure I was faithful to your idea and didn't misinterpret what you wanted. (Patch is on top of latest trunk.)
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Just wanted to give this issue a little 'bump'. If there's not much interest in this issue, that's fine with me. Not trying to push this over other important things people are working on. Just wanted to make sure it wasn't missed on accident.

          So if you're interested in seeing this go forward and have time to take a look, please review the attached patch, and let me know what you'd like tweaked/improved in the next iteration.

          Show
          gerlowskija Jason Gerlowski added a comment - Just wanted to give this issue a little 'bump'. If there's not much interest in this issue, that's fine with me. Not trying to push this over other important things people are working on. Just wanted to make sure it wasn't missed on accident. So if you're interested in seeing this go forward and have time to take a look, please review the attached patch, and let me know what you'd like tweaked/improved in the next iteration.
          Hide
          anshumg Anshum Gupta added a comment -

          I'll take a look tomorrow as I'd be away from the computer for most part of today.

          Show
          anshumg Anshum Gupta added a comment - I'll take a look tomorrow as I'd be away from the computer for most part of today.
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Jason. This looks good to me, how ever I'm debating about using an interface instead of an abstract class as that would do away with the single-inheritance restriction. I don't feel too strongly about it so I'm fine with either.

          Here are a few minor things I spotted so far:

          • updateLeadersOnly - let's just keep that as updatesToLeaders for consistency. It would make things easier for users and future developers. If you want to change it, it's best to change it throughout the file.
          • Unused imports cleanup
          • Javadocs wouldn't render correctly as the @return tag swallows the return char.

          A couple of changes in the patch seem unrelated. Let's move them into their own issue:

          • CUSC change seems unrelated.
          • BinaryResponseParser being used as default in the HttpSolrClient constructor and LBHttpSolrClient.

          Let's carry on with adding tests, javadocs, deprecation, and also using this in existing tests. We might not want to change all the tests but optionally pick between builder and existing way of constructing the clients for now in the tests though.

          Show
          anshumg Anshum Gupta added a comment - Thanks Jason. This looks good to me, how ever I'm debating about using an interface instead of an abstract class as that would do away with the single-inheritance restriction. I don't feel too strongly about it so I'm fine with either. Here are a few minor things I spotted so far: updateLeadersOnly - let's just keep that as updatesToLeaders for consistency. It would make things easier for users and future developers. If you want to change it, it's best to change it throughout the file. Unused imports cleanup Javadocs wouldn't render correctly as the @return tag swallows the return char. A couple of changes in the patch seem unrelated. Let's move them into their own issue: CUSC change seems unrelated. BinaryResponseParser being used as default in the HttpSolrClient constructor and LBHttpSolrClient. Let's carry on with adding tests, javadocs, deprecation, and also using this in existing tests. We might not want to change all the tests but optionally pick between builder and existing way of constructing the clients for now in the tests though.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          updateLeadersOnly

          If you want to make it more specific, I believe it should be preferLeaders. updateLeadersOnly does not really make sense.

          Show
          markrmiller@gmail.com Mark Miller added a comment - updateLeadersOnly If you want to make it more specific, I believe it should be preferLeaders. updateLeadersOnly does not really make sense.
          Hide
          anshumg Anshum Gupta added a comment -

          I think we should not use 'prefer' as it's not a preference but an explicit choice to send the request to the shard leaders. It wouldn't fall back to non-leaders if the preferred choice isn't available.

          Show
          anshumg Anshum Gupta added a comment - I think we should not use 'prefer' as it's not a preference but an explicit choice to send the request to the shard leaders. It wouldn't fall back to non-leaders if the preferred choice isn't available.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          I don't think you are right. All it does it prefer leaders. They are tried first, and the replicas are tried after rather than full random. The javadoc is incorrect.

          Show
          markrmiller@gmail.com Mark Miller added a comment - I don't think you are right. All it does it prefer leaders. They are tried first, and the replicas are tried after rather than full random. The javadoc is incorrect.
          Hide
          anshumg Anshum Gupta added a comment -

          Ah ok. I was just looking at the code and it might just be an idea thing, but seems like that param isn't even used right now and that seems strange.
          but yes, I agree that is 'should' only prefer sending the request to the leader.

          I'll dig more into this.

          Show
          anshumg Anshum Gupta added a comment - Ah ok. I was just looking at the code and it might just be an idea thing, but seems like that param isn't even used right now and that seems strange. but yes, I agree that is 'should' only prefer sending the request to the leader. I'll dig more into this.
          Hide
          markrmiller@gmail.com Mark Miller added a comment - - edited

          Yup, looks like someone just made it hardcoded to true. I think Chris Hostetter (Unused) or someone has mentioned valid reasons for keeping the option, so probably we should fix it rather than remove the param in 6.0.

          Show
          markrmiller@gmail.com Mark Miller added a comment - - edited Yup, looks like someone just made it hardcoded to true. I think Chris Hostetter (Unused) or someone has mentioned valid reasons for keeping the option, so probably we should fix it rather than remove the param in 6.0.
          Hide
          hossman Hoss Man added a comment -

          I'm guessing mark is remembering SOLR-6312.

          I don't have a strong opinion about the default behavior – but as for "that param isn't even used right now and that seems strange" that's the heart of SOLR-6312, and that issue mentions reasons why supporting an explicit "false" to do round robin updates can be useful in some usecases.

          Show
          hossman Hoss Man added a comment - I'm guessing mark is remembering SOLR-6312 . I don't have a strong opinion about the default behavior – but as for "that param isn't even used right now and that seems strange" that's the heart of SOLR-6312 , and that issue mentions reasons why supporting an explicit "false" to do round robin updates can be useful in some usecases.
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Hoss. I'll work on adding support for this in SOLR-6312 without changing the default.

          Show
          anshumg Anshum Gupta added a comment - Thanks Hoss. I'll work on adding support for this in SOLR-6312 without changing the default.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Gonna revert my different name (updateLeadersOnly) as Anshum initially suggested, and keep the name updatesToLeaders as it is for now. Maybe the name/behavior/Javadocs should change, but I'd rather not touch that in this JIRA, as I don't have a great understanding of the background there, and it'd take some time to look into that.

          If there's still changes regarding that param, I'm happy to do that in a separate JIRA/patch if you guys want.

          Show
          gerlowskija Jason Gerlowski added a comment - Gonna revert my different name (updateLeadersOnly) as Anshum initially suggested, and keep the name updatesToLeaders as it is for now. Maybe the name/behavior/Javadocs should change, but I'd rather not touch that in this JIRA, as I don't have a great understanding of the background there, and it'd take some time to look into that. If there's still changes regarding that param, I'm happy to do that in a separate JIRA/patch if you guys want.
          Hide
          gerlowskija Jason Gerlowski added a comment - - edited

          Thanks for the review Anshum. Sorry it took me a few days to get back to this. Wanted to respond to your comments:

          • re: updateLeadersOnly. Done; I've changed the name back to updatesToLeaders.
          • re: import cleanup. Done.
          • re: Javadoc return tag. Didn't quite understand your comment. Looking through the patch, I couldn't find the @return tag anywhere. Can you clarify what you meant please?
          • re: CUSC change being unrelated. I think this change is necessary. The CUSC ctor that the change occurs in is now called from CUSCB (CUSCBuilder), which may or may not have a real ExecutorService to pass in. Since CUSC needs an ExecutorService, I changed to ctor to create its own if the param is null. Hence those lines in the patch. I'm sure there's other ways to solve this problem if there's something you don't like about those lines, but they are related/required for the patch as it is now.
          • re: BRP is now used as default in HSC/LBHSC. Looking at these classes, the ctors that don't take in a ResponseParser create their own BinaryResponseParser to take its place. So BRP is already the default here. I had to make this explicit in the ctors that take a ResponseParser, due to the same dynamic mentioned in the bullet point above (That is, the ctor is now called from a builder, where responseParser may or may not have actually been set). So I think this change is also related/required for this patch.
          Show
          gerlowskija Jason Gerlowski added a comment - - edited Thanks for the review Anshum. Sorry it took me a few days to get back to this. Wanted to respond to your comments: re: updateLeadersOnly . Done; I've changed the name back to updatesToLeaders . re: import cleanup. Done. re: Javadoc return tag. Didn't quite understand your comment. Looking through the patch, I couldn't find the @return tag anywhere. Can you clarify what you meant please? re: CUSC change being unrelated. I think this change is necessary. The CUSC ctor that the change occurs in is now called from CUSCB (CUSCBuilder), which may or may not have a real ExecutorService to pass in. Since CUSC needs an ExecutorService, I changed to ctor to create its own if the param is null. Hence those lines in the patch. I'm sure there's other ways to solve this problem if there's something you don't like about those lines, but they are related/required for the patch as it is now. re: BRP is now used as default in HSC/LBHSC. Looking at these classes, the ctors that don't take in a ResponseParser create their own BinaryResponseParser to take its place. So BRP is already the default here. I had to make this explicit in the ctors that take a ResponseParser, due to the same dynamic mentioned in the bullet point above (That is, the ctor is now called from a builder, where responseParser may or may not have actually been set). So I think this change is also related/required for this patch.
          Hide
          gerlowskija Jason Gerlowski added a comment - - edited

          So, looking close at these builder classes, I'm not sure that either an abstract-class, or an interface approach will allow these methods to be shared the way we want.

          In a nutshell, the problem is that each builder type returns itself, so that calls can be chained. Setters inherited from an abstract-class/interface return the type of that abstract interface, which only has a small subset of the methods of the implementation. This really limits how calls can be chained.

          The explanation above seems a little, well, abstract when I re-read it, so maybe an example will clarify what I'm trying to say. Consider:

          abstract class FooBasedBuilder {
              public FooBasedBuilder withFoo(Foo foo) { ... return this;}
              public abstract Bar build();
          }
          
          class BarBuilder extends FooBasedBuilder {
              public BarBuilder withBoo(Boo boo) { ... return this; }
              public Bar build() {...}
          }
          
          // WORKS!
          new BarBuilder()
              .withBoo(...)  //returns BarBuilder
              .withFoo(...)  // BarBuilder has a withFoo() method, so this works.
              .build()
          
          // ERROR!
          new BarBuilder()
              .withFoo(...)  // returns FooBuilder reference
              .withBoo(...)  // FooBuilder type doesn't know about withBar(), so this is a compilation error!
          

          As the examples above show, with the abstract-class-for-code-sharing design, the order that setters are called in matters. Once a parent class method is called, subclass methods can't be chained on.

          Maybe there's a way around this, but I haven't been able to find one after spending an hour or so racking my brains about it. With this in mind, if no one can find an alternative, I'm going to go back to removing the code-sharing portion of this patch, as much as that sucks.

          If no one has any better ideas, I'm hoping to push up a patch with this change (and the others Anshum suggested above).

          Show
          gerlowskija Jason Gerlowski added a comment - - edited So, looking close at these builder classes, I'm not sure that either an abstract-class, or an interface approach will allow these methods to be shared the way we want. In a nutshell, the problem is that each builder type returns itself, so that calls can be chained. Setters inherited from an abstract-class/interface return the type of that abstract interface, which only has a small subset of the methods of the implementation. This really limits how calls can be chained. The explanation above seems a little, well, abstract when I re-read it, so maybe an example will clarify what I'm trying to say. Consider: abstract class FooBasedBuilder { public FooBasedBuilder withFoo(Foo foo) { ... return this ;} public abstract Bar build(); } class BarBuilder extends FooBasedBuilder { public BarBuilder withBoo(Boo boo) { ... return this ; } public Bar build() {...} } // WORKS! new BarBuilder() .withBoo(...) //returns BarBuilder .withFoo(...) // BarBuilder has a withFoo() method, so this works. .build() // ERROR! new BarBuilder() .withFoo(...) // returns FooBuilder reference .withBoo(...) // FooBuilder type doesn't know about withBar(), so this is a compilation error! As the examples above show, with the abstract-class-for-code-sharing design, the order that setters are called in matters. Once a parent class method is called, subclass methods can't be chained on. Maybe there's a way around this, but I haven't been able to find one after spending an hour or so racking my brains about it. With this in mind, if no one can find an alternative, I'm going to go back to removing the code-sharing portion of this patch, as much as that sucks. If no one has any better ideas, I'm hoping to push up a patch with this change (and the others Anshum suggested above).
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Puts patch on top of latest trunk/master. Fixes some of Anshum's concerns. Still needs tests, fixes based on comments above before it's ready for another review. But I wanted to push up the changes I have so far at least.

          Show
          gerlowskija Jason Gerlowski added a comment - Puts patch on top of latest trunk/master. Fixes some of Anshum's concerns. Still needs tests, fixes based on comments above before it's ready for another review. But I wanted to push up the changes I have so far at least.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Adds tests for each builder. Removes abstract-class approach.

          Still need to push usage of builders into other SolrJ tests. Coming soon hopefully.

          Show
          gerlowskija Jason Gerlowski added a comment - Adds tests for each builder. Removes abstract-class approach. Still need to push usage of builders into other SolrJ tests. Coming soon hopefully.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Let's carry on with adding tests, javadocs, deprecation, and also using this in existing tests. We might not want to change all the tests but optionally pick between builder and existing way of constructing the clients for now in the tests though.

          Time for a short check-in on making the revisions you suggested Anshum Gupta:

          • tests? check.
          • javadocs? check.
          • deprecation? nope.
          • using in existing tests? In progress (see below).

          The attached patch changes all tests that previously created HttpSolrClient objects directly. With this patch, these tests now either use the builder, or create the objects directly, based on the tests' random() value. Is that about what you were thinking of when you suggested this Anshum?

          Should this (or a similar change) be made to all test files that create SolrClients, or just enough of the tests to get decent exposure for the builder? I'd push for creating SolrClients uniformly in all the tests, though that does make this patch/commit much more burdensome in many aspects (keeping it up to date, reviewing it, etc.). So I see arguments either way. I'll revise the changes in the HttpSolrClient-consuming tests, and make similar changes for tests consuming the other types of SolrClients once I hear any thoughts that others might have.

          Show
          gerlowskija Jason Gerlowski added a comment - Let's carry on with adding tests, javadocs, deprecation, and also using this in existing tests. We might not want to change all the tests but optionally pick between builder and existing way of constructing the clients for now in the tests though. Time for a short check-in on making the revisions you suggested Anshum Gupta : tests? check. javadocs? check. deprecation? nope. using in existing tests? In progress (see below). The attached patch changes all tests that previously created HttpSolrClient objects directly. With this patch, these tests now either use the builder, or create the objects directly, based on the tests' random() value. Is that about what you were thinking of when you suggested this Anshum? Should this (or a similar change) be made to all test files that create SolrClients, or just enough of the tests to get decent exposure for the builder? I'd push for creating SolrClients uniformly in all the tests, though that does make this patch/commit much more burdensome in many aspects (keeping it up to date, reviewing it, etc.). So I see arguments either way. I'll revise the changes in the HttpSolrClient-consuming tests, and make similar changes for tests consuming the other types of SolrClients once I hear any thoughts that others might have.
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Jason. Looking at it now. Let's not move all the tests and have a massive patch. Also, we need to continue testing the current SolrJ implementation too i.e. non-builder pattern.

          Show
          anshumg Anshum Gupta added a comment - Thanks Jason. Looking at it now. Let's not move all the tests and have a massive patch. Also, we need to continue testing the current SolrJ implementation too i.e. non-builder pattern.
          Hide
          anshumg Anshum Gupta added a comment -

          Seems like there's some issue with the patch as it has .orig files.
          Do you mind posting another one?

          Show
          anshumg Anshum Gupta added a comment - Seems like there's some issue with the patch as it has .orig files. Do you mind posting another one?
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Oops, the carelessness of git add -A (and the user who ran it) strikes again.

          Sorry for wasting your time. The updated patch should work.

          Show
          gerlowskija Jason Gerlowski added a comment - Oops, the carelessness of git add -A (and the user who ran it) strikes again. Sorry for wasting your time. The updated patch should work.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          we need to continue testing the current SolrJ implementation too

          Does RandomizedSolrClientCreator address your concerns there? I wrote it with that in mind, but I might've misinterpretted what you were asking for. It does a coin flip using each test's Random object, and does builder or non-builder SolrClient creation based on the result. I thought that was what you were suggested having a way to "optionally pick between builder and existing way" in your comment above.

          I'm happy to back out the changes to all the tests if you aren't happy with a coin-flip-creation approach here. In that case, should I only be changing a subset of the tests?

          Show
          gerlowskija Jason Gerlowski added a comment - we need to continue testing the current SolrJ implementation too Does RandomizedSolrClientCreator address your concerns there? I wrote it with that in mind, but I might've misinterpretted what you were asking for. It does a coin flip using each test's Random object, and does builder or non-builder SolrClient creation based on the result. I thought that was what you were suggested having a way to "optionally pick between builder and existing way" in your comment above. I'm happy to back out the changes to all the tests if you aren't happy with a coin-flip-creation approach here. In that case, should I only be changing a subset of the tests?
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Hmm, still seeing some test failures with this patch. Haven't narrowed down the problem yet, but I'm working on it.

          Not ready for review yet (other than general feedback on how to expose the new builders into existing tests).

          Show
          gerlowskija Jason Gerlowski added a comment - Hmm, still seeing some test failures with this patch. Haven't narrowed down the problem yet, but I'm working on it. Not ready for review yet (other than general feedback on how to expose the new builders into existing tests).
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Updates patch on top of recent changes in master. Tests pass locally. Should be ready for review.

          The only/main open question on this that I know of is "testing". Right now the patch changes many tests to randomly choose between (1) normal creation and (2) using the new builder when creating HttpSolrClient instances. It does nothing for other SolrClient implementations (excepting unit tests on the builders themselves, which are included).

          It was kindof unclear to me whether these changes were desired or not. I'm happy to move this in either direction (remove the random-client-creation changes vs. expanding the changes to encompass other SolrClient types), based on how people would like this to be structured. Looking for some feedback here in particular; I don't know how to move forward until the testing approach either gets a "thumbs up" or "thumbs down".

          Show
          gerlowskija Jason Gerlowski added a comment - Updates patch on top of recent changes in master. Tests pass locally. Should be ready for review. The only/main open question on this that I know of is "testing". Right now the patch changes many tests to randomly choose between (1) normal creation and (2) using the new builder when creating HttpSolrClient instances. It does nothing for other SolrClient implementations (excepting unit tests on the builders themselves, which are included). It was kindof unclear to me whether these changes were desired or not. I'm happy to move this in either direction (remove the random-client-creation changes vs. expanding the changes to encompass other SolrClient types), based on how people would like this to be structured. Looking for some feedback here in particular; I don't know how to move forward until the testing approach either gets a "thumbs up" or "thumbs down".
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Jason. Looks good overall.

          • Change maybeCreateHttpSolrClientWithBuilder to getNewSolrClient() ? 'maybe' makes it sound like a client may/may not be created. Also, with a method called getNewSolrClient(), we could just change it to always construct a new client object using the new design, without having to rename the method to a non-maybe* pattern. Also, we passing the 'random()' isn't really required.
          • Deprecation to the non-builder calls. Ideally, we should move the builder classes to be static inner classes for the existing Client implementations. Then we could switch everything to private and leave out the Builder exposed when we want to remove the builder, rather than moving the code around.
          • CUSC, and HttpSolrClient changes are unrelated
          • Do you plan on adding more tests to ConcurrentUpdateSolrClientBuilderTest ?
          • There are a few unused imports, we can clean them out before committing.

          I'm happy to move this in either direction (remove the random-client-creation changes vs. expanding the changes to encompass other SolrClient types)

          Let's have a getter for randomizing client creation, while keeping the concept of randomizing transparent i.e. the calling code doesn't ever know when we randomize. Also, let's have other clients covered too.

          I'll create a sub-task so we don't forget that we intend to rename updatesToLeaders to preferLeaders. Feel free to create sub-tasks for everything that you think is related but doesn't need to go with this.

          Show
          anshumg Anshum Gupta added a comment - Thanks Jason. Looks good overall. Change maybeCreateHttpSolrClientWithBuilder to getNewSolrClient() ? 'maybe' makes it sound like a client may/may not be created. Also, with a method called getNewSolrClient(), we could just change it to always construct a new client object using the new design, without having to rename the method to a non-maybe* pattern. Also, we passing the 'random()' isn't really required. Deprecation to the non-builder calls. Ideally, we should move the builder classes to be static inner classes for the existing Client implementations. Then we could switch everything to private and leave out the Builder exposed when we want to remove the builder, rather than moving the code around. CUSC, and HttpSolrClient changes are unrelated Do you plan on adding more tests to ConcurrentUpdateSolrClientBuilderTest ? There are a few unused imports, we can clean them out before committing. I'm happy to move this in either direction (remove the random-client-creation changes vs. expanding the changes to encompass other SolrClient types) Let's have a getter for randomizing client creation, while keeping the concept of randomizing transparent i.e. the calling code doesn't ever know when we randomize. Also, let's have other clients covered too. I'll create a sub-task so we don't forget that we intend to rename updatesToLeaders to preferLeaders. Feel free to create sub-tasks for everything that you think is related but doesn't need to go with this.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Thanks for the review Anshum. Made all the changes you suggested with 1 or 2 exceptions:

          • I wanted to add more tests to ConcurrentUpdateSolrClientBuilderTest, but for some reason ConcurrentUpdateSolrClient lacks many of the getters that other SolrClients have. I wrote tests for everything that has getters, but for ConcurrentUpdateSolrClient that's not that much.
          • The change to CUSC wasn't unrelated. The CUSC builder now calls that ctor in such a way that the ExecutorService ​could​ be null. So the CUSC builder now needs to be able to handle that case. (We actually already discussed this, just leaving this note for completeness.)

          Other than that, I've made all the changes you suggested. All tests pass locally. Should be ready to go (or at least, review again).

          Show
          gerlowskija Jason Gerlowski added a comment - Thanks for the review Anshum. Made all the changes you suggested with 1 or 2 exceptions: I wanted to add more tests to ConcurrentUpdateSolrClientBuilderTest, but for some reason ConcurrentUpdateSolrClient lacks many of the getters that other SolrClients have. I wrote tests for everything that has getters, but for ConcurrentUpdateSolrClient that's not that much. The change to CUSC wasn't unrelated. The CUSC builder now calls that ctor in such a way that the ExecutorService ​ could ​ be null. So the CUSC builder now needs to be able to handle that case. (We actually already discussed this, just leaving this note for completeness.) Other than that, I've made all the changes you suggested. All tests pass locally. Should be ready to go (or at least, review again).
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Thanks for the review Anshum. Made all the changes you suggested with 1 or 2 exceptions:

          • I wanted to add more tests to ConcurrentUpdateSolrClientBuilderTest, but for some reason ConcurrentUpdateSolrClient lacks many of the getters that other SolrClients have. I wrote tests for everything that has getters, but for ConcurrentUpdateSolrClient that's not that much.
          • The change to CUSC wasn't unrelated. The CUSC builder now calls that ctor in such a way that the ExecutorService ​could​ be null. So the CUSC builder now needs to be able to handle that case. (We actually already discussed this, just leaving this note for completeness.)

          Other than that, I've made all the changes you suggested. All tests pass locally. Should be ready to go (or at least, review again).

          Show
          gerlowskija Jason Gerlowski added a comment - Thanks for the review Anshum. Made all the changes you suggested with 1 or 2 exceptions: I wanted to add more tests to ConcurrentUpdateSolrClientBuilderTest, but for some reason ConcurrentUpdateSolrClient lacks many of the getters that other SolrClients have. I wrote tests for everything that has getters, but for ConcurrentUpdateSolrClient that's not that much. The change to CUSC wasn't unrelated. The CUSC builder now calls that ctor in such a way that the ExecutorService ​ could ​ be null. So the CUSC builder now needs to be able to handle that case. (We actually already discussed this, just leaving this note for completeness.) Other than that, I've made all the changes you suggested. All tests pass locally. Should be ready to go (or at least, review again).
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Thanks for the review Anshum. Made all the changes you suggested with
          1 or 2 exceptions:

          • I wanted to add more tests to ConcurrentUpdateSolrClientBuilderTest,
            but for some reason ConcurrentUpdateSolrClient lacks many of the
            getters that other SolrClients have. I wrote tests for everything
            that has getters, but for ConcurrentUpdateSolrClient that's not that
            much.
          • The change to CUSC wasn't unrelated. The CUSC builder now calls
            that ctor in such a way that the ExecutorService could be null. So
            the CUSC builder now needs to be able to handle that case. (We
            actually already discussed this, just leaving this note for
            completeness.)

          Other than that, I've made all the changes you suggested. All tests
          pass locally. Should be ready to go (or at least, review again).

          Thanks!

          Show
          gerlowskija Jason Gerlowski added a comment - Thanks for the review Anshum. Made all the changes you suggested with 1 or 2 exceptions: I wanted to add more tests to ConcurrentUpdateSolrClientBuilderTest, but for some reason ConcurrentUpdateSolrClient lacks many of the getters that other SolrClients have. I wrote tests for everything that has getters, but for ConcurrentUpdateSolrClient that's not that much. The change to CUSC wasn't unrelated. The CUSC builder now calls that ctor in such a way that the ExecutorService could be null. So the CUSC builder now needs to be able to handle that case. (We actually already discussed this, just leaving this note for completeness.) Other than that, I've made all the changes you suggested. All tests pass locally. Should be ready to go (or at least, review again). Thanks!
          Hide
          anshumg Anshum Gupta added a comment -

          I'll take a look at this later today.

          Show
          anshumg Anshum Gupta added a comment - I'll take a look at this later today.
          Hide
          anshumg Anshum Gupta added a comment -

          Seems like you forgot to change the usage of deprecated constructors in the code. You changed it in the tests, but not the rest of the code base.
          The rest seems great.

          Show
          anshumg Anshum Gupta added a comment - Seems like you forgot to change the usage of deprecated constructors in the code. You changed it in the tests, but not the rest of the code base. The rest seems great.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          I didn't forget, but I was unsure whether that should be included in this JIRA or not. (I'm personally for changing the usage in production code, I was just being conservative.)

          I'll upload a patch shortly removing use of the deprecated ctors entirely.

          Show
          gerlowskija Jason Gerlowski added a comment - I didn't forget, but I was unsure whether that should be included in this JIRA or not. (I'm personally for changing the usage in production code, I was just being conservative.) I'll upload a patch shortly removing use of the deprecated ctors entirely.
          Hide
          anshumg Anshum Gupta added a comment -

          We should not be using deprecated code for anything other than back-compat testing

          Show
          anshumg Anshum Gupta added a comment - We should not be using deprecated code for anything other than back-compat testing
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Ok, this removes all* uses of the now-deprecated constructors.

          The asterisk above implies there are still a few cases where the constructors are called.

          • In SolrTestCaseJ4, where there are utility methods that randomly choose whether to create clients using the builder, or the now-deprecated ctor.
          • There are a few classes which have a private SolrClient subclass. In almost all cases this is done to specify some alternate behavior for SolrClient.handleError(). I changed these all to use the "largest" ctor for the SolrClient they're extending (the ctor with all parameters, that will be kept around with a reduced visibility after the other ctors are deleted.

          I'm not sure I described the second case well, but if my description is unclear, check out StreamingSolrClients.java, which has a good example of this.

          All other uses have been removed.

          As a side note, in this last revision, I realized that deprecating, eventually removing, and lowering the visibility of SolrClient ctors makes implementations difficult to extend anonymously. This isn't a big deal. Just wanted to mention it in case it bothers anyone.

          This should be ready to go Anshum Gupta!

          Show
          gerlowskija Jason Gerlowski added a comment - Ok, this removes all* uses of the now-deprecated constructors. The asterisk above implies there are still a few cases where the constructors are called. In SolrTestCaseJ4 , where there are utility methods that randomly choose whether to create clients using the builder, or the now-deprecated ctor. There are a few classes which have a private SolrClient subclass. In almost all cases this is done to specify some alternate behavior for SolrClient.handleError() . I changed these all to use the "largest" ctor for the SolrClient they're extending (the ctor with all parameters, that will be kept around with a reduced visibility after the other ctors are deleted. I'm not sure I described the second case well, but if my description is unclear, check out StreamingSolrClients.java , which has a good example of this. All other uses have been removed. As a side note, in this last revision, I realized that deprecating, eventually removing, and lowering the visibility of SolrClient ctors makes implementations difficult to extend anonymously. This isn't a big deal. Just wanted to mention it in case it bothers anyone. This should be ready to go Anshum Gupta !
          Hide
          anshumg Anshum Gupta added a comment - - edited

          Jason Gerlowski can you update the patch to current master and I'll take a final look and commit.

          Show
          anshumg Anshum Gupta added a comment - - edited Jason Gerlowski can you update the patch to current master and I'll take a final look and commit.
          Hide
          anshumg Anshum Gupta added a comment -

          I don't think that difficulty in anonymous extension of SolrClient should stop us from doing this. If no one else has a problem with this, I'll go ahead and commit.

          Show
          anshumg Anshum Gupta added a comment - I don't think that difficulty in anonymous extension of SolrClient should stop us from doing this. If no one else has a problem with this, I'll go ahead and commit.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Yep, working on updating the patch now. It looks like someone's added a new HttpSolrClient ctor while this patch has been outstanding, so updating the patch is a little more involved now. Hope to have something up shortly still though.

          Show
          gerlowskija Jason Gerlowski added a comment - Yep, working on updating the patch now. It looks like someone's added a new HttpSolrClient ctor while this patch has been outstanding, so updating the patch is a little more involved now. Hope to have something up shortly still though.
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Jason

          Show
          anshumg Anshum Gupta added a comment - Thanks Jason
          Hide
          elyograg Shawn Heisey added a comment -

          We could completely eliminate all the public constructors in master. If a constructor is actually needed by the Builder classes, we would leave only one of them (the one with all the options) marked "protected". Because the Builder would be in the same package, it would have access to that constructor.

          I just noticed that the HttpSolrClientBuilder class does not have its own .java file. I think that the builders should be entirely separate classes. This is the pattern that HttpClient uses. I'm not sure whether this enough to veto the patch, but it's how I think we should do it.

          There's no way to know whether people are actually trying to extend specific SolrClient implementations, but I would not expect that to be common. A single protected constructor would allow some leeway in this regard.

          Show
          elyograg Shawn Heisey added a comment - We could completely eliminate all the public constructors in master. If a constructor is actually needed by the Builder classes, we would leave only one of them (the one with all the options) marked "protected". Because the Builder would be in the same package, it would have access to that constructor. I just noticed that the HttpSolrClientBuilder class does not have its own .java file. I think that the builders should be entirely separate classes. This is the pattern that HttpClient uses. I'm not sure whether this enough to veto the patch, but it's how I think we should do it. There's no way to know whether people are actually trying to extend specific SolrClient implementations, but I would not expect that to be common. A single protected constructor would allow some leeway in this regard.
          Hide
          elyograg Shawn Heisey added a comment -

          Further research on this turns up both methods as valid – including the builder class inside the class it's building, or as a separate class.

          The example that I found where it was an internal class just named it "Builder" – which I think is wise if we are going to embed the class. In that case, we would not need any public or protected constructors, just a single private constructor that takes the Builder object.

          I do like the separate class idea, but I would not be opposed to HttpSolrClient.Builder instead.

          Show
          elyograg Shawn Heisey added a comment - Further research on this turns up both methods as valid – including the builder class inside the class it's building, or as a separate class. The example that I found where it was an internal class just named it "Builder" – which I think is wise if we are going to embed the class. In that case, we would not need any public or protected constructors, just a single private constructor that takes the Builder object. I do like the separate class idea, but I would not be opposed to HttpSolrClient.Builder instead.
          Hide
          anshumg Anshum Gupta added a comment -

          from my previous comment:

          Deprecation to the non-builder calls. Ideally, we should move the builder classes to be static inner classes for the existing Client implementations. Then we could switch everything to private and leave out the Builder exposed when we want to remove the builder, rather than moving the code around.

          I would have liked a separate class but for the purpose of back-compat and then having to move the code around when we let go of the compat, it makes sense to have the builder as an inner class. If you think *.Builder makes more sense than Builder, I could buy that.

          Show
          anshumg Anshum Gupta added a comment - from my previous comment: Deprecation to the non-builder calls. Ideally, we should move the builder classes to be static inner classes for the existing Client implementations. Then we could switch everything to private and leave out the Builder exposed when we want to remove the builder, rather than moving the code around. I would have liked a separate class but for the purpose of back-compat and then having to move the code around when we let go of the compat, it makes sense to have the builder as an inner class. If you think *.Builder makes more sense than Builder, I could buy that.
          Hide
          gerlowskija Jason Gerlowski added a comment - - edited

          So, I've got an updated patch that does everything except the renaming. I'm fine w/ shortening the names of the static *Builder classes, but when I went to do that, I ran into a bunch of compilation issues related to "ambiguous references" or "type mismatch".

          The root of the problem appears to be name conflicts, most of which are with HttpClient's Builder class. As an example, look at this snippet of code:

                    Builder requestConfigBuilder = HttpClientUtil.createDefaultRequestConfigBuilder();
                    if (soTimeout != null) {
                      requestConfigBuilder.setSocketTimeout(soTimeout);
                    }
                    if (connectionTimeout != null) {
                      requestConfigBuilder.setConnectTimeout(connectionTimeout);
                    }
          

          This will fail to compile with the message: "Type mismatch: Cannot convert from RequestConfig.Builder to ConcurrentUpdateSolrClient.Builder". This is easily fixable by changing all Builder references to be unambiguous (RequestConfig.Builder or ConcurrentUpdateSolrClient.Builder). I'm happy to do that, if this is what we want, but I'm reluctant to let this patch get much bigger than it already is, so I wanted to check with others before piling on and making the review more onerous.

          That said, I'm happy to pull the trigger if we're sure this is what we want. If anyone else sees a better way around this that I'm missing, please let me know. Otherwise, if there's no objections, I'll go forward with this later today.

          Show
          gerlowskija Jason Gerlowski added a comment - - edited So, I've got an updated patch that does everything except the renaming. I'm fine w/ shortening the names of the static *Builder classes, but when I went to do that, I ran into a bunch of compilation issues related to "ambiguous references" or "type mismatch". The root of the problem appears to be name conflicts, most of which are with HttpClient's Builder class. As an example, look at this snippet of code: Builder requestConfigBuilder = HttpClientUtil.createDefaultRequestConfigBuilder(); if (soTimeout != null ) { requestConfigBuilder.setSocketTimeout(soTimeout); } if (connectionTimeout != null ) { requestConfigBuilder.setConnectTimeout(connectionTimeout); } This will fail to compile with the message: "Type mismatch: Cannot convert from RequestConfig.Builder to ConcurrentUpdateSolrClient.Builder". This is easily fixable by changing all Builder references to be unambiguous (RequestConfig.Builder or ConcurrentUpdateSolrClient.Builder). I'm happy to do that, if this is what we want, but I'm reluctant to let this patch get much bigger than it already is, so I wanted to check with others before piling on and making the review more onerous. That said, I'm happy to pull the trigger if we're sure this is what we want. If anyone else sees a better way around this that I'm missing, please let me know. Otherwise, if there's no objections, I'll go forward with this later today.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Uploading a patch that has everything except the renames. (See my comment above for more information about the Builder renames)

          Show
          gerlowskija Jason Gerlowski added a comment - Uploading a patch that has everything except the renames. (See my comment above for more information about the Builder renames)
          Hide
          elyograg Shawn Heisey added a comment -

          I didn't think of potential naming conflicts.

          Fully qualified references for all "Builder" classes other than the local one seems like a reasonable solution. Or we could pull the builders into separate source files, but I don't think that would really reduce verbosity by enough to matter. The only REAL advantage to separate classes is that I like the clean separation. The relationship between a class and its builder is pretty intimate, so embedding is understandable.

          I will look at your patch later, when I have more than a few minutes.

          Patches will be as big as necessary. Keeping the size down is a good idea when possible, but don't worry too much about the size. Just make sure that the patch only makes required changes; keep away from massive code reformats.

          Show
          elyograg Shawn Heisey added a comment - I didn't think of potential naming conflicts. Fully qualified references for all "Builder" classes other than the local one seems like a reasonable solution. Or we could pull the builders into separate source files, but I don't think that would really reduce verbosity by enough to matter. The only REAL advantage to separate classes is that I like the clean separation. The relationship between a class and its builder is pretty intimate, so embedding is understandable. I will look at your patch later, when I have more than a few minutes. Patches will be as big as necessary. Keeping the size down is a good idea when possible, but don't worry too much about the size. Just make sure that the patch only makes required changes; keep away from massive code reformats.
          Hide
          anshumg Anshum Gupta added a comment -

          Here's an updated patch with renaming. I plan on getting this into master later today.

          Show
          anshumg Anshum Gupta added a comment - Here's an updated patch with renaming. I plan on getting this into master later today.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit b02b026b7d979a9dac3c549c156dd9706e6fc5ba in lucene-solr's branch refs/heads/master from Anshum Gupta
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b02b026 ]

          SOLR-8097: Implement builder pattern design for constructing SolrJ clients and deprecate direct construction of clients

          Show
          jira-bot ASF subversion and git services added a comment - Commit b02b026b7d979a9dac3c549c156dd9706e6fc5ba in lucene-solr's branch refs/heads/master from Anshum Gupta [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b02b026 ] SOLR-8097 : Implement builder pattern design for constructing SolrJ clients and deprecate direct construction of clients
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Jason and Shawn. I'll commit this to 6x tomorrow.

          Show
          anshumg Anshum Gupta added a comment - Thanks Jason and Shawn. I'll commit this to 6x tomorrow.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f479f16d3a8b57126560c19c57885a103360f1c3 in lucene-solr's branch refs/heads/branch_6x from Anshum Gupta
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f479f16 ]

          SOLR-8097: Implement builder pattern design for constructing SolrJ clients and deprecate direct construction of clients

          Show
          jira-bot ASF subversion and git services added a comment - Commit f479f16d3a8b57126560c19c57885a103360f1c3 in lucene-solr's branch refs/heads/branch_6x from Anshum Gupta [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f479f16 ] SOLR-8097 : Implement builder pattern design for constructing SolrJ clients and deprecate direct construction of clients
          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          Can we deprecate the various setters inside HttpSolrClient and remove them from master? Some of those are missing in the Builder too e.g. setRequestWriter

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - Can we deprecate the various setters inside HttpSolrClient and remove them from master? Some of those are missing in the Builder too e.g. setRequestWriter
          Hide
          anshumg Anshum Gupta added a comment -

          Sure

          Show
          anshumg Anshum Gupta added a comment - Sure
          Hide
          gerlowskija Jason Gerlowski added a comment -

          I thought about doing that as a part of my patch, but it seemed like it might be handled best in a separate JIRA. I'd be happy to push up a patch with that change in a sub-task if it's one that people are interested in.

          One question I have is whether there's ever a valid use-case for changing/tweaking a SolrClient after creating it. (i.e. Does anyone ever change their RequestWriter, zkHost, collection, etc, instead of just creating a new client). I'm not implying that there is a good reason to do so, or that I'm against removing the setters (I'm all for it). (Just want to make sure, after the issue I partially caused in SOLR-8642)

          Show
          gerlowskija Jason Gerlowski added a comment - I thought about doing that as a part of my patch, but it seemed like it might be handled best in a separate JIRA. I'd be happy to push up a patch with that change in a sub-task if it's one that people are interested in. One question I have is whether there's ever a valid use-case for changing/tweaking a SolrClient after creating it. (i.e. Does anyone ever change their RequestWriter, zkHost, collection, etc, instead of just creating a new client). I'm not implying that there is a good reason to do so, or that I'm against removing the setters (I'm all for it). (Just want to make sure, after the issue I partially caused in SOLR-8642 )
          Hide
          anshumg Anshum Gupta added a comment -

          Collection, perhaps yes. But I don't change any of the others myself. At the highest level, for anything we do, we need to handle the following (at least):
          1. If we are changing APIs, it's a deprecation, specially in minor versions and nothing is broken.
          2. There are tests for the changed behavior

          We have handled those well here.

          Also, let's do that as a separate JIRA. I don't have strong opinions on that.

          Show
          anshumg Anshum Gupta added a comment - Collection, perhaps yes. But I don't change any of the others myself. At the highest level, for anything we do, we need to handle the following (at least): 1. If we are changing APIs, it's a deprecation, specially in minor versions and nothing is broken. 2. There are tests for the changed behavior We have handled those well here. Also, let's do that as a separate JIRA. I don't have strong opinions on that.
          Hide
          elyograg Shawn Heisey added a comment -

          I personally would never change those things after the first use of a client – not even the default collection on a cloud client. If the client is being used by multiple threads, determining what will happen if you change the client after you start using it is difficult.

          IMHO this makes all setters superfluous, even setDefaultCollection. Anything a setter does should be handled by the builder, and you can supply a collection parameter to methods that make requests, so you're never locked in to the default.

          +1 to open a new issue to deprecate the setters in branch_6x and remove in master, after making sure all options are handled by the builder. If the javadoc on the setters doesn't already say so, it should indicate that they are not thread-safe and should only be used immediately after object creation.

          Show
          elyograg Shawn Heisey added a comment - I personally would never change those things after the first use of a client – not even the default collection on a cloud client. If the client is being used by multiple threads, determining what will happen if you change the client after you start using it is difficult. IMHO this makes all setters superfluous, even setDefaultCollection. Anything a setter does should be handled by the builder, and you can supply a collection parameter to methods that make requests, so you're never locked in to the default. +1 to open a new issue to deprecate the setters in branch_6x and remove in master, after making sure all options are handled by the builder. If the javadoc on the setters doesn't already say so, it should indicate that they are not thread-safe and should only be used immediately after object creation.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          I created SOLR-8975 for this work, with a short description. I'll copy over some more of the details/suggestions/requirements you've described here shortly, and get started.

          Thanks for chiming in, let's continue this discussion over on SOLR-8975.

          Show
          gerlowskija Jason Gerlowski added a comment - I created SOLR-8975 for this work, with a short description. I'll copy over some more of the details/suggestions/requirements you've described here shortly, and get started. Thanks for chiming in, let's continue this discussion over on SOLR-8975 .
          Hide
          hossman Hoss Man added a comment -

          Anshum Gupta - shouldn't this issue be resolved?

          Show
          hossman Hoss Man added a comment - Anshum Gupta - shouldn't this issue be resolved?
          Hide
          anshumg Anshum Gupta added a comment -

          Yes but it was only fixed for 6.1.

          Show
          anshumg Anshum Gupta added a comment - Yes but it was only fixed for 6.1.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9645d4213292121d2f011f5440684ea25d7beaa3 in lucene-solr's branch refs/heads/branch_6_0 from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9645d42 ]

          SOLR-9028: Fixed some test related bugs preventing SSL + ClientAuth from ever being tested
          (cherry picked from commit 791d1e7)

          Conflicts:
          solr/core/src/test/org/apache/solr/cloud/SSLMigrationTest.java
          solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java
          solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
          solr/test-framework/src/java/org/apache/solr/util/SSLTestConfig.java

          Conflicts:
          solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java

          For branch_6_0: Since SOLR-8097 will land in 6.1, remove calls in TestMiniSolrCloudClusterSSL to SolrTestCaseJ4.getHttpSolrClient().

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9645d4213292121d2f011f5440684ea25d7beaa3 in lucene-solr's branch refs/heads/branch_6_0 from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9645d42 ] SOLR-9028 : Fixed some test related bugs preventing SSL + ClientAuth from ever being tested (cherry picked from commit 791d1e7) Conflicts: solr/core/src/test/org/apache/solr/cloud/SSLMigrationTest.java solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java solr/test-framework/src/java/org/apache/solr/util/SSLTestConfig.java Conflicts: solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java For branch_6_0: Since SOLR-8097 will land in 6.1, remove calls in TestMiniSolrCloudClusterSSL to SolrTestCaseJ4.getHttpSolrClient().
          Hide
          perrin.bignoli Perrin Bignoli added a comment - - edited

          Why is the the visibility of the following constructor in CloudSolrClient:

          private CloudSolrClient(Collection<String> zkHosts, String chroot, HttpClient httpClient, LBHttpSolrClient lbSolrClient,
          boolean updatesToLeaders, boolean directUpdatesToLeadersOnly)

          set to private and not protected?

          There are also a number of private variables in CloudSolrClient that make subclassing difficult. I am not familiar enough with the source code to make an exhaustive list.

          Show
          perrin.bignoli Perrin Bignoli added a comment - - edited Why is the the visibility of the following constructor in CloudSolrClient: private CloudSolrClient(Collection<String> zkHosts, String chroot, HttpClient httpClient, LBHttpSolrClient lbSolrClient, boolean updatesToLeaders, boolean directUpdatesToLeadersOnly) set to private and not protected? There are also a number of private variables in CloudSolrClient that make subclassing difficult. I am not familiar enough with the source code to make an exhaustive list.
          Show
          anshumg Anshum Gupta added a comment - Here's the discussion about that: https://issues.apache.org/jira/browse/SOLR-8097?focusedCommentId=15227338&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15227338
          Hide
          perrin.bignoli Perrin Bignoli added a comment -

          What was the result of that discussion?

          I am interested in creating a subclass of CloudSolrClient. I don't see how that is possible with the current code or if there are only private constructors. Other *SolrClient classes appear to have a protected "Builder" constructor. They also have external Builder classes (at least on Master). Is there a reason why CloudSolrClient is set up to prevent subclassing? Please let me know if I am missing something obvious.

          Also, that discussion does not involve member variable visibility, although that is probably outside of the scope of this particular ticket.

          Show
          perrin.bignoli Perrin Bignoli added a comment - What was the result of that discussion? I am interested in creating a subclass of CloudSolrClient. I don't see how that is possible with the current code or if there are only private constructors. Other *SolrClient classes appear to have a protected "Builder" constructor. They also have external Builder classes (at least on Master). Is there a reason why CloudSolrClient is set up to prevent subclassing? Please let me know if I am missing something obvious. Also, that discussion does not involve member variable visibility, although that is probably outside of the scope of this particular ticket.
          Hide
          elyograg Shawn Heisey added a comment -

          What is your specific goal in creating a subclass? I ask because the X is usually more important than the Y. See this:

          http://people.apache.org/~hossman/#xyproblem

          A SolrClient object is a complex thing, particularly the Cloud version. Although we try to keep the public API from changing much in minor releases, the internal implementation is a VERY different story. Because the implementation can change dramatically from release to release, certain details are kept private. This reduces the risk of breaking user code.

          That said, there really is no reason we should prevent subclassing like we currently do, even if we recommend not doing it because it makes user code brittle.

          It makes sense to change the kitchen-sink constructor from private to protected. SOLR-8975 might be a good place to tackle this, but it might need its own issue.

          I think we should also recommend extending the Builder when subclassing. When 7.0 is released, all public constructors will be gone, and the Builder will be the only way to create a client object.

          Show
          elyograg Shawn Heisey added a comment - What is your specific goal in creating a subclass? I ask because the X is usually more important than the Y. See this: http://people.apache.org/~hossman/#xyproblem A SolrClient object is a complex thing, particularly the Cloud version. Although we try to keep the public API from changing much in minor releases, the internal implementation is a VERY different story. Because the implementation can change dramatically from release to release, certain details are kept private. This reduces the risk of breaking user code. That said, there really is no reason we should prevent subclassing like we currently do, even if we recommend not doing it because it makes user code brittle. It makes sense to change the kitchen-sink constructor from private to protected. SOLR-8975 might be a good place to tackle this, but it might need its own issue. I think we should also recommend extending the Builder when subclassing. When 7.0 is released, all public constructors will be gone, and the Builder will be the only way to create a client object.
          Hide
          anshumg Anshum Gupta added a comment -

          Marking this as resolved to avoid confusion.

          P.S: That only means that this was committed and released already. It's still open for fixing as part of another issue though.

          Show
          anshumg Anshum Gupta added a comment - Marking this as resolved to avoid confusion. P.S: That only means that this was committed and released already. It's still open for fixing as part of another issue though.
          Hide
          anshumg Anshum Gupta added a comment -

          Right, we can open up the constructor for subclassing but I can't figure the need. I may be missing something here but the Builder could be extended instead of extending the constructor itself and I think that's the right way to go considering we'd be doing away with access to the constructors in 7.0 anyways.

          Show
          anshumg Anshum Gupta added a comment - Right, we can open up the constructor for subclassing but I can't figure the need. I may be missing something here but the Builder could be extended instead of extending the constructor itself and I think that's the right way to go considering we'd be doing away with access to the constructors in 7.0 anyways.
          Hide
          elyograg Shawn Heisey added a comment -

          the Builder could be extended instead of extending the constructor itself

          I tried to extend the client in this way, adding a new setting to the derived client class and exposing it in the derived Builder, but ultimately it comes down to the same problem – the "all parameters" constructor, which is the only one that will survive the transition to 7.0, cannot be used in an extended Client/Builder because it's private.

          One option, which I would not want to employ because it would involve duplicate code that will quickly become stale, is to copy all the code in the private constructor and paste it into the subclass, then add parameters as required and use that constructor in a derived Builder class.

          IMHO, the only sane option for experienced developers is to change the internal constructor from private to protected, allowing derivative classes to utilize it after doing class-specific setup. The developer will usually also need to extend the internal Builder class to expose configuration of any new capability.

          I like what we've done with the Builder, and I agree that after 7.0 removes deprecated code, the constructor should not be public ... but making it private is too limiting.

          Show
          elyograg Shawn Heisey added a comment - the Builder could be extended instead of extending the constructor itself I tried to extend the client in this way, adding a new setting to the derived client class and exposing it in the derived Builder, but ultimately it comes down to the same problem – the "all parameters" constructor, which is the only one that will survive the transition to 7.0, cannot be used in an extended Client/Builder because it's private. One option, which I would not want to employ because it would involve duplicate code that will quickly become stale, is to copy all the code in the private constructor and paste it into the subclass, then add parameters as required and use that constructor in a derived Builder class. IMHO, the only sane option for experienced developers is to change the internal constructor from private to protected, allowing derivative classes to utilize it after doing class-specific setup. The developer will usually also need to extend the internal Builder class to expose configuration of any new capability. I like what we've done with the Builder, and I agree that after 7.0 removes deprecated code, the constructor should not be public ... but making it private is too limiting.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          I'll create a separate issue for changing the appropriate SolrClient ctors to be protected, and will push up a patch shortly.

          I'll summarize the discussion here, and others can weigh in on alternate approaches if they wish.

          Show
          gerlowskija Jason Gerlowski added a comment - I'll create a separate issue for changing the appropriate SolrClient ctors to be protected , and will push up a patch shortly. I'll summarize the discussion here, and others can weigh in on alternate approaches if they wish.
          Hide
          anshumg Anshum Gupta added a comment -

          I can't recollect right now and I'm not looking at the code but I thought there was a way around it. I'll take your word for it here .
          The main thing here was to make sure that the end user uses the builder. Converting the constructor to be protected doesn't defeat that purpose so I'm fine.

          Show
          anshumg Anshum Gupta added a comment - I can't recollect right now and I'm not looking at the code but I thought there was a way around it. I'll take your word for it here . The main thing here was to make sure that the end user uses the builder. Converting the constructor to be protected doesn't defeat that purpose so I'm fine.
          Hide
          anshumg Anshum Gupta added a comment -

          From how I'm looking at this, it's a bug really as it took away a capability that existed in previous releases and this was not a planned move. I think it'd make sense for us to push this with 6.2.1 if we're sure that the only way is to change the scope.

          Show
          anshumg Anshum Gupta added a comment - From how I'm looking at this, it's a bug really as it took away a capability that existed in previous releases and this was not a planned move. I think it'd make sense for us to push this with 6.2.1 if we're sure that the only way is to change the scope.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          I created SOLR-9535 to address the recent concern brought up here, and attached a small patch containing the fix.

          Show
          gerlowskija Jason Gerlowski added a comment - I created SOLR-9535 to address the recent concern brought up here, and attached a small patch containing the fix.

            People

            • Assignee:
              anshumg Anshum Gupta
              Reporter:
              hgadre Hrishikesh Gadre
            • Votes:
              0 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development