Kafka
  1. Kafka
  2. KAFKA-404

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

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.8.1
    • Fix Version/s: 0.8.2
    • Component/s: None
    • Labels:
    • Environment:
      CentOS 5.5, Linux 2.6.18-194.32.1.el5 x86_64 GNU/Linux

      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 }

        People

        • Assignee:
          Unassigned
          Reporter:
          Jonathan Creasy
        • Votes:
          0 Vote for this issue
          Watchers:
          5 Start watching this issue

          Dates

          • Created:
            Updated:

            Development