Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Not A Problem
    • Affects Version/s: 0.20.204.0
    • Fix Version/s: None
    • Component/s: tasktracker
    • Labels:
      None

      Description

      Per HADOOP-7551 the TT does not remove bad mapred.local.dirs from the conf so after a single disk failure every call to get a local path for reading or writing results in a disk check of all configured local dirs. After detecting that a local dir is bad we should remove it from the conf so that we don't repeatedly perform this expensive operation.

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          What's the method to re-trigger a check, eg if the disk is onlined again? Restart the TT, I guess?

          Show
          Todd Lipcon added a comment - What's the method to re-trigger a check, eg if the disk is onlined again? Restart the TT, I guess?
          Hide
          Ravi Gummadi added a comment -

          I too have that concern of "TT not handling the case of 'bad disks becoming good'".

          BTW, How expensive is this check in DiskChecker.checkDirs() ? This is not being called for bad local dirs in the current code of TT.offerService(). LocalDirAllocator object being held by TaskController is also updated by TT.initialize(). We need to see if we can update all LocalDirAllocator objects with good local dirs list, if we are not doing that already. Will it be good enough here and for HADOOP-7551 ?

          Show
          Ravi Gummadi added a comment - I too have that concern of "TT not handling the case of 'bad disks becoming good'". BTW, How expensive is this check in DiskChecker.checkDirs() ? This is not being called for bad local dirs in the current code of TT.offerService(). LocalDirAllocator object being held by TaskController is also updated by TT.initialize(). We need to see if we can update all LocalDirAllocator objects with good local dirs list, if we are not doing that already. Will it be good enough here and for HADOOP-7551 ?
          Hide
          Eli Collins added a comment -

          @Todd - Yes, to re-trigger you need to restart the TT. This is how the code currently works - once a directory is removed from LocalStorage's "good list" it is never put back while the TT is running, ie once a dir is identified as bad it won't be used by the TT. LocalDirAllocator#confChanged tries to notice when a new dir is added to the conf but we don't add new MR local dirs at runtime so this feature isn't used. Per HADOOP-7551 LocalDirAllocator (common) and LocalStorage (mr) are currently independent but should be aware of each other.

          @Ravi LocalDirAllocator already keeps track of the valid dirs itself. Once there is a bad dir LocalDirAllocator#confChanged executes for every call to get a local directory, it's this code that calls checkDirs on each local directory. It turns out the version of checkDirs that doesn't take a permissions parameter is not as expensive as I thought (the method that takes a permission forks a call to ls for each directory which is expensive). However confChanged creates a new DF object for each local dir which has the side effect of resetting the df interval which means forking a call to df instead of caching the last result when LocalDirAllocator uses each DF.

          In short, I think it's expensive if the configured dirs are different from the list of valid dirs maintained by LocalDirAllocator. If we remove bad dirs from the conf in the TT then they won't differ. Alternatively, we could modify LocalDirAllocator to ignore bad directories but that would conflict with its current design that explicitly tries to notice a difference between the set of valid and configured dirs.

          Show
          Eli Collins added a comment - @Todd - Yes, to re-trigger you need to restart the TT. This is how the code currently works - once a directory is removed from LocalStorage's "good list" it is never put back while the TT is running, ie once a dir is identified as bad it won't be used by the TT. LocalDirAllocator#confChanged tries to notice when a new dir is added to the conf but we don't add new MR local dirs at runtime so this feature isn't used. Per HADOOP-7551 LocalDirAllocator (common) and LocalStorage (mr) are currently independent but should be aware of each other. @Ravi LocalDirAllocator already keeps track of the valid dirs itself. Once there is a bad dir LocalDirAllocator#confChanged executes for every call to get a local directory, it's this code that calls checkDirs on each local directory. It turns out the version of checkDirs that doesn't take a permissions parameter is not as expensive as I thought (the method that takes a permission forks a call to ls for each directory which is expensive). However confChanged creates a new DF object for each local dir which has the side effect of resetting the df interval which means forking a call to df instead of caching the last result when LocalDirAllocator uses each DF. In short, I think it's expensive if the configured dirs are different from the list of valid dirs maintained by LocalDirAllocator. If we remove bad dirs from the conf in the TT then they won't differ. Alternatively, we could modify LocalDirAllocator to ignore bad directories but that would conflict with its current design that explicitly tries to notice a difference between the set of valid and configured dirs.
          Hide
          Ravi Gummadi added a comment -

          LocalDirAllocator#AllocatorPerContext#confChanged() is actually updating the savedLocalDirs everytime conf is changed. So every call to confChanged() is not resulting into disk checks (except once per configuration change i.e. once per every bad disk). Right ?

          Show
          Ravi Gummadi added a comment - LocalDirAllocator#AllocatorPerContext#confChanged() is actually updating the savedLocalDirs everytime conf is changed. So every call to confChanged() is not resulting into disk checks (except once per configuration change i.e. once per every bad disk). Right ?
          Hide
          Eli Collins added a comment -

          Updating savedLocalDirs is the reason why it runs through the disk checking code each time. The disk checking is done when newLocalDirs != savedLocalDirs, and because saveLocalDirs has the bad dirs removed and newLocalDirs is retrieved from the conf (and per this jira we don't modify the conf) we will run the disk checking code every time confChanged is called. Make sense?

          Show
          Eli Collins added a comment - Updating savedLocalDirs is the reason why it runs through the disk checking code each time. The disk checking is done when newLocalDirs != savedLocalDirs, and because saveLocalDirs has the bad dirs removed and newLocalDirs is retrieved from the conf (and per this jira we don't modify the conf) we will run the disk checking code every time confChanged is called. Make sense?
          Hide
          Ravi Gummadi added a comment -

          AFAIK TT is modifying the conf object(s) based on disks' health. If you see some cases where we are missing updating conf, we can fix them too. Fine ?

          Show
          Ravi Gummadi added a comment - AFAIK TT is modifying the conf object(s) based on disks' health. If you see some cases where we are missing updating conf, we can fix them too. Fine ?
          Hide
          Eli Collins added a comment -

          You're right, I missed that we're always updating the conf the job is launched with the latest known good dirs in TT#launchTaskForJob. Thanks for the explanation. I verified we weren't missing other locations by failing a local dir, logging in the confChanged case and verifying that we only notice the change once.

          Show
          Eli Collins added a comment - You're right, I missed that we're always updating the conf the job is launched with the latest known good dirs in TT#launchTaskForJob. Thanks for the explanation. I verified we weren't missing other locations by failing a local dir, logging in the confChanged case and verifying that we only notice the change once.

            People

            • Assignee:
              Eli Collins
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development