ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1301

backport patches related to the zk startup script from 3.4 to 3.3 release

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.4
    • Fix Version/s: 3.3.4
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. zookeeper-1301-2.patch
      7 kB
      Giridharan Kesavan
    2. zookeeper-1301-1.patch
      7 kB
      Giridharan Kesavan
    3. zookeeper-1301.patch
      7 kB
      Giridharan Kesavan

      Activity

      Hide
      Patrick Hunt added a comment -

      committed to branch 3.3, Thanks Giri!

      Show
      Patrick Hunt added a comment - committed to branch 3.3, Thanks Giri!
      Hide
      Patrick Hunt added a comment -

      +1 looks good to me as well.

      Show
      Patrick Hunt added a comment - +1 looks good to me as well.
      Hide
      Mahadev konar added a comment -

      looks good. +1 on the patch.

      Show
      Mahadev konar added a comment - looks good. +1 on the patch.
      Hide
      Giridharan Kesavan added a comment -

      revised patch which uses partial patches from zookeeper-905 and zookeeper-983 to address 2 & 4

      Show
      Giridharan Kesavan added a comment - revised patch which uses partial patches from zookeeper-905 and zookeeper-983 to address 2 & 4
      Hide
      Patrick Hunt added a comment -

      sounds good.

      Show
      Patrick Hunt added a comment - sounds good.
      Hide
      Mahadev konar added a comment -

      Looking at the patch, I think we should do this:

      3) looks fine to me (giri can you just add an echo statement as roman mentioned)

      1) giri already fixed.

      2) lets revert

      4) lets revert

      Show
      Mahadev konar added a comment - Looking at the patch, I think we should do this: 3) looks fine to me (giri can you just add an echo statement as roman mentioned) 1) giri already fixed. 2) lets revert 4) lets revert
      Hide
      Patrick Hunt added a comment -

      Here's my suggestion. Giri lmk if this makes sense for you.

      1) update this patch to drop 2/3/4 changes
      2) create new jira(s) for 2/3/4 against 3.3.5/3.4.1/3.5.0(trunk) in order to get those fixed/addressed in later versions.

      Giri can you do 1 & 2? Subsequently i'll review this patch for commit in 3.3.4

      Thanks!

      Show
      Patrick Hunt added a comment - Here's my suggestion. Giri lmk if this makes sense for you. 1) update this patch to drop 2/3/4 changes 2) create new jira(s) for 2/3/4 against 3.3.5/3.4.1/3.5.0(trunk) in order to get those fixed/addressed in later versions. Giri can you do 1 & 2? Subsequently i'll review this patch for commit in 3.3.4 Thanks!
      Hide
      Giridharan Kesavan added a comment -

      update patch to change the snapshot dir to point to /tmp/zookeeper

      Show
      Giridharan Kesavan added a comment - update patch to change the snapshot dir to point to /tmp/zookeeper
      Hide
      Eric Yang added a comment -

      1) +1 on datadir=/tmp

      2) readlink -f causes to follow symlink recursively. It is not supported on Mac. If we are just doing back port, we should follow it strictly without making more variants that can potentially lose track. We can be sure that both 3.4 and 3.3.4 would behave the same for 1 level symlink.

      3) The patch is right, it will use user input. Why show user input again?

      4) Again, it's strict porting. Any corner case should be addressed in trunk then backport to avoid fragmented branches.

      Show
      Eric Yang added a comment - 1) +1 on datadir=/tmp 2) readlink -f causes to follow symlink recursively. It is not supported on Mac. If we are just doing back port, we should follow it strictly without making more variants that can potentially lose track. We can be sure that both 3.4 and 3.3.4 would behave the same for 1 level symlink. 3) The patch is right, it will use user input. Why show user input again? 4) Again, it's strict porting. Any corner case should be addressed in trunk then backport to avoid fragmented branches.
      Hide
      Patrick Hunt added a comment -

      1) Let's change datadir like we did in 3.4/trunk (tmp).

      2) I don't understand this change, it's for mac but seems to break other things? (see ZOOKEEPER-905)

      Is there a better way to work around this on mac?

      Can we drop this change from the patch?

      3) ?

      4) ?

      Show
      Patrick Hunt added a comment - 1) Let's change datadir like we did in 3.4/trunk (tmp). 2) I don't understand this change, it's for mac but seems to break other things? (see ZOOKEEPER-905 ) Is there a better way to work around this on mac? Can we drop this change from the patch? 3) ? 4) ?
      Hide
      Roman Shaposhnik added a comment -

      I have a couple of concerns regarding the attached code

      1. This has to be fixed:

      -dataDir=/export/crawlspace/mahadev/zookeeper/server1/data
      +dataDir=/home/tdunning/tmp
      

      2. I'm not sure why -f was dropped (in fact it might actually break some ZK deployments)

      -if readlink -f "$0" > /dev/null 2>&1
      +if readlink "$0" > /dev/null 2>&1
      

      3. I'm really nervous about changing user requests like this without at least a diagnostic message:

      +# if we give a more complicated path to the config, don't screw around in $ZOOCFGDIR
      +if [ "x`dirname $ZOOCFG`" != "x$ZOOCFGDIR" ]
      +then
      +    ZOOCFG="$2"
      +fi
      +
      

      4. I'm not sure I understand the logic of creating ZOO_DATADIR ONLY when ZOOPIDFILE is not defined. In fact,
      I a production deployment any attempt at mkdir $ZOO_DATADIR/mkdir -p $(dirname "$ZOOPIDFILE") is very likely
      to fail and hence these at least need to be handled gracefully.

      +if [ -z $ZOOPIDFILE ]; then
      +    ZOO_DATADIR=$(grep "^[[:space:]]*dataDir" "$ZOOCFG" | sed -e 's/.*=//')
      +    if [ ! -d "$ZOO_DATADIR" ]; then
      +        mkdir -p "$ZOO_DATADIR"
      +    fi
      +    ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
      +else
      +    # ensure it exists, otw stop will fail
      +    mkdir -p $(dirname "$ZOOPIDFILE")
      +fi
      
      Show
      Roman Shaposhnik added a comment - I have a couple of concerns regarding the attached code 1. This has to be fixed: -dataDir=/export/crawlspace/mahadev/zookeeper/server1/data +dataDir=/home/tdunning/tmp 2. I'm not sure why -f was dropped (in fact it might actually break some ZK deployments) -if readlink -f "$0" > /dev/null 2>&1 +if readlink "$0" > /dev/null 2>&1 3. I'm really nervous about changing user requests like this without at least a diagnostic message: +# if we give a more complicated path to the config, don't screw around in $ZOOCFGDIR +if [ "x`dirname $ZOOCFG`" != "x$ZOOCFGDIR" ] +then + ZOOCFG="$2" +fi + 4. I'm not sure I understand the logic of creating ZOO_DATADIR ONLY when ZOOPIDFILE is not defined. In fact, I a production deployment any attempt at mkdir $ZOO_DATADIR/mkdir -p $(dirname "$ZOOPIDFILE") is very likely to fail and hence these at least need to be handled gracefully. +if [ -z $ZOOPIDFILE ]; then + ZOO_DATADIR=$(grep "^[[:space:]]*dataDir" "$ZOOCFG" | sed -e 's/.*=//') + if [ ! -d "$ZOO_DATADIR" ]; then + mkdir -p "$ZOO_DATADIR" + fi + ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid" +else + # ensure it exists, otw stop will fail + mkdir -p $(dirname "$ZOOPIDFILE") +fi
      Hide
      Giridharan Kesavan added a comment -

      patch comprises all the patches listed in the previous comment.

      Show
      Giridharan Kesavan added a comment - patch comprises all the patches listed in the previous comment.
      Hide
      Giridharan Kesavan added a comment - - edited

      this requires applying/back-porting the following list of patches from 3.4 to 3.3 branch

      ZOOKEEPER-905
      ZOOKEEPER-796
      ZOOKEEPER-983
      ZOOKEEPER-976
      ZOOKEEPER-1013
      ZOOKEEPER-1012
      ZOOKEEPER-1061
      ZOOKEEPER-1074
      ZOOKEEPER-1119

      I would upload one single patch if that make things easier..

      Show
      Giridharan Kesavan added a comment - - edited this requires applying/back-porting the following list of patches from 3.4 to 3.3 branch ZOOKEEPER-905 ZOOKEEPER-796 ZOOKEEPER-983 ZOOKEEPER-976 ZOOKEEPER-1013 ZOOKEEPER-1012 ZOOKEEPER-1061 ZOOKEEPER-1074 ZOOKEEPER-1119 I would upload one single patch if that make things easier..

        People

        • Assignee:
          Giridharan Kesavan
          Reporter:
          Giridharan Kesavan
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development