Kafka
  1. Kafka
  2. KAFKA-404

When using chroot path, create chroot on startup if it doesn't exist

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.1
    • Fix Version/s: 0.8.2.0
    • Component/s: None
    • Labels:
    • Environment:
      CentOS 5.5, Linux 2.6.18-194.32.1.el5 x86_64 GNU/Linux
    1. KAFKA-404_2014-09-25_13:11:02.patch
      4 kB
      Jonathan Creasy
    2. KAFKA-404.patch
      4 kB
      Jonathan Creasy
    3. KAFKA-404-auto-create-zookeeper-chroot-on-start-up-v5.patch
      4 kB
      Jonathan Creasy
    4. KAFKA-404-auto-create-zookeeper-chroot-on-start-up-v4.patch
      2 kB
      Jonathan Creasy
    5. KAFKA-404-auto-create-zookeeper-chroot-on-start-up-v3.patch
      2 kB
      Marek Dolgos
    6. KAFKA-404-auto-create-zookeeper-chroot-on-start-up-v2.patch
      2 kB
      Marek Dolgos
    7. KAFKA-404-auto-create-zookeeper-chroot-on-start-up-i.patch
      2 kB
      Marek Dolgos
    8. KAFKA-404-0.8.patch
      1 kB
      Jonathan Creasy
    9. KAFKA-404-0.7.1.patch
      1 kB
      Jonathan Creasy

      Issue Links

        Activity

        Hide
        Jay Kreps added a comment -

        Yeah, this would be nice. We might have to parse into the zookeeper url a bit to grab the chroot string-right now we hand those zk connect strings directly to zk-but other than that, should be straight-forward. Patches accepted

        Show
        Jay Kreps added a comment - Yeah, this would be nice. We might have to parse into the zookeeper url a bit to grab the chroot string- right now we hand those zk connect strings directly to zk -but other than that, should be straight-forward. Patches accepted
        Hide
        Jonathan Creasy added a comment -

        Cool, I have "add a patch for this" on my todo list.

        Show
        Jonathan Creasy added a comment - Cool, I have "add a patch for this" on my todo list.
        Hide
        Joel Koshy added a comment -

        Jonathan, thanks for taking this up. Couple of minor comments:
        1 - May want to close the zkclient after creating the namespace.
        2 - Can you use ZkUtils.makeSurePersistentPathExists?
        3 - Use vals instead of the vars
        4 - Can you also provide a patch for 0.8? Trunk and 0.8 are different enough that we need to do double-commits going forward (i.e., until 0.8 is code complete).

        Show
        Joel Koshy added a comment - Jonathan, thanks for taking this up. Couple of minor comments: 1 - May want to close the zkclient after creating the namespace. 2 - Can you use ZkUtils.makeSurePersistentPathExists? 3 - Use vals instead of the vars 4 - Can you also provide a patch for 0.8? Trunk and 0.8 are different enough that we need to do double-commits going forward (i.e., until 0.8 is code complete).
        Hide
        Jonathan Creasy added a comment -

        Yep,I can do those things. I've never written any Scala so I wasn't sure of the best times to use val vs. var.

        Show
        Jonathan Creasy added a comment - Yep,I can do those things. I've never written any Scala so I wasn't sure of the best times to use val vs. var.
        Hide
        Jonathan Creasy added a comment -

        Cancelling this patch, I'll pull 0.8 and make a new patch against that.

        Show
        Jonathan Creasy added a comment - Cancelling this patch, I'll pull 0.8 and make a new patch against that.
        Hide
        Jonathan Creasy added a comment -

        I did some basic testing on each branch I started the server and used a simple consumer and producer to exchange a few messages.

        Show
        Jonathan Creasy added a comment - I did some basic testing on each branch I started the server and used a simple consumer and producer to exchange a few messages.
        Hide
        Joel Koshy added a comment -

        Sorry I didn't get a chance to look at this sooner. Thanks for making the changes. Re: using vals, in this case we don't need any vars and I think it will be clearer if we just use substring instead of splitting zkconnect and reconstructing path out of that. So something like this, but I haven't checked:

        if (zkConnect.contains('/'))
        {
        val sepIndex = zkConnect.indexOf('/')
        val connect = zkConnect.substring(0, sepIndex)
        val path = zkConnect.substring(sepIndex)
        ...

        Show
        Joel Koshy added a comment - Sorry I didn't get a chance to look at this sooner. Thanks for making the changes. Re: using vals, in this case we don't need any vars and I think it will be clearer if we just use substring instead of splitting zkconnect and reconstructing path out of that. So something like this, but I haven't checked: if (zkConnect.contains('/')) { val sepIndex = zkConnect.indexOf('/') val connect = zkConnect.substring(0, sepIndex) val path = zkConnect.substring(sepIndex) ...
        Hide
        Marek Dolgos added a comment -

        Submitting a patch to auto-create the chroot on Kafka server startup if it does not exist.

        Please give feedback, I would be happy to make changes until the patch is acceptable.

        Show
        Marek Dolgos added a comment - Submitting a patch to auto-create the chroot on Kafka server startup if it does not exist. Please give feedback, I would be happy to make changes until the patch is acceptable.
        Hide
        Guozhang Wang added a comment -

        Thanks for the patch, a few comments:

        1. val parts = chroot.split("/").drop(1) // Do you need to drop(1) since you are spliting from chroot, not zkConnect string?

        2. Could you try to reuse ZkUtil.createPersistentPath ?

        3. Could you rename xxTmp to xxForChrootCreation?

        4. info("Created zookeeper path "+path) // Might be better just having one info logging at the end of the loop?

        Guozhang

        Show
        Guozhang Wang added a comment - Thanks for the patch, a few comments: 1. val parts = chroot.split("/").drop(1) // Do you need to drop(1) since you are spliting from chroot, not zkConnect string? 2. Could you try to reuse ZkUtil.createPersistentPath ? 3. Could you rename xxTmp to xxForChrootCreation? 4. info("Created zookeeper path "+path) // Might be better just having one info logging at the end of the loop? Guozhang
        Hide
        Marek Dolgos added a comment - - edited

        @Guozhang Wang

        I've added the changes you requested above. The new patch is: KAFKA-404-auto-create-zookeeper-chroot-on-start-up-v2.patch

        It is patched against the latest in trunk. If there is more that needs to be changed please give more feedback and I'll change it.

        Show
        Marek Dolgos added a comment - - edited @ Guozhang Wang I've added the changes you requested above. The new patch is: KAFKA-404 -auto-create-zookeeper-chroot-on-start-up-v2.patch It is patched against the latest in trunk. If there is more that needs to be changed please give more feedback and I'll change it.
        Hide
        Guozhang Wang added a comment -

        Hi Marek,

        Thanks for the v2 patch. When I am trying ./sbt test to run all unit test, many of them fails due to

        java.net.BindException: Address already in use

        I think that is probably due to the permanently created chroot. Do you re-produce the failures on your end?

        Show
        Guozhang Wang added a comment - Hi Marek, Thanks for the v2 patch. When I am trying ./sbt test to run all unit test, many of them fails due to java.net.BindException: Address already in use I think that is probably due to the permanently created chroot. Do you re-produce the failures on your end?
        Hide
        Marek Dolgos added a comment -

        @Guozhang Wang

        You are right, I have attached v3 of the patch. For me all 214 tests now pass.

        Please try this patch on your side, and tell me if you also have all 214 pass...

        Show
        Marek Dolgos added a comment - @ Guozhang Wang You are right, I have attached v3 of the patch. For me all 214 tests now pass. Please try this patch on your side, and tell me if you also have all 214 pass...
        Hide
        Guozhang Wang added a comment -

        Thanks for v3. A few more comments:

        1. There is one function makeSurePersistentPathExists in ZkUtils, which we can reuse to replace the for-loop-createPersistent code.

        2. With the above function, we can do something like Joel suggested above, as:

        val sepIndex = zkConnect.indexOf('/')

        if (seqIndex > 0)

        { val connect = zkConnect.substring(0, sepIndex) val path = zkConnect.substring(sepIndex) ... // makeSurePersistentPathExists }
        Show
        Guozhang Wang added a comment - Thanks for v3. A few more comments: 1. There is one function makeSurePersistentPathExists in ZkUtils, which we can reuse to replace the for-loop-createPersistent code. 2. With the above function, we can do something like Joel suggested above, as: val sepIndex = zkConnect.indexOf('/') if (seqIndex > 0) { val connect = zkConnect.substring(0, sepIndex) val path = zkConnect.substring(sepIndex) ... // makeSurePersistentPathExists }
        Hide
        Guozhang Wang added a comment -

        Marek Dolgos do you have some time to finish this before 0.8.2 release?

        Show
        Guozhang Wang added a comment - Marek Dolgos do you have some time to finish this before 0.8.2 release?
        Hide
        Jonathan Creasy added a comment -

        Version 4 of the patch, let me know if it needs anything else. Hopefully I made it in time. Thanks Marek Dolgos for your work!

        Show
        Jonathan Creasy added a comment - Version 4 of the patch, let me know if it needs anything else. Hopefully I made it in time. Thanks Marek Dolgos for your work!
        Hide
        Marek Dolgos added a comment -

        Jonathan Creasy

        Thanks for finishing it!

        Show
        Marek Dolgos added a comment - Jonathan Creasy Thanks for finishing it!
        Hide
        Jonathan Creasy added a comment - - edited

        It doesn't really work. Any chroot path breaks the build, it doesn't create the chroot automatically. I'll figure it out tomorrow.

        Show
        Jonathan Creasy added a comment - - edited It doesn't really work. Any chroot path breaks the build, it doesn't create the chroot automatically. I'll figure it out tomorrow.
        Hide
        Neha Narkhede added a comment -

        Thanks for the latest patch. I think there is a bug in that the check for sepIndex happens on the part of the zk connection string that does not have the path starting from /. So the creation logic is unreachable. How about simplifying it to be-

            if (chroot.length > 1) {
              val zkConnForChrootCreation = config.zkConnect.substring(0, config.zkConnect.indexOf("/"))
              val zkClientForChrootCreation = new ZkClient(zkConnForChrootCreation, config.zkSessionTimeoutMs, config.zkConnectionTimeoutMs, ZKStringSerializer)
              ZkUtils.makeSurePersistentPathExists(zkClientForChrootCreation, chroot)
              info("Created zookeeper path " + chroot)
              zkClientForChrootCreation.close()
            }
        
        Show
        Neha Narkhede added a comment - Thanks for the latest patch. I think there is a bug in that the check for sepIndex happens on the part of the zk connection string that does not have the path starting from /. So the creation logic is unreachable. How about simplifying it to be- if (chroot.length > 1) { val zkConnForChrootCreation = config.zkConnect.substring(0, config.zkConnect.indexOf("/")) val zkClientForChrootCreation = new ZkClient(zkConnForChrootCreation, config.zkSessionTimeoutMs, config.zkConnectionTimeoutMs, ZKStringSerializer) ZkUtils.makeSurePersistentPathExists(zkClientForChrootCreation, chroot) info("Created zookeeper path " + chroot) zkClientForChrootCreation.close() }
        Hide
        Jonathan Creasy added a comment -

        I added a unit test for this and implemented your suggested simplification.

        Tests are passing again. I'm going to do one more check where I add the unit test against trunk, see that if fails, apply the patch and see that it then passes.

        Show
        Jonathan Creasy added a comment - I added a unit test for this and implemented your suggested simplification. Tests are passing again. I'm going to do one more check where I add the unit test against trunk, see that if fails, apply the patch and see that it then passes.
        Hide
        Jonathan Creasy added a comment -

        Confirmed, unit test fails until patch is addded, then passes. Also tested with running Kafka and brand new Zookeeper, creates chroot properly.

        Show
        Jonathan Creasy added a comment - Confirmed, unit test fails until patch is addded, then passes. Also tested with running Kafka and brand new Zookeeper, creates chroot properly.
        Hide
        Jonathan Creasy added a comment -

        Created reviewboard https://reviews.apache.org/r/26019/diff/
        against branch trunk

        Show
        Jonathan Creasy added a comment - Created reviewboard https://reviews.apache.org/r/26019/diff/ against branch trunk
        Hide
        Jonathan Creasy added a comment -

        Updated reviewboard https://reviews.apache.org/r/26019/diff/
        against branch trunk

        Show
        Jonathan Creasy added a comment - Updated reviewboard https://reviews.apache.org/r/26019/diff/ against branch trunk
        Hide
        Neha Narkhede added a comment -

        Your suggestion about consolidation is also fine.

        Show
        Neha Narkhede added a comment - Your suggestion about consolidation is also fine.
        Hide
        Jonathan Creasy added a comment -

        RB Updated, let's merge this! (it's only 2 years old...)

        Show
        Jonathan Creasy added a comment - RB Updated, let's merge this! (it's only 2 years old...)
        Hide
        Neha Narkhede added a comment -

        Thanks for the patches, pushed to trunk

        Show
        Neha Narkhede added a comment - Thanks for the patches, pushed to trunk

          People

          • Assignee:
            Jonathan Creasy
            Reporter:
            Jonathan Creasy
            Reviewer:
            Neha Narkhede
          • Votes:
            1 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development