Details

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

      Windows

      Description

      Trace:
      java.io.IOException: Unable to delete file: C:\Users\uma\AppData\Local\Temp\bookie7010257841258186070test\current\136fcc63892.txn
      at org.apache.commons.io.FileUtils.forceDelete(FileUtils.java:1919)
      at org.apache.commons.io.FileUtils.cleanDirectory(FileUtils.java:1399)
      at org.apache.commons.io.FileUtils.deleteDirectory(FileUtils.java:1331)
      at org.apache.commons.io.FileUtils.forceDelete(FileUtils.java:1910)
      at org.apache.commons.io.FileUtils.cleanDirectory(FileUtils.java:1399)
      at org.apache.commons.io.FileUtils.deleteDirectory(FileUtils.java:1331)
      at org.apache.bookkeeper.test.BookKeeperClusterTestCase.stopBKCluster(BookKeeperClusterTestCase.java:150)
      at org.apache.bookkeeper.test.BookKeeperClusterTestCase.tearDown(BookKeeperClusterTestCase.java:94)
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
      at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
      at java.lang.reflect.Method.invoke(Unknown Source)
      at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
      at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
      at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)

      1. BOOKKEEPER-232.patch
        8 kB
        Uma Maheswara Rao G
      2. BOOKKEEPER-232.patch
        6 kB
        Uma Maheswara Rao G
      3. BOOKKEEPER-232.patch
        4 kB
        Uma Maheswara Rao G

        Activity

        Hide
        Ivan Kelly added a comment -

        Submitted to r1335117. Thanks Uma

        Show
        Ivan Kelly added a comment - Submitted to r1335117. Thanks Uma
        Hide
        Ivan Kelly added a comment -

        lgtm +1. Don't worry about the rename of LedgerCache yet. It needs to be done when the patch queue is small because otherwise it'll probably hit a conflict. We should do before 4.1 though.

        Show
        Ivan Kelly added a comment - lgtm +1. Don't worry about the rename of LedgerCache yet. It needs to be done when the patch queue is small because otherwise it'll probably hit a conflict. We should do before 4.1 though.
        Hide
        Flavio Junqueira added a comment -

        +1, looks good. Ivan?

        Show
        Flavio Junqueira added a comment - +1, looks good. Ivan?
        Hide
        Uma Maheswara Rao G added a comment -

        Hi Ivan, Nice point about LedgerCache naming. I will leave it for other JIRA to rename it as you mentioned.( if it is ok for you, you can directly assign that issue to me, i will upload patch). As discussed, now LedgerCache extends Closeable and just implemented close method. Also formatted correctly as per BK guidelines.

        If you have time, please take a look.

        Show
        Uma Maheswara Rao G added a comment - Hi Ivan, Nice point about LedgerCache naming. I will leave it for other JIRA to rename it as you mentioned.( if it is ok for you, you can directly assign that issue to me, i will upload patch). As discussed, now LedgerCache extends Closeable and just implemented close method. Also formatted correctly as per BK guidelines. If you have time, please take a look.
        Hide
        Ivan Kelly added a comment -

        Another issue with the patch is that it leaks internal implementation detail of LedgerCache. Basically, LedgerCache should not be called that. It should be called LedgerIndex as all methods on the interface have to do with indexing. In fact, ill open a JIRA to rename it after this comment. The problem is that clearCache tells the user of the API that it maybe a cache there. I think it would be much better to simply call this method "close()" and implement the Closeable interface. This way the interface is kept a lot cleaner.

        Show
        Ivan Kelly added a comment - Another issue with the patch is that it leaks internal implementation detail of LedgerCache. Basically, LedgerCache should not be called that. It should be called LedgerIndex as all methods on the interface have to do with indexing. In fact, ill open a JIRA to rename it after this comment. The problem is that clearCache tells the user of the API that it maybe a cache there. I think it would be much better to simply call this method "close()" and implement the Closeable interface. This way the interface is kept a lot cleaner.
        Hide
        Uma Maheswara Rao G added a comment -

        Yes, Flavio, I forgot to include IOUtils class in the patch...my bad. I just ran the tests in my env. Did not re apply the patch and check. Let me re-generate the patch. Also will correct formatting issues. Thanks for taking a look.

        Show
        Uma Maheswara Rao G added a comment - Yes, Flavio, I forgot to include IOUtils class in the patch...my bad. I just ran the tests in my env. Did not re apply the patch and check. Let me re-generate the patch. Also will correct formatting issues. Thanks for taking a look.
        Hide
        Flavio Junqueira added a comment -

        It doesn't compile for me, it looks like import org.apache.bookkeeper.util.IOUtils is missing. The patch also contains tabs. Just in case you haven't seen it yet, please have a look at the format we have been using here:

        https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute

        Show
        Flavio Junqueira added a comment - It doesn't compile for me, it looks like import org.apache.bookkeeper.util.IOUtils is missing. The patch also contains tabs. Just in case you haven't seen it yet, please have a look at the format we have been using here: https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
        Hide
        Flavio Junqueira added a comment -

        Looking into it...

        Show
        Flavio Junqueira added a comment - Looking into it...
        Hide
        Uma Maheswara Rao G added a comment -

        @Flavio, Do you have time to review it?. So, that our tests will pass in windows ( our dev env).

        Show
        Uma Maheswara Rao G added a comment - @Flavio, Do you have time to review it?. So, that our tests will pass in windows ( our dev env).
        Hide
        Uma Maheswara Rao G added a comment -

        Attached the patch with IOUtils.close api.

        Show
        Uma Maheswara Rao G added a comment - Attached the patch with IOUtils.close api.
        Hide
        Uma Maheswara Rao G added a comment -

        Yep, Rakesh, I will bring IOUtils cleanup methods here. That would handle this checks.

        Show
        Uma Maheswara Rao G added a comment - Yep, Rakesh, I will bring IOUtils cleanup methods here. That would handle this checks.
        Hide
        Rakesh R added a comment -

        I feel, before closing the streams, it would be good to have null checks to avoid unwanted exceptions.

        Show
        Rakesh R added a comment - I feel, before closing the streams, it would be good to have null checks to avoid unwanted exceptions.
        Hide
        Uma Maheswara Rao G added a comment -

        Closing streams in finally would have a bad properties. It will mask the root exceptions. Let me try to bring IOUtils clean up with null checks and close the streams in side try itself and retry on finally with IOUtils. for example: HDFS-2330

        Show
        Uma Maheswara Rao G added a comment - Closing streams in finally would have a bad properties. It will mask the root exceptions. Let me try to bring IOUtils clean up with null checks and close the streams in side try itself and retry on finally with IOUtils. for example: HDFS-2330
        Hide
        Uma Maheswara Rao G added a comment -

        2012-04-29 17:01:11,382 - INFO - [main:FinalRequestProcessor@423] - shutdown of
        request processor complete
        2012-04-29 17:01:11,383 - INFO - [main:ClientBase@227] - connecting to localhos
        t 2181
        Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 50.091 sec

        Results :

        Tests run: 14, Failures: 0, Errors: 0, Skipped: 0

        [INFO] ------------------------------------------------------------------------
        [INFO] BUILD SUCCESS
        [INFO] ------------------------------------------------------------------------
        [INFO] Total time: 55.195s
        [INFO] Finished at: Sun Apr 29 17:01:12 IST 2012
        [INFO] Final Memory: 7M/17M
        [INFO] ------------------------------------------------------------------------

        Show
        Uma Maheswara Rao G added a comment - 2012-04-29 17:01:11,382 - INFO - [main:FinalRequestProcessor@423] - shutdown of request processor complete 2012-04-29 17:01:11,383 - INFO - [main:ClientBase@227] - connecting to localhos t 2181 Tests run: 14, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 50.091 sec Results : Tests run: 14, Failures: 0, Errors: 0, Skipped: 0 [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 55.195s [INFO] Finished at: Sun Apr 29 17:01:12 IST 2012 [INFO] Final Memory: 7M/17M [INFO] ------------------------------------------------------------------------
        Hide
        Uma Maheswara Rao G added a comment -

        Attached the patch. with this fix, all async tests passing.

        Show
        Uma Maheswara Rao G added a comment - Attached the patch. with this fix, all async tests passing.
        Hide
        Uma Maheswara Rao G added a comment -

        I just analysed the cause for this failures.
        After stopping bk, we are not closing the EntryLogger and LedgerCache. Seems like ledger cache and entryLogger holding the locks on that files (*.txn, *.idx files).
        If we clear the cache and entrylogger can help in this tests to pass.

        Show
        Uma Maheswara Rao G added a comment - I just analysed the cause for this failures. After stopping bk, we are not closing the EntryLogger and LedgerCache. Seems like ledger cache and entryLogger holding the locks on that files (*.txn, *.idx files). If we clear the cache and entrylogger can help in this tests to pass.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development