Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-14233

Delay construction of PreCondition.check failure message in Configuration#set

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-alpha4, 2.8.2
    • Component/s: None
    • Labels:
      None

      Description

      The String in the precondition check is constructed prior to failure detection. Since the normal case is no error, we can gain performance by delaying the construction of the string until the failure is detected.

      1. HADOOP-14233.1.patch
        0.9 kB
        Jonathan Eagles

        Activity

        Hide
        jeagles Jonathan Eagles added a comment -

        My local benchmarking shows a roughly %15 performance gain for Configuration.set after applying this patch.

        Show
        jeagles Jonathan Eagles added a comment - My local benchmarking shows a roughly %15 performance gain for Configuration.set after applying this patch.
        Hide
        hanishakoneru Hanisha Koneru added a comment -

        Thank you Jonathan Eagles. The patch LGTM.

        It would be good to follow this practice for logging as well. Passing concatenated strings into a logging method can also incur a needless performance hit because the concatenation will be performed every time the method is called, whether or not the log level is set low enough to show the message.

        Show
        hanishakoneru Hanisha Koneru added a comment - Thank you Jonathan Eagles . The patch LGTM. It would be good to follow this practice for logging as well. Passing concatenated strings into a logging method can also incur a needless performance hit because the concatenation will be performed every time the method is called, whether or not the log level is set low enough to show the message.
        Hide
        stevel@apache.org Steve Loughran added a comment -

        +1

        regarding the suggested logging improvements, every @ debug has traditionally been guarded by an if (LOG.isDebugEnabled(); we've been slowly moving to SLF4J on a file-by-file, package-by-package basis, which offers better dynamic message construction. We could do more to speed that migration up.

        Show
        stevel@apache.org Steve Loughran added a comment - +1 regarding the suggested logging improvements, every @ debug has traditionally been guarded by an if (LOG.isDebugEnabled() ; we've been slowly moving to SLF4J on a file-by-file, package-by-package basis, which offers better dynamic message construction. We could do more to speed that migration up.
        Hide
        jeagles Jonathan Eagles added a comment -

        Thanks Steve Loughran and Hanisha Koneru for the reviews. Committed this trivial patch to trunk, branch-2, and branch-2.8

        Show
        jeagles Jonathan Eagles added a comment - Thanks Steve Loughran and Hanisha Koneru for the reviews. Committed this trivial patch to trunk, branch-2, and branch-2.8
        Hide
        andrew.wang Andrew Wang added a comment -

        Little reminder, please include the JIRA ID in the first component of the commit message.

        Show
        andrew.wang Andrew Wang added a comment - Little reminder, please include the JIRA ID in the first component of the commit message.
        Hide
        stevel@apache.org Steve Loughran added a comment -

        Andrew Wang should we have a policy here these days, like revert-and-reapply if the commit message is missing a JIRA or has the wrong one?

        Show
        stevel@apache.org Steve Loughran added a comment - Andrew Wang should we have a policy here these days, like revert-and-reapply if the commit message is missing a JIRA or has the wrong one ?
        Hide
        jeagles Jonathan Eagles added a comment -

        Good catch on the commit message. Let me know if you want me to perform the corrective revert-and-reapply.

        Show
        jeagles Jonathan Eagles added a comment - Good catch on the commit message. Let me know if you want me to perform the corrective revert-and-reapply.
        Hide
        stevel@apache.org Steve Loughran added a comment -

        I'm not worried, though its caused me confusion in the past, with git grep not finding a patch in a branch, as I was looking for the JIRA.

        Show
        stevel@apache.org Steve Loughran added a comment - I'm not worried, though its caused me confusion in the past, with git grep not finding a patch in a branch, as I was looking for the JIRA.
        Hide
        andrew.wang Andrew Wang added a comment -

        I don't think it's worth having a revert-and-reapply policy, my comment was for people who are also doing grepping on git log. A revert-and-reapply policy won't work unless there's someone or something enforcing this. My (forlorn) hope is that one day we'll solve this by using Gerrit to automate the commit and have a precommit check on the commit message.

        FWIW I maintain a list of commits->JIRAs fixups for my versions script here, which is what prompted this comment: https://github.com/umbrant/versions/blob/master/metadata/3.0.0-alpha3.yaml

        Show
        andrew.wang Andrew Wang added a comment - I don't think it's worth having a revert-and-reapply policy, my comment was for people who are also doing grepping on git log. A revert-and-reapply policy won't work unless there's someone or something enforcing this. My (forlorn) hope is that one day we'll solve this by using Gerrit to automate the commit and have a precommit check on the commit message. FWIW I maintain a list of commits->JIRAs fixups for my versions script here, which is what prompted this comment: https://github.com/umbrant/versions/blob/master/metadata/3.0.0-alpha3.yaml
        Hide
        stevel@apache.org Steve Loughran added a comment - - edited

        BTW, this is causing problems linking against old guava versions; the relevant method in Guava is 20.0+; any app with an older guava version on their CP is going to see a stack trace here.

        I'm wondering if, short term, I can change it to do the check/string construct ourselves

        Show
        stevel@apache.org Steve Loughran added a comment - - edited BTW, this is causing problems linking against old guava versions; the relevant method in Guava is 20.0+; any app with an older guava version on their CP is going to see a stack trace here. I'm wondering if, short term, I can change it to do the check/string construct ourselves
        Hide
        vinodkv Vinod Kumar Vavilapalli added a comment -

        2.8.1 became a security release. Moving fix-version to 2.8.2 after the fact.

        Show
        vinodkv Vinod Kumar Vavilapalli added a comment - 2.8.1 became a security release. Moving fix-version to 2.8.2 after the fact.

          People

          • Assignee:
            jeagles Jonathan Eagles
            Reporter:
            jeagles Jonathan Eagles
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development