HBase
  1. HBase
  2. HBASE-5860

splitlogmanager should not unnecessarily resubmit tasks when zk unavailable

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      (Doesn't really impact the run time or correctness of log splitting)

      say the master has lost connection to zk. splitlogmanager's timeoutmanager will realize that all the tasks that were submitted are still unassigned. It will resubmit those tasks (i.e. create dummy znodes)

      splitlogmanager should realze that the tasks are unassigned but their znodes have not been created.

      012-04-20 13:11:20,516 INFO org.apache.hadoop.hbase.master.SplitLogManager: dead splitlog worker msgstore295.snc4.facebook.com,60020,1334948757026
      2012-04-20 13:11:20,517 DEBUG org.apache.hadoop.hbase.master.SplitLogManager: Scheduling batch of logs to split
      2012-04-20 13:11:20,517 INFO org.apache.hadoop.hbase.master.SplitLogManager: started splitting logs in [hdfs://msgstore215.snc4.facebook.com:9000/MSGSTORE215-SNC4-HBASE/.logs/msgstore295.snc4.facebook.com,60020,1334948757026-splitting]
      2012-04-20 13:11:20,565 INFO org.apache.zookeeper.ClientCnxn: Opening socket connection to server msgstore235.snc4.facebook.com/10.30.222.186:2181
      2012-04-20 13:11:20,566 INFO org.apache.zookeeper.ClientCnxn: Socket connection established to msgstore235.snc4.facebook.com/10.30.222.186:2181, initiating session
      2012-04-20 13:11:20,575 INFO org.apache.hadoop.hbase.master.SplitLogManager: total tasks = 4 unassigned = 4
      2012-04-20 13:11:20,576 DEBUG org.apache.hadoop.hbase.master.SplitLogManager: resubmitting unassigned task(s) after timeout
      2012-04-20 13:11:21,577 DEBUG org.apache.hadoop.hbase.master.SplitLogManager: resubmitting unassigned task(s) after timeout
      2012-04-20 13:11:21,683 INFO org.apache.zookeeper.ClientCnxn: Unable to read additional data from server sessionid 0x36ccb0f8010002, likely server has closed socket, closing socket connection and attempting reconnect
      2012-04-20 13:11:21,683 INFO org.apache.zookeeper.ClientCnxn: Unable to read additional data from server sessionid 0x136ccb0f4890000, likely server has closed socket, closing socket connection and attempting reconnect
      2012-04-20 13:11:21,786 WARN org.apache.hadoop.hbase.master.SplitLogManager$CreateAsyncCallback: create rc =CONNECTIONLOSS for /hbase/splitlog/hdfs%3A%2F%2Fmsgstore215.snc4.facebook.com%3A9000%2FMSGSTORE215-SNC4-HBASE%2F.logs%2Fmsgstore295.snc4.facebook.com%2C60020%2C1334948757026-splitting%2F10.30.251.186%253A60020.1334951586677 retry=3
      2012-04-20 13:11:21,786 WARN org.apache.hadoop.hbase.master.SplitLogManager$CreateAsyncCallback: create rc =CONNECTIONLOSS for /hbase/splitlog/hdfs%3A%2F%2Fmsgstore215.snc4.facebook.com%3A9000%2FMSGSTORE215-SNC4-HBASE%2F.logs%2Fmsgstore295.snc4.facebook.com%2C60020%2C1334948757026-splitting%2F10.30.251.186%253A60020.1334951920332 retry=3

        Activity

        Hide
        Asaf Mesika added a comment -

        We have this message in production after complete cluster shutdown and startup, many many times:

        2014-04-08 05:08:00,083 DEBUG org.apache.hadoop.hbase.master.SplitLogManager: resubmitting unassigned task(s) after timeout
        2014-04-08 05:08:01,077 DEBUG org.apache.hadoop.hbase.master.SplitLogManager: total tasks = 98 unassigned = 98
        20

        Is this related?

        Show
        Asaf Mesika added a comment - We have this message in production after complete cluster shutdown and startup, many many times: 2014-04-08 05:08:00,083 DEBUG org.apache.hadoop.hbase.master.SplitLogManager: resubmitting unassigned task(s) after timeout 2014-04-08 05:08:01,077 DEBUG org.apache.hadoop.hbase.master.SplitLogManager: total tasks = 98 unassigned = 98 20 Is this related?
        Hide
        Jean-Daniel Cryans added a comment -

        The patch doesn't apply to trunk, unmarking as available.

        Show
        Jean-Daniel Cryans added a comment - The patch doesn't apply to trunk, unmarking as available.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12525131/0001-HBASE-5860-splitlogmanager-should-not-unnecessarily-.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 4 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.client.TestShell

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1699//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1699//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1699//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12525131/0001-HBASE-5860-splitlogmanager-should-not-unnecessarily-.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 4 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.client.TestShell Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1699//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1699//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1699//console This message is automatically generated.
        Hide
        Nicolas Spiegelberg added a comment -

        I guess changing the retries to 0 should also fix the HBASE-5890 problem as well? We shouldn't get a NODEEXISTS return for the RESCAN because we create it as EPHEMERAL_SEQUENTIAL.

        Show
        Nicolas Spiegelberg added a comment - I guess changing the retries to 0 should also fix the HBASE-5890 problem as well? We shouldn't get a NODEEXISTS return for the RESCAN because we create it as EPHEMERAL_SEQUENTIAL.
        Hide
        Prakash Khemani added a comment -

        Nicolas's feedback applied.

        also reduced the RESCAN retries to 0.

        Show
        Prakash Khemani added a comment - Nicolas's feedback applied. also reduced the RESCAN retries to 0.
        Hide
        Prakash Khemani added a comment -

        I had missed the fact that isAnyCreateZKNodePending() misses the create of RESCAN nodes. Will provide a fix.

        I was aware of the race condition where isAnyCreateZKNodePending() will return false even when create-zknode is soon going to be retried. Not worth fixing for the reason you outlined - creating an extra RESCAN node doesn't hurt. (The code change you have outlined will need some more changes to make it work)

        Show
        Prakash Khemani added a comment - I had missed the fact that isAnyCreateZKNodePending() misses the create of RESCAN nodes. Will provide a fix. I was aware of the race condition where isAnyCreateZKNodePending() will return false even when create-zknode is soon going to be retried. Not worth fixing for the reason you outlined - creating an extra RESCAN node doesn't hurt. (The code change you have outlined will need some more changes to make it work)
        Hide
        Nicolas Spiegelberg added a comment -

        Also, it looks like there is a race condition in CreateAsyncCallback.processResult. The code is roughly:

        tot_mgr_node_create_result.incrementAndGet();
          if (rc != KeeperException.Code.NODEEXISTS.intValue()) {
            if (retry_count > 0) {
              tot_mgr_node_create_retry.incrementAndGet();
              createNode(path, retry_count - 1);
            }
          }
        

        So, we should change this to:

        try {
          if (rc != KeeperException.Code.NODEEXISTS.intValue()) {
            if (retry_count > 0) {
              tot_mgr_node_create_retry.incrementAndGet();
              createNode(path, retry_count - 1);
            }
          }
        } finally {
          tot_mgr_node_create_result.incrementAndGet();
        }
        

        so we don't mark the znode as responding until we decide if it's a failure and we need to reenqueue. Maybe the repercussions of creating an extra RESCAN node aren't worth finding and fixing all these subtle race conditions?

        Show
        Nicolas Spiegelberg added a comment - Also, it looks like there is a race condition in CreateAsyncCallback.processResult. The code is roughly: tot_mgr_node_create_result.incrementAndGet(); if (rc != KeeperException.Code.NODEEXISTS.intValue()) { if (retry_count > 0) { tot_mgr_node_create_retry.incrementAndGet(); createNode(path, retry_count - 1); } } So, we should change this to: try { if (rc != KeeperException.Code.NODEEXISTS.intValue()) { if (retry_count > 0) { tot_mgr_node_create_retry.incrementAndGet(); createNode(path, retry_count - 1); } } } finally { tot_mgr_node_create_result.incrementAndGet(); } so we don't mark the znode as responding until we decide if it's a failure and we need to reenqueue. Maybe the repercussions of creating an extra RESCAN node aren't worth finding and fixing all these subtle race conditions?
        Hide
        Nicolas Spiegelberg added a comment -

        @Prakash: this code wouldn't pick up that the RESCAN znode was created because that uses createRescanNode() instead of createNode(). Should we not also increment tot_mgr_node_create_queued for createRescanNode() and increment tot_mgr_node_create_result in CreateRescanAsyncCallback.processResult?

        Show
        Nicolas Spiegelberg added a comment - @Prakash: this code wouldn't pick up that the RESCAN znode was created because that uses createRescanNode() instead of createNode(). Should we not also increment tot_mgr_node_create_queued for createRescanNode() and increment tot_mgr_node_create_result in CreateRescanAsyncCallback.processResult?
        Hide
        Jimmy Xiang added a comment -

        Looks good to me.

        Show
        Jimmy Xiang added a comment - Looks good to me.
        Hide
        Nicolas Spiegelberg added a comment -

        +1

        Show
        Nicolas Spiegelberg added a comment - +1
        Hide
        Ted Yu added a comment -

        Patch makes sense.

        +  static boolean isAnyCreateZNodePending() {
        

        This method can be made private, right ?
        Would isAnyZNodeCreationPending be a better name ?

        Show
        Ted Yu added a comment - Patch makes sense. + static boolean isAnyCreateZNodePending() { This method can be made private, right ? Would isAnyZNodeCreationPending be a better name ?
        Hide
        Prakash Khemani added a comment -

        avoid resubmitting tasks to zk when there are pending zkk nodes create.

        Show
        Prakash Khemani added a comment - avoid resubmitting tasks to zk when there are pending zkk nodes create.

          People

          • Assignee:
            Prakash Khemani
            Reporter:
            Prakash Khemani
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:

              Development