Flume
  1. Flume
  2. FLUME-1993

On Windows, when using the spooling directory source, there is a file sharing violation when trying to delete tracker file

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: v1.4.0
    • Fix Version/s: v1.4.0
    • Component/s: Sinks+Sources
    • Labels:
      None

      Description

      spooling directory source
      org.apache.flume.client.avro.ReliableSpoolingFileEventReader.getNextFile()

      Gets an instance of a DurablePositionTracker and is not calling close() on it before then creating another instance... thus not calling close on the underlying DataFileWriter. This left the file locked in Windows and hence any delete attempts were failing...causing the Agent to spin around and around trying to delete this file and creating endless numbers of the temporary tracker files.

      1. FLUME-1993.patch
        0.9 kB
        Paul Chavez
      2. ReliableSpoolingFileEventReader.java
        21 kB
        Phil Scala
      3. ReliableSpoolingFileEventReader.java.patch
        0.8 kB
        Phil Scala

        Issue Links

          Activity

          Phil Scala created issue -
          Hide
          Phil Scala added a comment -

          Attaching my implementation of a minor change to close the DurablePositionTracker, before creating a new instance so that the underlying streams are closed.

          Show
          Phil Scala added a comment - Attaching my implementation of a minor change to close the DurablePositionTracker, before creating a new instance so that the underlying streams are closed.
          Phil Scala made changes -
          Field Original Value New Value
          Attachment ReliableSpoolingFileEventReader.java [ 12578564 ]
          Hide
          Hari Shreedharan added a comment -

          Hi Phil,
          Could you please submit a patch, rather than the file itself? You can get more info here: https://cwiki.apache.org/FLUME/how-to-contribute.html

          Show
          Hari Shreedharan added a comment - Hi Phil, Could you please submit a patch, rather than the file itself? You can get more info here: https://cwiki.apache.org/FLUME/how-to-contribute.html
          Hide
          Phil Scala added a comment -

          Patch attached.

          Show
          Phil Scala added a comment - Patch attached.
          Phil Scala made changes -
          Attachment ReliableSpoolingFileEventReader.jav.patch [ 12578565 ]
          Phil Scala made changes -
          Attachment ReliableSpoolingFileEventReader.jav.patch [ 12578565 ]
          Phil Scala made changes -
          Hide
          Phil Scala added a comment -

          Please see attached patch for changes.

          Show
          Phil Scala added a comment - Please see attached patch for changes.
          Phil Scala made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Israel Ekpo added a comment - - edited

          Phil, in addition to the wiki link from Hari above, you can follow the steps below to create a review for the patch.

          Check first to make sure that it follows the formatting guidelines and recommendations specified in the wiki page.

          I will be updating the wiki page soon with these steps but in the time being, here are the steps for submitting the review for this patch:

          (1) Get the latest code from GIT

          git clone http://git-wip-us.apache.org/repos/asf/flume.git flume

          cd flume

          git checkout flume-1.4

          The current branch for the next release is flume-1.4; this is why we are using "flume-1.4".

          (2) Apply your changes to the code you have checked out

          (3) Try to compile the code changes, create a tarball and run the unit tests to make sure that all is well with your changes

          mvn clean install

          It is very important to test the changes to make sure the work satisfactorily before submitting the patch

          So you can configure the generated tarball and run it to see if it works for you first

          (4) If all is well, then create the patches:

          git diff --no-prefix > /path/to/FLUME-0011.patch

          Use the JIRA issue ID for the patch files you generate.

          If you have more than one pending patch (for different issues) to submit, you can specify the path to the file or directory where the changes are

          For example:

          git diff --no-prefix flume-ng-core/src/main/java/org/apache/flume/serialization/LineDeserializer.java > /path/to/FLUME-0001.patch

          git diff --no-prefix flume-ng-doc/sphinx/Flume* > /path/to/FLUME-0002.patch

          git diff --no-prefix flume-ng-core/src/main/java/org/apache/flume/sink/ > /path/to/FLUME-0003.patch

          (5) Upload the patche file(s) to the JIRA issue and set the status to PATCH AVAILABLE.

          (6) Create a review for your patch files here:

          https://reviews.apache.org/

          Pick "flume-git" as the repository and "Flume" as the group. The current branch is "flume-1.4"

          (7) You can take a look at other code reviews here for some examples:

          https://reviews.apache.org/groups/Flume/

          Thank you for your contribution

          Show
          Israel Ekpo added a comment - - edited Phil, in addition to the wiki link from Hari above, you can follow the steps below to create a review for the patch. Check first to make sure that it follows the formatting guidelines and recommendations specified in the wiki page. I will be updating the wiki page soon with these steps but in the time being, here are the steps for submitting the review for this patch: (1) Get the latest code from GIT git clone http://git-wip-us.apache.org/repos/asf/flume.git flume cd flume git checkout flume-1.4 The current branch for the next release is flume-1.4; this is why we are using "flume-1.4". (2) Apply your changes to the code you have checked out (3) Try to compile the code changes, create a tarball and run the unit tests to make sure that all is well with your changes mvn clean install It is very important to test the changes to make sure the work satisfactorily before submitting the patch So you can configure the generated tarball and run it to see if it works for you first (4) If all is well, then create the patches: git diff --no-prefix > /path/to/ FLUME-0011 .patch Use the JIRA issue ID for the patch files you generate. If you have more than one pending patch (for different issues) to submit, you can specify the path to the file or directory where the changes are For example: git diff --no-prefix flume-ng-core/src/main/java/org/apache/flume/serialization/LineDeserializer.java > /path/to/ FLUME-0001 .patch git diff --no-prefix flume-ng-doc/sphinx/Flume* > /path/to/ FLUME-0002 .patch git diff --no-prefix flume-ng-core/src/main/java/org/apache/flume/sink/ > /path/to/ FLUME-0003 .patch (5) Upload the patche file(s) to the JIRA issue and set the status to PATCH AVAILABLE. (6) Create a review for your patch files here: https://reviews.apache.org/ Pick "flume-git" as the repository and "Flume" as the group. The current branch is "flume-1.4" (7) You can take a look at other code reviews here for some examples: https://reviews.apache.org/groups/Flume/ Thank you for your contribution
          Hide
          Paul Chavez added a comment - - edited

          Attached alternate fix. Instead of explicitly closing the file don't create the new instance unless we deleted the old one. Also check for file existence before attempting delete when getting next instance to appease Windows.

          Note: tried to follow the instructions for creating a code review, but the review web app would not let me attach the patch "The selected file does not appear to be a diff."

          Show
          Paul Chavez added a comment - - edited Attached alternate fix. Instead of explicitly closing the file don't create the new instance unless we deleted the old one. Also check for file existence before attempting delete when getting next instance to appease Windows. Note: tried to follow the instructions for creating a code review, but the review web app would not let me attach the patch "The selected file does not appear to be a diff."
          Paul Chavez made changes -
          Attachment FLUME-1993.patch [ 12578765 ]
          Hide
          Mike Percy added a comment -

          Hi Paul Chavez, I think you got to the bottom of this one. Yep, it's a bug and I agree with your change to ReliableSpoolingFileEventReader.java. However, I am unclear on why you made the change you did to DurablePositionTracker.java. As far as I can tell, we already check at the top of getInstance() whether the file exists, so an additional check should be unnecessary.

          By the way, I had trouble applying your patch as well. I think you have a UTF BOM at the beginning of the file or something because it's messing with all my tools. I was able to manually remove those bytes and eventually apply your patch, but I bet that's why Review Board didn't like it.

          Show
          Mike Percy added a comment - Hi Paul Chavez , I think you got to the bottom of this one. Yep, it's a bug and I agree with your change to ReliableSpoolingFileEventReader.java. However, I am unclear on why you made the change you did to DurablePositionTracker.java. As far as I can tell, we already check at the top of getInstance() whether the file exists, so an additional check should be unnecessary. By the way, I had trouble applying your patch as well. I think you have a UTF BOM at the beginning of the file or something because it's messing with all my tools. I was able to manually remove those bytes and eventually apply your patch, but I bet that's why Review Board didn't like it.
          Mike Percy made changes -
          Assignee Mike Percy [ mpercy ]
          Hide
          Mike Percy added a comment -

          Phil Scala: Please take a look @ Paul Chavez's changes and let us know what you think. Thanks!

          Show
          Mike Percy added a comment - Phil Scala : Please take a look @ Paul Chavez 's changes and let us know what you think. Thanks!
          Hide
          Paul Chavez added a comment -

          Mike, I agree the change to DurablePositionTracker.java is not required. I put that in there thinking a code path called delete on an already deleted file and then later a co-worker found the real issue. I should have backed it out before generating the diff. Thanks for the feedback on the file attach issue, now I think I can suppress the BOM with the windows tools I use.

          Show
          Paul Chavez added a comment - Mike, I agree the change to DurablePositionTracker.java is not required. I put that in there thinking a code path called delete on an already deleted file and then later a co-worker found the real issue. I should have backed it out before generating the diff. Thanks for the feedback on the file attach issue, now I think I can suppress the BOM with the windows tools I use.
          Hide
          Mike Percy added a comment -

          Paul do you want to resubmit your patch with only the desired changes?

          Otherwise I will revert the changes to that file on commit.

          Show
          Mike Percy added a comment - Paul do you want to resubmit your patch with only the desired changes? Otherwise I will revert the changes to that file on commit.
          Hide
          Phil Scala added a comment -

          Paul's fix is cleaner than mine, I have tested it locally and confirm that the spool source is working on my windows 7 machine. My run of the unit tests failed, though I don't believe they ever 100% passed, if Paul Chavez has confirmed the unit tests work, then I am happy.

          Show
          Phil Scala added a comment - Paul's fix is cleaner than mine, I have tested it locally and confirm that the spool source is working on my windows 7 machine. My run of the unit tests failed, though I don't believe they ever 100% passed, if Paul Chavez has confirmed the unit tests work, then I am happy.
          Paul Chavez made changes -
          Attachment FLUME-1993.patch [ 12578765 ]
          Hide
          Paul Chavez added a comment -

          Update patch is attached. I have not attempted to run the unit tests, only did functional testing for this specific issue on Server 2008 R2. Review request submitted: https://reviews.apache.org/r/10607/

          Show
          Paul Chavez added a comment - Update patch is attached. I have not attempted to run the unit tests, only did functional testing for this specific issue on Server 2008 R2. Review request submitted: https://reviews.apache.org/r/10607/
          Paul Chavez made changes -
          Attachment FLUME-1993.patch [ 12579360 ]
          Mike Percy made changes -
          Remote Link This issue links to "Review Board (Web Link)" [ 12157 ]
          Hide
          Mike Percy added a comment -

          Paul Chavez, +1 on your latest patch.

          Phil Scala, thanks for reporting this issue as well as reviewing and testing this patch.

          I think there are a few unit tests (particularly for Exec source) that fail on Windows, so for now since you guys have tested this manually let's go ahead with it.

          Since you worked together on this I'll attribute you both with in the commit. I'm also adding you both as Flume contributors in JIRA, so you can assign JIRA tickets to yourselves now.

          Paul, since we settled on your version of the change, I'm assigning this JIRA to you.

          Show
          Mike Percy added a comment - Paul Chavez , +1 on your latest patch. Phil Scala , thanks for reporting this issue as well as reviewing and testing this patch. I think there are a few unit tests (particularly for Exec source) that fail on Windows, so for now since you guys have tested this manually let's go ahead with it. Since you worked together on this I'll attribute you both with in the commit. I'm also adding you both as Flume contributors in JIRA, so you can assign JIRA tickets to yourselves now. Paul, since we settled on your version of the change, I'm assigning this JIRA to you.
          Mike Percy made changes -
          Assignee Mike Percy [ mpercy ] Paul Chavez [ paulchavez ]
          Hide
          Paul Chavez added a comment -

          Mike, I'm not clear on what further action I should take.

          Show
          Paul Chavez added a comment - Mike, I'm not clear on what further action I should take.
          Hide
          Mike Percy added a comment -

          Ah, you deleted the old one and replaced it. Looks good.

          I just pushed this change to trunk and flume-1.4 branches. Thanks for your contributions Paul and Phil!

          Show
          Mike Percy added a comment - Ah, you deleted the old one and replaced it. Looks good. I just pushed this change to trunk and flume-1.4 branches. Thanks for your contributions Paul and Phil!
          Mike Percy made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s v1.4.0 [ 12323372 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in flume-trunk #396 (See https://builds.apache.org/job/flume-trunk/396/)
          FLUME-1993. Fix spooldir tracker file sharing violation on Windows. (Revision 6662b34c03511be65cdad8ead04860f7b0ab5fa6)

          Result = FAILURE
          mpercy : http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=6662b34c03511be65cdad8ead04860f7b0ab5fa6
          Files :

          • flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java
          Show
          Hudson added a comment - Integrated in flume-trunk #396 (See https://builds.apache.org/job/flume-trunk/396/ ) FLUME-1993 . Fix spooldir tracker file sharing violation on Windows. (Revision 6662b34c03511be65cdad8ead04860f7b0ab5fa6) Result = FAILURE mpercy : http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=6662b34c03511be65cdad8ead04860f7b0ab5fa6 Files : flume-ng-core/src/main/java/org/apache/flume/client/avro/ReliableSpoolingFileEventReader.java

            People

            • Assignee:
              Paul Chavez
              Reporter:
              Phil Scala
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development