Hadoop Common
  1. Hadoop Common
  2. HADOOP-5721

Provide EditLogFileInputStream and EditLogFileOutputStream as independent classes

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.19.1
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      EditLogFileInputStream and EditLogFileOutputStream are currently part of FSEditLog. With this patch we want to extract them and provide as independent classes.

      1. HADOOP-5721.patch
        21 kB
        Konstantin Shvachko
      2. HADOOP-5721.patch
        21 kB
        Luca Telloli
      3. HADOOP-5721.patch
        21 kB
        Flavio Junqueira
      4. HADOOP-5721.patch
        20 kB
        Flavio Junqueira
      5. HADOOP-5721.patch
        83 kB
        Luca Telloli
      6. HADOOP-5721.patch
        81 kB
        Flavio Junqueira
      7. HADOOP-5721.patch
        20 kB
        Flavio Junqueira
      8. HADOOP-5721.patch
        17 kB
        Flavio Junqueira
      9. HADOOP-5721.patch
        17 kB
        Luca Telloli
      10. HADOOP-5721.patch
        10 kB
        Luca Telloli
      11. HADOOP-5721.patch
        16 kB
        Luca Telloli

        Issue Links

          Activity

          Hide
          Luca Telloli added a comment -

          Patch attached

          Show
          Luca Telloli added a comment - Patch attached
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12406145/HADOOP-5721.patch
          against trunk revision 768073.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/237/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/237/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/237/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/237/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12406145/HADOOP-5721.patch against trunk revision 768073. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/237/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/237/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/237/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/237/console This message is automatically generated.
          Hide
          Luca Telloli added a comment -
          • Fixed unit test issues
          • Fixed 'protected' keyword
          • No need for a unit test since no feature has been added or modified
          Show
          Luca Telloli added a comment - Fixed unit test issues Fixed 'protected' keyword No need for a unit test since no feature has been added or modified
          Hide
          Luca Telloli added a comment -

          Latest version fixes missing file

          Show
          Luca Telloli added a comment - Latest version fixes missing file
          Hide
          Konstantin Shvachko added a comment -
          1. Several includes in FSEditLog became redundant after the input and output streams have been factored out.
            import java.io.FileInputStream;
            import java.io.FileOutputStream;
            import java.io.RandomAccessFile;
            import java.nio.ByteBuffer;
            import java.nio.channels.FileChannel;
            import org.apache.hadoop.hdfs.server.namenode.EditLogFileInputStream;
            import org.apache.hadoop.io.DataOutputBuffer;
            
          2. Could you please move the import of EditLogFileOutputStream down in the respective group of imports. The imports are lexicographically ordered in the file.
          3. FSEditLog.sizeFlushBuffer should be a private static field of EditLogFileOutputStream together with FSEditLog.setBufferCapacity() method.

          Please resubmit the patch (cancel and then submit again) so that Hudson could run it again.

          Show
          Konstantin Shvachko added a comment - Several includes in FSEditLog became redundant after the input and output streams have been factored out. import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.RandomAccessFile; import java.nio.ByteBuffer; import java.nio.channels.FileChannel; import org.apache.hadoop.hdfs.server.namenode.EditLogFileInputStream; import org.apache.hadoop.io.DataOutputBuffer; Could you please move the import of EditLogFileOutputStream down in the respective group of imports. The imports are lexicographically ordered in the file. FSEditLog.sizeFlushBuffer should be a private static field of EditLogFileOutputStream together with FSEditLog.setBufferCapacity() method. Please resubmit the patch (cancel and then submit again) so that Hudson could run it again.
          Hide
          Flavio Junqueira added a comment -

          Submitting a new patch that fixes the issues pointed out by Konstantin.

          Please note that I have made a couple of small modifications to sizeFlushBuffer. I think they make its use cleaner, and I don't think it breaks anything, but please verify. Here are the modifications and my rationale. First, I have made it a constant because I don't think we should be modifying that value during an execution of the namenode. Second, as we only need the size of the buffer when constructing a new EditLogFileOutputStream, if we need to have a different buffer size, then we could simply pass the new size as a parameter of the constructor. So, I have added a simple constructor that takes a second parameter, which is the buffer size. This way we can change the size of the buffer, and yet have a default value.

          Show
          Flavio Junqueira added a comment - Submitting a new patch that fixes the issues pointed out by Konstantin. Please note that I have made a couple of small modifications to sizeFlushBuffer. I think they make its use cleaner, and I don't think it breaks anything, but please verify. Here are the modifications and my rationale. First, I have made it a constant because I don't think we should be modifying that value during an execution of the namenode. Second, as we only need the size of the buffer when constructing a new EditLogFileOutputStream, if we need to have a different buffer size, then we could simply pass the new size as a parameter of the constructor. So, I have added a simple constructor that takes a second parameter, which is the buffer size. This way we can change the size of the buffer, and yet have a default value.
          Hide
          Luca Telloli added a comment -

          Canceling for resubmission

          Show
          Luca Telloli added a comment - Canceling for resubmission
          Hide
          Flavio Junqueira added a comment -

          Submitting patch so that QA runs it.

          Show
          Flavio Junqueira added a comment - Submitting patch so that QA runs it.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12406434/HADOOP-5721.patch
          against trunk revision 768376.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/248/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/248/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/248/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/248/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12406434/HADOOP-5721.patch against trunk revision 768376. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/248/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/248/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/248/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/248/console This message is automatically generated.
          Hide
          Luca Telloli added a comment -

          cancelled for resubmission

          Show
          Luca Telloli added a comment - cancelled for resubmission
          Hide
          Flavio Junqueira added a comment - - edited

          There was a compilation problem with one of the tests. I have implemented some modifications to FSEditLog.java to comply with the previous use of the interface of FSEditLog and with the modifications of this patch. More specifically, I have added variants of the methods open, addNewEditLogStream and createEditLogFile that take the buffer size as a parameter.

          Core tests pass fine for me.

          Show
          Flavio Junqueira added a comment - - edited There was a compilation problem with one of the tests. I have implemented some modifications to FSEditLog.java to comply with the previous use of the interface of FSEditLog and with the modifications of this patch. More specifically, I have added variants of the methods open, addNewEditLogStream and createEditLogFile that take the buffer size as a parameter. Core tests pass fine for me.
          Hide
          Konstantin Shvachko added a comment -
          1. In FSEditLog methods open(), addNewEditLogStream(), createEditLogFile() should not be public. Since you are changing them could you please also make them package private.
          2. Long lines in new methods, should be 80 chars.
          3. Indentation should be 2, not 4 like in addNewEditLogStream(), createEditLogFile() and EditLogFileOutputStream(File name).
          Show
          Konstantin Shvachko added a comment - In FSEditLog methods open(), addNewEditLogStream(), createEditLogFile() should not be public. Since you are changing them could you please also make them package private. Long lines in new methods, should be 80 chars. Indentation should be 2, not 4 like in addNewEditLogStream(), createEditLogFile() and EditLogFileOutputStream(File name).
          Hide
          Flavio Junqueira added a comment -

          Thanks for more comments, Konstantin. I have implemented your suggestions and also some other modifications due to some observations that Luca made offline. Luca pointed out that the way open() was implemented in the previous version of the patch, it was not very flexible and it would prevent us from having open methods to other types of log devices because it was taking a parameter specific to a file.

          I have instead moved the parameter that configures the output buffer size to FSEditLog (FSEditLog::sizeOutputFlushBuffer), and now the calls in FSEditLog that currently create an output log stream (EditLogFileOutputStream) use FSEditLog::sizeOutputFlushBuffer when calling the constructor of EditLogFileOutputStream. To set the value of FSEditLog::sizeOutputFlushBuffer, there is a method called setOutputFlushBuffer(int). Note that FSEditLog::sizeOutputFlushBuffer is not static as before. I prefer to have an instance attribute instead of a static one to avoid confusion when instantiating multiple times the same class.

          I also think that FSEditLog is currently the right place to keep the configuration of log devices because FSEditoLog contains calls to start and stop such devices. However, I would like to propose a different solution for configuring log devices (not for this patch, though). We should have a class, called perhaps EditLogDevConfig, that holds all the parameters necessary for the log devices a particular instance of the namenode requires. When calling the constructor of a particular log stream device, we just pass such a configuration parameter and the constructor takes care of extracting the necessary parameters.

          Show
          Flavio Junqueira added a comment - Thanks for more comments, Konstantin. I have implemented your suggestions and also some other modifications due to some observations that Luca made offline. Luca pointed out that the way open() was implemented in the previous version of the patch, it was not very flexible and it would prevent us from having open methods to other types of log devices because it was taking a parameter specific to a file. I have instead moved the parameter that configures the output buffer size to FSEditLog (FSEditLog::sizeOutputFlushBuffer), and now the calls in FSEditLog that currently create an output log stream (EditLogFileOutputStream) use FSEditLog::sizeOutputFlushBuffer when calling the constructor of EditLogFileOutputStream. To set the value of FSEditLog::sizeOutputFlushBuffer, there is a method called setOutputFlushBuffer(int). Note that FSEditLog::sizeOutputFlushBuffer is not static as before. I prefer to have an instance attribute instead of a static one to avoid confusion when instantiating multiple times the same class. I also think that FSEditLog is currently the right place to keep the configuration of log devices because FSEditoLog contains calls to start and stop such devices. However, I would like to propose a different solution for configuring log devices (not for this patch, though). We should have a class, called perhaps EditLogDevConfig, that holds all the parameters necessary for the log devices a particular instance of the namenode requires. When calling the constructor of a particular log stream device, we just pass such a configuration parameter and the constructor takes care of extracting the necessary parameters.
          Hide
          Luca Telloli added a comment -

          +1 overall

          works good for me; the only drawback is the amount of whitespace that changed due to eclipse reformatting. Since Konstantin asked for is he can confirm if it's ok.

          Additionally, I think that your proposal of a configuration class might be part of HADOOP-5188 or even have its own patch.

          Show
          Luca Telloli added a comment - +1 overall works good for me; the only drawback is the amount of whitespace that changed due to eclipse reformatting. Since Konstantin asked for is he can confirm if it's ok. Additionally, I think that your proposal of a configuration class might be part of HADOOP-5188 or even have its own patch.
          Hide
          Luca Telloli added a comment -

          submitting for QA

          Show
          Luca Telloli added a comment - submitting for QA
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12406617/HADOOP-5721.patch
          against trunk revision 769643.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          -1 release audit. The applied patch generated 481 release audit warnings (more than the trunk's current 480 warnings).

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/artifact/trunk/current/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12406617/HADOOP-5721.patch against trunk revision 769643. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. -1 release audit. The applied patch generated 481 release audit warnings (more than the trunk's current 480 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/artifact/trunk/current/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/258/console This message is automatically generated.
          Hide
          Luca Telloli added a comment -

          This patch should address the synchronization issue reported by findbugs. The issue is not real since the involved method (divertFileStream) is called only by another method which is already synchronized, but the sychronized keyword has been added anyway.

          Additionally the siteOutputFlushBuffer has been declared as volatile. This should not have any side effect as well.

          Show
          Luca Telloli added a comment - This patch should address the synchronization issue reported by findbugs. The issue is not real since the involved method (divertFileStream) is called only by another method which is already synchronized, but the sychronized keyword has been added anyway. Additionally the siteOutputFlushBuffer has been declared as volatile. This should not have any side effect as well.
          Hide
          Konstantin Shvachko added a comment -

          Now I really can not see what has changed, and what hasn't.
          This was intended as a refactoring patch, not a reformatting patch.
          What I was "asking", as Luca puts it, is to make newly introduced code follow the common code formatting requirements, which it didn't.
          New features like configuration of log devices proposed by Flavio should also be discussed in another jira.
          http://wiki.apache.org/hadoop/HowToContribute

          Show
          Konstantin Shvachko added a comment - Now I really can not see what has changed, and what hasn't. This was intended as a refactoring patch, not a reformatting patch. What I was "asking", as Luca puts it, is to make newly introduced code follow the common code formatting requirements, which it didn't. New features like configuration of log devices proposed by Flavio should also be discussed in another jira. http://wiki.apache.org/hadoop/HowToContribute
          Hide
          Luca Telloli added a comment -

          cancelling for later resubmission

          Show
          Luca Telloli added a comment - cancelling for later resubmission
          Hide
          Flavio Junqueira added a comment -

          Thanks for your comments, Konstantin. The patch I uploaded removes all reformatting modifications from FSEditLog and TestEditLog, and hopefully now relevant changes are more evident.

          The discussion on configuration was triggered when you asked to make FSEditLog.sizeFlushBuffer a private static field of EditLogFileOutputStream together with FSEditLog.setBufferCapacity() method. TestEditLog calls FSEditLog::setBufferCapacity(), so it seems necessary to pass a buffer size to FSEditLog, in particular because the buffer size value is necessary in the constructor of EditLogFileOutputStream. In any case, as I said before, I agree that a more extensive discussion of how to improve the configuration of log devices belongs in another jira.

          I have run tests for this new patch: core tests pass fine and the findbugs count does not increase. I also believe that the new files are formatted according to the guidelines.

          Show
          Flavio Junqueira added a comment - Thanks for your comments, Konstantin. The patch I uploaded removes all reformatting modifications from FSEditLog and TestEditLog, and hopefully now relevant changes are more evident. The discussion on configuration was triggered when you asked to make FSEditLog.sizeFlushBuffer a private static field of EditLogFileOutputStream together with FSEditLog.setBufferCapacity() method. TestEditLog calls FSEditLog::setBufferCapacity(), so it seems necessary to pass a buffer size to FSEditLog, in particular because the buffer size value is necessary in the constructor of EditLogFileOutputStream. In any case, as I said before, I agree that a more extensive discussion of how to improve the configuration of log devices belongs in another jira. I have run tests for this new patch: core tests pass fine and the findbugs count does not increase. I also believe that the new files are formatted according to the guidelines.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12407091/HADOOP-5721.patch
          against trunk revision 770685.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          -1 release audit. The applied patch generated 486 release audit warnings (more than the trunk's current 484 warnings).

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/artifact/trunk/current/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12407091/HADOOP-5721.patch against trunk revision 770685. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. -1 release audit. The applied patch generated 486 release audit warnings (more than the trunk's current 484 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/artifact/trunk/current/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/281/console This message is automatically generated.
          Hide
          Luca Telloli added a comment -

          Canceling for resubmission

          Show
          Luca Telloli added a comment - Canceling for resubmission
          Hide
          Flavio Junqueira added a comment -

          Apologies for yet another patch. The previous QA gave a -1 to release audit, and I realized that two new files didn't have the license header. I imagine this is the problem hudson was complaining about. I can't say for sure because the link to the file containing release audit warnings is broken.

          This new patch adds the license header to the new files on top of the changes I mentioned earlier.

          Show
          Flavio Junqueira added a comment - Apologies for yet another patch. The previous QA gave a -1 to release audit, and I realized that two new files didn't have the license header. I imagine this is the problem hudson was complaining about. I can't say for sure because the link to the file containing release audit warnings is broken. This new patch adds the license header to the new files on top of the changes I mentioned earlier.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12407121/HADOOP-5721.patch
          against trunk revision 770685.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/284/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/284/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/284/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/284/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12407121/HADOOP-5721.patch against trunk revision 770685. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/284/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/284/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/284/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/284/console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          The reason for core tests to fail seem to be unrelated to this patch. I have checked the test code and run on two different systems (Mac OS X 10.5.6 and Ubuntu 8.04.1) and tests succeed for me. In fact, the core tests have passed in the previous time that hudson ran it, and this patch is essentially the same, with the difference of the license headers in two new files. I'd suggest to let hudson run it again to see if it fails another time.

          Show
          Flavio Junqueira added a comment - The reason for core tests to fail seem to be unrelated to this patch. I have checked the test code and run on two different systems (Mac OS X 10.5.6 and Ubuntu 8.04.1) and tests succeed for me. In fact, the core tests have passed in the previous time that hudson ran it, and this patch is essentially the same, with the difference of the license headers in two new files. I'd suggest to let hudson run it again to see if it fails another time.
          Hide
          Luca Telloli added a comment -

          cancelling for re-testing on Hudson

          Show
          Luca Telloli added a comment - cancelling for re-testing on Hudson
          Hide
          Luca Telloli added a comment -

          resub for QA

          A diff among the two latest patches shows that nothing changed apart from adding the apache license.

          Show
          Luca Telloli added a comment - resub for QA A diff among the two latest patches shows that nothing changed apart from adding the apache license.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12407121/HADOOP-5721.patch
          against trunk revision 771607.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/290/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/290/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/290/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/290/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12407121/HADOOP-5721.patch against trunk revision 771607. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/290/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/290/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/290/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/290/console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          The contrib test that has failed in the last hudson run is org.apache.hadoop.mapred.TestCapacitySchedulerConf. I have also observed the following message:

          FATAL: Java heap space
          java.lang.OutOfMemoryError: Java heap space

          but it is not clear if this is related to the failure because it is at the end of the console output. The test seems to be unrelated to the current patch as it tests a map-reduce scheduler (this is how I understand it at least).

          Unless it is strictly necessary and given that the queue of hudson is quite long, I would suggest not to submit it again. Does anyone else have an opinion about the results of the last hudson runs?

          Show
          Flavio Junqueira added a comment - The contrib test that has failed in the last hudson run is org.apache.hadoop.mapred.TestCapacitySchedulerConf. I have also observed the following message: FATAL: Java heap space java.lang.OutOfMemoryError: Java heap space but it is not clear if this is related to the failure because it is at the end of the console output. The test seems to be unrelated to the current patch as it tests a map-reduce scheduler (this is how I understand it at least). Unless it is strictly necessary and given that the queue of hudson is quite long, I would suggest not to submit it again. Does anyone else have an opinion about the results of the last hudson runs?
          Hide
          Hemanth Yamijala added a comment -

          I guess the failure in the capacity scheduler test case is because of a stale capacity-scheduler config file remaining in the workspace where the test is run. Does hudson run tests on a clean workspace everytime ? If it does, this error should not occur.

          Show
          Hemanth Yamijala added a comment - I guess the failure in the capacity scheduler test case is because of a stale capacity-scheduler config file remaining in the workspace where the test is run. Does hudson run tests on a clean workspace everytime ? If it does, this error should not occur.
          Hide
          Nigel Daley added a comment -

          Hemanth out patch testing script used "svn stat" to find and remove all non-workspace files, then it does an "svn update". Clean checkout for every build was taking too long and hammering Apache SVN.

          Show
          Nigel Daley added a comment - Hemanth out patch testing script used "svn stat" to find and remove all non-workspace files, then it does an "svn update". Clean checkout for every build was taking too long and hammering Apache SVN.
          Hide
          Flavio Junqueira added a comment -

          Thanks for the comment, Hemanth. I don't have an answer to your question on a clean workspace. I have checked the test, though, and it doesn't seem to instantiate a cluster, like MiniDFSCluster, so it doesn't look like the patch can affect that test. Do you agree? Am I missing anything?

          Show
          Flavio Junqueira added a comment - Thanks for the comment, Hemanth. I don't have an answer to your question on a clean workspace. I have checked the test, though, and it doesn't seem to instantiate a cluster, like MiniDFSCluster, so it doesn't look like the patch can affect that test. Do you agree? Am I missing anything?
          Hide
          Hemanth Yamijala added a comment -

          I don't think the failure is related to this patch at all and shouldn't hold commit of this patch.

          I committed a change to capacity scheduler (HADOOP-5726) which changed the capacity scheduler config template file (capacity-scheduler.xml.template). At build time, in a clean workspace, the template file is copied to become the capacity-scheduler.xml file. But this does not happen if the file already exists (as it could in an existing workspace). In such a case, the new code is looking for some config properties that changed in HADOOP-5726, which are not found in the older capacity-scheduler.xml file and hence fails.

          I don't know how this can be fixed without changing the logic by which the build script copies the template files to the actual files. Any ideas, Nigel ?

          Show
          Hemanth Yamijala added a comment - I don't think the failure is related to this patch at all and shouldn't hold commit of this patch. I committed a change to capacity scheduler ( HADOOP-5726 ) which changed the capacity scheduler config template file (capacity-scheduler.xml.template). At build time, in a clean workspace, the template file is copied to become the capacity-scheduler.xml file. But this does not happen if the file already exists (as it could in an existing workspace). In such a case, the new code is looking for some config properties that changed in HADOOP-5726 , which are not found in the older capacity-scheduler.xml file and hence fails. I don't know how this can be fixed without changing the logic by which the build script copies the template files to the actual files. Any ideas, Nigel ?
          Hide
          Nigel Daley added a comment -

          Hemanth, we "rm" each file identified by "svn stat" before we call "svn update". Why do you think that doesn't mimic a clean workspace?

          Show
          Nigel Daley added a comment - Hemanth, we "rm" each file identified by "svn stat" before we call "svn update". Why do you think that doesn't mimic a clean workspace?
          Hide
          Hemanth Yamijala added a comment -

          Hemanth, we "rm" each file identified by "svn stat" before we call "svn update". Why do you think that doesn't mimic a clean workspace?

          Nigel, when I check out a fresh local copy, the conf directory contains only *.xml.template files. When a build is done on this copy, the *.xml.template files are copied to *.xml files. These files have already been added to the svn ignore list as they are not to be under version control. Hence svn stat does not list these files, and hence they are not removed. So, maybe the hudson script should also remove the files ignored by svn to mimic a clean workspace ?

          Note that this problem should also occur to any file that's ignored and changed in some revision. However, since most of the *.xml files are empty by default, they work fine. In the capacity scheduler case, there are some contents in the default file as well which changed in HADOOP-5726.

          Show
          Hemanth Yamijala added a comment - Hemanth, we "rm" each file identified by "svn stat" before we call "svn update". Why do you think that doesn't mimic a clean workspace? Nigel, when I check out a fresh local copy, the conf directory contains only *.xml.template files. When a build is done on this copy, the *.xml.template files are copied to *.xml files. These files have already been added to the svn ignore list as they are not to be under version control. Hence svn stat does not list these files, and hence they are not removed. So, maybe the hudson script should also remove the files ignored by svn to mimic a clean workspace ? Note that this problem should also occur to any file that's ignored and changed in some revision. However, since most of the *.xml files are empty by default, they work fine. In the capacity scheduler case, there are some contents in the default file as well which changed in HADOOP-5726 .
          Hide
          Nigel Daley added a comment -

          Ok, thanks Hemanth. Now I get it. Looks like the test-patch.sh script should also remove svn:ignore files. Since this is a little tricky to ascertain recursively, I suggest we use the .gitignore contents. This will get us 95+% improvement from what we're doing today.

          Show
          Nigel Daley added a comment - Ok, thanks Hemanth. Now I get it. Looks like the test-patch.sh script should also remove svn:ignore files. Since this is a little tricky to ascertain recursively, I suggest we use the .gitignore contents. This will get us 95+% improvement from what we're doing today.
          Hide
          Konstantin Shvachko added a comment -
          • The patch should be regenerated because of the recent test packages restructuring.
          • You forgot to remove sizeOutputFlushBuffer from EditLogFileOutputStream
          • In EditLogFileOutputStream and EditLogFileInputStream "@Override" and the comment "// JournalStream" should be on the same line because the comments clarifies what the override overrides.
            +1 other than that.
          Show
          Konstantin Shvachko added a comment - The patch should be regenerated because of the recent test packages restructuring. You forgot to remove sizeOutputFlushBuffer from EditLogFileOutputStream In EditLogFileOutputStream and EditLogFileInputStream "@Override" and the comment "// JournalStream" should be on the same line because the comments clarifies what the override overrides. +1 other than that.
          Hide
          Luca Telloli added a comment -

          canceling for resubmission

          Show
          Luca Telloli added a comment - canceling for resubmission
          Hide
          Luca Telloli added a comment -

          Addressed Konstantin's requests:

          • The patch uses the new test structure
          • I didn't find any reference to sizeOutputFlushBuffer in EditLogFileOutputStream
          • The override lines have been corrected

          Additionally, I removed an unused import from the previous patch in EditLogFileOutputStream.

          namenode tests pass without errors on my machine

          Show
          Luca Telloli added a comment - Addressed Konstantin's requests: The patch uses the new test structure I didn't find any reference to sizeOutputFlushBuffer in EditLogFileOutputStream The override lines have been corrected Additionally, I removed an unused import from the previous patch in EditLogFileOutputStream. namenode tests pass without errors on my machine
          Hide
          Konstantin Shvachko added a comment -

          +1
          Sorry about the sizeOutputFlushBuffer confusion, my bad.

          Show
          Konstantin Shvachko added a comment - +1 Sorry about the sizeOutputFlushBuffer confusion, my bad.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12407780/HADOOP-5721.patch
          against trunk revision 773655.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/324/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/324/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/324/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/324/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12407780/HADOOP-5721.patch against trunk revision 773655. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/324/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/324/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/324/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/324/console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          Can we have this patch committed? Is there anything else we need to do?

          Show
          Flavio Junqueira added a comment - Can we have this patch committed? Is there anything else we need to do?
          Hide
          Konstantin Shvachko added a comment -

          I just committed this. Thank you Luca Telloli and Flavio Junqueira.

          Show
          Konstantin Shvachko added a comment - I just committed this. Thank you Luca Telloli and Flavio Junqueira.
          Hide
          Konstantin Shvachko added a comment -

          Attaching the file to reflect the actual commit. I made a one word change (DeprecatedUTF8 vs UTF8) in order to avoid further delays.

          Show
          Konstantin Shvachko added a comment - Attaching the file to reflect the actual commit. I made a one word change (DeprecatedUTF8 vs UTF8) in order to avoid further delays.

            People

            • Assignee:
              Unassigned
              Reporter:
              Luca Telloli
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development