Bookkeeper
  1. Bookkeeper
  2. BOOKKEEPER-234

EntryLogger will throw NPE, if any dir does not exist or IO Errors.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.1.0
    • Fix Version/s: 4.1.0
    • Component/s: bookkeeper-server
    • Labels:
      None

      Description

      I think, Entry log should check the exitance of directories, before going to get the last Log entries. Because, listFiles will retunr null on non existant directories or any IO Errors. We have to add the check for directory existnace check and null checks in side getLastLogID api in EntryLogger.

      We may need to handle in LedgerCacheImpl also.

      Do we need to add them in bad disks list? and others will refer this list before they use the dirs.

      1. BOOKKEEPER-234.patch
        2 kB
        Uma Maheswara Rao G
      2. BOOKKEEPER-234.patch
        3 kB
        Uma Maheswara Rao G

        Activity

        Hide
        Uma Maheswara Rao G added a comment -

        Attached the patch, which addresses the issue in EntryLogger.

        Show
        Uma Maheswara Rao G added a comment - Attached the patch, which addresses the issue in EntryLogger.
        Hide
        Sijie Guo added a comment -

        lgtm +1

        Show
        Sijie Guo added a comment - lgtm +1
        Hide
        Uma Maheswara Rao G added a comment -

        Thanks a lot, Sijie for the review!

        Show
        Uma Maheswara Rao G added a comment - Thanks a lot, Sijie for the review!
        Hide
        Ivan Kelly added a comment -

        Where are you seeing the error occurring? There's a check when Bookie is created already to ensure that the directories exist.

        Show
        Ivan Kelly added a comment - Where are you seeing the error occurring? There's a check when Bookie is created already to ensure that the directories exist.
        Hide
        Uma Maheswara Rao G added a comment -

        Yep, Ivan, I have seen it in checkEnvironment call. If directory does not exist then it is creating it.

        Yes, In my testing environment it happened on the the startup of BK, unfortunately dirs have been cleaned(through some test scripts) . I am assuming that, after this step(checkEnvironment) only that might have happend. So, I directly verified that EntryLogger code and found that peice for NPE possibility. Do we need to throw the exception if we meet that situation, as we may not have Version files and all right? Now I can see this is very very rare case. Do we need to handle it?

        Show
        Uma Maheswara Rao G added a comment - Yep, Ivan, I have seen it in checkEnvironment call. If directory does not exist then it is creating it. Yes, In my testing environment it happened on the the startup of BK, unfortunately dirs have been cleaned(through some test scripts) . I am assuming that, after this step(checkEnvironment) only that might have happend. So, I directly verified that EntryLogger code and found that peice for NPE possibility. Do we need to throw the exception if we meet that situation, as we may not have Version files and all right? Now I can see this is very very rare case. Do we need to handle it?
        Hide
        Ivan Kelly added a comment -

        I think this is a very exceptional scenario (where the directory is deleted, after checkEnvironment and while the bookie is running). We shouldn't allow the bookie to continue as normal if an admin is doing this. That said NPEs are ugly. It would be better if the entry logger checked for the existence of the directory, and if it doesn't exist, throw a FileNotFoundException.

        Show
        Ivan Kelly added a comment - I think this is a very exceptional scenario (where the directory is deleted, after checkEnvironment and while the bookie is running). We shouldn't allow the bookie to continue as normal if an admin is doing this. That said NPEs are ugly. It would be better if the entry logger checked for the existence of the directory, and if it doesn't exist, throw a FileNotFoundException.
        Hide
        Uma Maheswara Rao G added a comment -

        true...Let me update the patch with this change in some time.

        Show
        Uma Maheswara Rao G added a comment - true...Let me update the patch with this change in some time.
        Hide
        Uma Maheswara Rao G added a comment -

        Attached the patch as discussed.

        Show
        Uma Maheswara Rao G added a comment - Attached the patch as discussed.
        Hide
        Ivan Kelly added a comment -

        Committed as r1335996. Thanks Uma.

        Show
        Ivan Kelly added a comment - Committed as r1335996. Thanks Uma.
        Hide
        Hudson added a comment -

        Integrated in bookkeeper-trunk #498 (See https://builds.apache.org/job/bookkeeper-trunk/498/)
        BOOKKEEPER-234: EntryLogger will throw NPE, if any dir does not exist or IO Errors. (umamaheswararao via ivank) (Revision 1335996)

        Result = UNSTABLE
        ivank :
        Files :

        • /zookeeper/bookkeeper/trunk/CHANGES.txt
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java
        Show
        Hudson added a comment - Integrated in bookkeeper-trunk #498 (See https://builds.apache.org/job/bookkeeper-trunk/498/ ) BOOKKEEPER-234 : EntryLogger will throw NPE, if any dir does not exist or IO Errors. (umamaheswararao via ivank) (Revision 1335996) Result = UNSTABLE ivank : Files : /zookeeper/bookkeeper/trunk/CHANGES.txt /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java

          People

          • Assignee:
            Uma Maheswara Rao G
            Reporter:
            Uma Maheswara Rao G
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development