Solr
  1. Solr
  2. SOLR-5852

Add CloudSolrServer helper method to connect to a ZK ensemble

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      We should have a CloudSolrServer constructor which takes a list of ZK servers to connect to.

      Something Like

      public CloudSolrServer(String... zkHost);
      
      • Document the current constructor better to mention that to connect to a ZK ensemble you can pass a comma-delimited list of ZK servers like
        zk1:2181,zk2:2181,zk3:2181
      • Thirdly should getLbServer() and getZKStatereader() be public?
      1. SOLR-5852_FK.patch
        3 kB
        Furkan KAMACI
      2. SOLR-5852_FK.patch
        2 kB
        Furkan KAMACI
      3. SOLR-5852.patch
        10 kB
        Shalin Shekhar Mangar
      4. SOLR-5852.patch
        11 kB
        Shalin Shekhar Mangar
      5. SOLR-5852.patch
        13 kB
        Varun Thacker
      6. SOLR-5852.patch
        16 kB
        Shawn Heisey
      7. SOLR-5852.patch
        16 kB
        Shawn Heisey
      8. SOLR-5852.patch
        2 kB
        Varun Thacker
      9. SOLR-5852-SH.patch
        19 kB
        Shawn Heisey
      10. SOLR-5852-SH.patch
        18 kB
        Shawn Heisey

        Activity

        Hide
        Varun Thacker added a comment -

        Simple Patch.

        • Adds Javadocs to the current constructor to detail on how to connect to a ZK ensemble
        • Adds another constructor which takes an list of servers and converts them into a comma separated list of servers.
        Show
        Varun Thacker added a comment - Simple Patch. Adds Javadocs to the current constructor to detail on how to connect to a ZK ensemble Adds another constructor which takes an list of servers and converts them into a comma separated list of servers.
        Hide
        Furkan KAMACI added a comment - - edited

        Varun Thacker I think that getLbServer() may be restricted (if errors are resolved at test classes) but getZKStatereader() can be used to access cluster state for client APIs.

        Show
        Furkan KAMACI added a comment - - edited Varun Thacker I think that getLbServer() may be restricted (if errors are resolved at test classes) but getZKStatereader() can be used to access cluster state for client APIs.
        Hide
        Furkan KAMACI added a comment -

        Varun Thacker I've improved your patched and attached. You can check it.

        Show
        Furkan KAMACI added a comment - Varun Thacker I've improved your patched and attached. You can check it.
        Hide
        Shawn Heisey added a comment -

        Varun Thacker has proposed that we include the javadoc changes for SOLR-4620 on this issue. Before proceeding with this plan, I would like to hear from committers with more seniority as to whether this change is a good idea.

        My own gut reaction is that we should just stick with the format already provided by zookeeper, especially since that's the only format that Solr itself accepts when starting SolrCloud.

        The current patch does not allow you to specify a chroot. You could check the starting character and assume it's a chroot if it starts with a forward slash. Unless you limited this check to the last argument, you'd need to throw an exception if there were multiple chroots found and they were not all identical.

        When I looked into whether it was possible to have two methods, here's what I found: Eclipse does allow me to have foo(String,String...) as well as foo(String...). This surprised me, because Java would likely ignore one of them. Makes me wonder whether this is an oversight in the JDK, Eclipse, or both. It wasn't even flagged by a FindBugs run.

        Show
        Shawn Heisey added a comment - Varun Thacker has proposed that we include the javadoc changes for SOLR-4620 on this issue. Before proceeding with this plan, I would like to hear from committers with more seniority as to whether this change is a good idea. My own gut reaction is that we should just stick with the format already provided by zookeeper, especially since that's the only format that Solr itself accepts when starting SolrCloud. The current patch does not allow you to specify a chroot. You could check the starting character and assume it's a chroot if it starts with a forward slash. Unless you limited this check to the last argument, you'd need to throw an exception if there were multiple chroots found and they were not all identical. When I looked into whether it was possible to have two methods, here's what I found: Eclipse does allow me to have foo(String,String...) as well as foo(String...). This surprised me, because Java would likely ignore one of them. Makes me wonder whether this is an oversight in the JDK, Eclipse, or both. It wasn't even flagged by a FindBugs run.
        Hide
        Shawn Heisey added a comment -

        Scratch what I just said about Eclipse allowing multiple ambiguous methods. It actually does flag the problem, but it does it when you try to USE the method, not at the actual method definitions.

        Show
        Shawn Heisey added a comment - Scratch what I just said about Eclipse allowing multiple ambiguous methods. It actually does flag the problem, but it does it when you try to USE the method, not at the actual method definitions.
        Hide
        Furkan KAMACI added a comment -

        Shawn Heisey I'will improve the patch as you said.

        Show
        Furkan KAMACI added a comment - Shawn Heisey I'will improve the patch as you said.
        Hide
        Furkan KAMACI added a comment -

        Shawn Heisey ConnectStringParser at Zookeeper checks chroot and other invalid situations. We can give that checking responsibility to Zookeeper. If anything changes within Zookeeper check condition our CloudSolrServer will not be affected from it because we will pass that check to Zookeeper and it will handle it.

        I think that we can handle chroot with current situation too. Zookeeper.java works like that: 127.0.0.1:3000,127.0.0.1:3001,127.0.0.1:3002/app/a so I can improve the javadoc and include that: if there is a chroot add it to the end of the last host string (this is how original Zookeeper code works). All in all if anybody sends multiple chroot definitions or anything else Zookeeper will return an error.

        Another approach is accepting like that: 127.0.0.1:3000/app/a,127.0.0.1:3001/app/a,127.0.0.1:3002/app/a so parsing if there any chroot and valid for all hosts etc.

        Show
        Furkan KAMACI added a comment - Shawn Heisey ConnectStringParser at Zookeeper checks chroot and other invalid situations. We can give that checking responsibility to Zookeeper. If anything changes within Zookeeper check condition our CloudSolrServer will not be affected from it because we will pass that check to Zookeeper and it will handle it. I think that we can handle chroot with current situation too. Zookeeper.java works like that: 127.0.0.1:3000,127.0.0.1:3001,127.0.0.1:3002/app/a so I can improve the javadoc and include that: if there is a chroot add it to the end of the last host string (this is how original Zookeeper code works). All in all if anybody sends multiple chroot definitions or anything else Zookeeper will return an error. Another approach is accepting like that: 127.0.0.1:3000/app/a,127.0.0.1:3001/app/a,127.0.0.1:3002/app/a so parsing if there any chroot and valid for all hosts etc.
        Hide
        Furkan KAMACI added a comment -

        I've improved the javadoc. We can use whether SOLR-4620 or this. On the other hand I can implement another patch according to second approach at my previous comment.

        Show
        Furkan KAMACI added a comment - I've improved the javadoc. We can use whether SOLR-4620 or this. On the other hand I can implement another patch according to second approach at my previous comment.
        Hide
        Shawn Heisey added a comment -

        The patches already submitted didn't have the kind of flexibility and error reporting that I hoped for. I've built a new patch for this issue. This patch also addresses SOLR-4620, eliminates all but one warning in CloudSolrServer, and includes tests for the new constructor.

        There are probably at least two additional test cases that need to be created.

        Show
        Shawn Heisey added a comment - The patches already submitted didn't have the kind of flexibility and error reporting that I hoped for. I've built a new patch for this issue. This patch also addresses SOLR-4620 , eliminates all but one warning in CloudSolrServer, and includes tests for the new constructor. There are probably at least two additional test cases that need to be created.
        Hide
        Furkan KAMACI added a comment -

        Shawn Heisey Actually I tried to mention different approaches at my comment I and attached a patch for first style. User can pass chroot as a parameter at constructor and so there will be no need to pass chroot at the end of ever Host:Port pair and need to check it at constructor.

        Show
        Furkan KAMACI added a comment - Shawn Heisey Actually I tried to mention different approaches at my comment I and attached a patch for first style. User can pass chroot as a parameter at constructor and so there will be no need to pass chroot at the end of ever Host:Port pair and need to check it at constructor.
        Hide
        Erick Erickson added a comment -

        Hey Shawn:

        I tried to apply your patch to a new checkout for trunk and had merge conflicts. It looks like the test code changed. Could you regenerate the patch?

        Thanks!

        Show
        Erick Erickson added a comment - Hey Shawn: I tried to apply your patch to a new checkout for trunk and had merge conflicts. It looks like the test code changed. Could you regenerate the patch? Thanks!
        Hide
        Shawn Heisey added a comment -

        I just made that patch a few hours ago!

        New patch coming up as soon as I work my way through the conflicts.

        Show
        Shawn Heisey added a comment - I just made that patch a few hours ago! New patch coming up as soon as I work my way through the conflicts.
        Hide
        Erick Erickson added a comment -

        Yeah, I saw that. Was it against trunk or 4x? That might account for the difference.

        BTW, I thought I'd mention that sleep is a good thing .

        Show
        Erick Erickson added a comment - Yeah, I saw that. Was it against trunk or 4x? That might account for the difference. BTW, I thought I'd mention that sleep is a good thing .
        Hide
        Shawn Heisey added a comment -

        New patch against trunk. Previous patch was against trunk too, but a couple of hours after I went to bed, a conflicting patch was committed.

        This does make a change to CloudSolrServerTest bits that just got added, but only to eliminate warnings. It does not change the function.

        Show
        Shawn Heisey added a comment - New patch against trunk. Previous patch was against trunk too, but a couple of hours after I went to bed, a conflicting patch was committed. This does make a change to CloudSolrServerTest bits that just got added, but only to eliminate warnings. It does not change the function.
        Hide
        Erick Erickson added a comment -

        As I read this, I don't quite see the utility of offering all the different ways
        of specifying the ensemble.

        1> ("host1:2181", "/mychroot")
        2> ("127.0.0.1:3000", "127.0.0.1:3001", "127.0.0.1:3002")
        3> ("localhost:2181", "localhost:2181", "localhost:2181/solrtwo")
        4> ("zoo1:2181", "zoo2:2181", "zoo3:2181", "/solr-three")
        5> ("zoo1.example.com:2181", "zoo2.example.com:2181","zoo3.example/com:2181","/solr-three")
        6> ("zoo1:2181/root", "zoo2:2181/root", "zoo3:2181/root")

        Aren't these all handled by the "typical" ZK ensemble connection string? I.e.
        1> "host1:2101/mychroot"
        2> "127.0.0.1:3000,127.0.0.1:3001,127.0.0.1:3002"
        3> "localhost:2181,localhost:2181,localhost:2181/solrtwo"
        4> like 3
        5> like 3
        6> like 3

        I confess I'm just looking at it from a rather ignorant level, but it seems
        like this would add complexity for no added functionality. Of course I may be
        missing a lot, if there are places where this kind of processing is
        already being done and this moves things to a c'tor that would
        be a reason.

        I'd rather have a single form than multiple forms, unless the multiple
        forms give me added functionality. Otherwise, one adds maintenance
        without adding value..

        Let me know if I've missed the boat here.

        Show
        Erick Erickson added a comment - As I read this, I don't quite see the utility of offering all the different ways of specifying the ensemble. 1> ("host1:2181", "/mychroot") 2> ("127.0.0.1:3000", "127.0.0.1:3001", "127.0.0.1:3002") 3> ("localhost:2181", "localhost:2181", "localhost:2181/solrtwo") 4> ("zoo1:2181", "zoo2:2181", "zoo3:2181", "/solr-three") 5> ("zoo1.example.com:2181", "zoo2.example.com:2181","zoo3.example/com:2181","/solr-three") 6> ("zoo1:2181/root", "zoo2:2181/root", "zoo3:2181/root") Aren't these all handled by the "typical" ZK ensemble connection string? I.e. 1> "host1:2101/mychroot" 2> "127.0.0.1:3000,127.0.0.1:3001,127.0.0.1:3002" 3> "localhost:2181,localhost:2181,localhost:2181/solrtwo" 4> like 3 5> like 3 6> like 3 I confess I'm just looking at it from a rather ignorant level, but it seems like this would add complexity for no added functionality. Of course I may be missing a lot, if there are places where this kind of processing is already being done and this moves things to a c'tor that would be a reason. I'd rather have a single form than multiple forms, unless the multiple forms give me added functionality. Otherwise, one adds maintenance without adding value.. Let me know if I've missed the boat here.
        Hide
        Furkan KAMACI added a comment -

        Erick Erickson could you check my comments and my patch?

        Show
        Furkan KAMACI added a comment - Erick Erickson could you check my comments and my patch?
        Hide
        Shawn Heisey added a comment -

        As I read this, I don't quite see the utility of offering all the different ways of specifying the ensemble.

        Aren't these all handled by the "typical" ZK ensemble connection string?

        I actually agree. But if a method like this is created, that's how I would want to do it.

        Show
        Shawn Heisey added a comment - As I read this, I don't quite see the utility of offering all the different ways of specifying the ensemble. Aren't these all handled by the "typical" ZK ensemble connection string? I actually agree. But if a method like this is created, that's how I would want to do it.
        Hide
        Shawn Heisey added a comment -

        The inclusion of my patch should not be taken as an endorsement on this issue. I had these ideas floating around in my head that wanted to be put into actual code, so I acquiesced and wrote it.

        I don't believe we need this at all. If there's consensus that disagrees, then I think it requires the robustness that I put into my patch.

        Show
        Shawn Heisey added a comment - The inclusion of my patch should not be taken as an endorsement on this issue. I had these ideas floating around in my head that wanted to be put into actual code, so I acquiesced and wrote it. I don't believe we need this at all. If there's consensus that disagrees, then I think it requires the robustness that I put into my patch.
        Hide
        Mark Miller added a comment -

        We should have a CloudSolrServer constructor which takes a list of ZK servers to connect to.

        +1

        Document the current constructor better to mention that to connect to a ZK ensemble you can pass a comma-delimited list of ZK servers like zk1:2181,zk2:2181,zk3:2181

        +1

        Thirdly should getLbServer() and getZKStatereader() be public?

        I think so - we could mark them @lucene.experimental though. Or really, we just need an expert tag.

        Show
        Mark Miller added a comment - We should have a CloudSolrServer constructor which takes a list of ZK servers to connect to. +1 Document the current constructor better to mention that to connect to a ZK ensemble you can pass a comma-delimited list of ZK servers like zk1:2181,zk2:2181,zk3:2181 +1 Thirdly should getLbServer() and getZKStatereader() be public? I think so - we could mark them @lucene.experimental though. Or really, we just need an expert tag.
        Hide
        Erick Erickson added a comment -

        Mark Miller Hmmm, see my comment 20-Mar at 13:09. Do you really think that supporting all those variants is worthwhile when they can all be satisfied via the current code?

        Or are you just saying that we need to doc that the current connection string would handle all those case?

        Show
        Erick Erickson added a comment - Mark Miller Hmmm, see my comment 20-Mar at 13:09. Do you really think that supporting all those variants is worthwhile when they can all be satisfied via the current code? Or are you just saying that we need to doc that the current connection string would handle all those case?
        Hide
        Mark Miller added a comment -

        I think having a list option is nice.

        Personally, I'd probably look at implementing it as something like (String chroot, List<String> hosts) though.

        Show
        Mark Miller added a comment - I think having a list option is nice. Personally, I'd probably look at implementing it as something like (String chroot, List<String> hosts) though.
        Hide
        Erick Erickson added a comment -

        Yeah, I can live with that 'cause it doesn't require regexes, or really any complex logic.

        Something like this?
        if (! isGoodRoot(chroot)) throw error
        if (! allHostsWellFormed(hosts)) throw an error

        this.zkHost = StringUtils.join(hosts, ',') + normalized(chroot);

        Show
        Erick Erickson added a comment - Yeah, I can live with that 'cause it doesn't require regexes, or really any complex logic. Something like this? if (! isGoodRoot(chroot)) throw error if (! allHostsWellFormed(hosts)) throw an error this.zkHost = StringUtils.join(hosts, ',') + normalized(chroot);
        Hide
        Shawn Heisey added a comment -

        Sometimes you can't see the forest because you're so busy looking at trees. That is an awesome method signature!

        Show
        Shawn Heisey added a comment - Sometimes you can't see the forest because you're so busy looking at trees. That is an awesome method signature!
        Hide
        Varun Thacker added a comment -

        Personally, I'd probably look at implementing it as something like (String chroot, List<String> hosts) though.

        This looks like a perfect solution.

        Show
        Varun Thacker added a comment - Personally, I'd probably look at implementing it as something like (String chroot, List<String> hosts) though. This looks like a perfect solution.
        Hide
        Furkan KAMACI added a comment -

        Yeah, I can live with that 'cause it doesn't require regexes, or really any complex logic.

        I think so. I tried to explain same idea that we should pass it to Zookeeper class. Because if any logic changes within Zookeeper class we have to change our constructor too.

        (String chroot, List<String> hosts)

        this is a nice signature for us.

        It is not important but: (List<String> hosts, String chroot) maybe good because of chroot is not mandatory and it seems me more human readeble not to pass a null value at first paramater. Also there may be a signature as like (List<String> hosts) too for whom does not use a chroot parameter.

        Show
        Furkan KAMACI added a comment - Yeah, I can live with that 'cause it doesn't require regexes, or really any complex logic. I think so. I tried to explain same idea that we should pass it to Zookeeper class. Because if any logic changes within Zookeeper class we have to change our constructor too. (String chroot, List<String> hosts) this is a nice signature for us. It is not important but: (List<String> hosts, String chroot) maybe good because of chroot is not mandatory and it seems me more human readeble not to pass a null value at first paramater. Also there may be a signature as like (List<String> hosts) too for whom does not use a chroot parameter.
        Hide
        Shawn Heisey added a comment -

        New patch. Will throw IAE if the chroot doesn't start with a forward slash, but no other error checking. It uses Furkan's order change and Collection in the signature.

        Show
        Shawn Heisey added a comment - New patch. Will throw IAE if the chroot doesn't start with a forward slash, but no other error checking. It uses Furkan's order change and Collection in the signature.
        Hide
        Shawn Heisey added a comment -

        Forgot to change the parameter order in the javadocs. Fixed.

        Show
        Shawn Heisey added a comment - Forgot to change the parameter order in the javadocs. Fixed.
        Hide
        Varun Thacker added a comment -
        • Updated Shawn Heisey's patch to trunk
        • Modified CloudSolrServerMultiConstructorTest to make the tests random.
        Show
        Varun Thacker added a comment - Updated Shawn Heisey 's patch to trunk Modified CloudSolrServerMultiConstructorTest to make the tests random.
        Hide
        Shalin Shekhar Mangar added a comment -

        Updating Varun's patch to trunk.

        My +1 to commit.

        Show
        Shalin Shekhar Mangar added a comment - Updating Varun's patch to trunk. My +1 to commit.
        Hide
        Shalin Shekhar Mangar added a comment -

        My last patch had messed up javadocs because of a bad merge. This patch fixes it.

        Show
        Shalin Shekhar Mangar added a comment - My last patch had messed up javadocs because of a bad merge. This patch fixes it.
        Hide
        ASF subversion and git services added a comment -

        Commit 1631409 from shalin@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1631409 ]

        SOLR-5852: Add CloudSolrServer helper method to connect to a ZK ensemble

        Show
        ASF subversion and git services added a comment - Commit 1631409 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1631409 ] SOLR-5852 : Add CloudSolrServer helper method to connect to a ZK ensemble
        Hide
        ASF subversion and git services added a comment -

        Commit 1631411 from shalin@apache.org in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1631411 ]

        SOLR-5852: Add CloudSolrServer helper method to connect to a ZK ensemble

        Show
        ASF subversion and git services added a comment - Commit 1631411 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1631411 ] SOLR-5852 : Add CloudSolrServer helper method to connect to a ZK ensemble
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks everyone!

        Show
        Shalin Shekhar Mangar added a comment - Thanks everyone!
        Hide
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Varun Thacker
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development