Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: hdfs-client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This sub task separate DataStreamer from DFSOutputStream. New DataStreamer will accept packets and write them to remote datanodes.

      1. HDFS-7854.010.patch
        148 kB
        Jing Zhao
      2. HDFS-7854-001.patch
        138 kB
        Li Bo
      3. HDFS-7854-002.patch
        137 kB
        Li Bo
      4. HDFS-7854-003.patch
        137 kB
        Li Bo
      5. HDFS-7854-004.patch
        140 kB
        Li Bo
      6. HDFS-7854-004-duplicate.patch
        140 kB
        Zhe Zhang
      7. HDFS-7854-004-duplicate2.patch
        140 kB
        Li Bo
      8. HDFS-7854-004-duplicate3.patch
        140 kB
        Li Bo
      9. HDFS-7854-005.patch
        142 kB
        Li Bo
      10. HDFS-7854-006.patch
        147 kB
        Li Bo
      11. HDFS-7854-007.patch
        149 kB
        Zhe Zhang
      12. HDFS-7854-008.patch
        149 kB
        Zhe Zhang
      13. HDFS-7854-009.patch
        149 kB
        Zhe Zhang

        Issue Links

          Activity

          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12702107/HDFS-7854-001.patch
          against trunk revision 952640f.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9761//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12702107/HDFS-7854-001.patch against trunk revision 952640f. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9761//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12703032/HDFS-7854-002.patch
          against trunk revision 95bfd08.

          -1 patch. Trunk compilation may be broken.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9771//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12703032/HDFS-7854-002.patch against trunk revision 95bfd08. -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9771//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12703032/HDFS-7854-002.patch
          against trunk revision 95bfd08.

          -1 patch. Trunk compilation may be broken.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9773//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12703032/HDFS-7854-002.patch against trunk revision 95bfd08. -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9773//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12703032/HDFS-7854-002.patch
          against trunk revision 24db081.

          -1 patch. Trunk compilation may be broken.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9783//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12703032/HDFS-7854-002.patch against trunk revision 24db081. -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9783//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12703032/HDFS-7854-002.patch
          against trunk revision 21101c0.

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

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

          -1 javac. The applied patch generated 1156 javac compiler warnings (more than the trunk's current 1155 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings.

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

          +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9785//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9785//artifact/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
          Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9785//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9785//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12703032/HDFS-7854-002.patch against trunk revision 21101c0. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. -1 javac . The applied patch generated 1156 javac compiler warnings (more than the trunk's current 1155 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9785//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9785//artifact/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9785//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9785//console This message is automatically generated.
          Hide
          libo-intel Li Bo added a comment -

          Patch 003 fixes one javac warning. Two findBugs warnings seem already existing before refactoring. They're new warnings because DataStreamer is separated out as a new class.

          Show
          libo-intel Li Bo added a comment - Patch 003 fixes one javac warning. Two findBugs warnings seem already existing before refactoring. They're new warnings because DataStreamer is separated out as a new class.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12703349/HDFS-7854-003.patch
          against trunk revision dfd8da7.

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

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

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings.

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

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.util.TestByteArrayManager
          org.apache.hadoop.hdfs.server.namenode.TestFileTruncate

          The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestAppendSnapshotTruncate

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9789//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9789//artifact/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9789//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12703349/HDFS-7854-003.patch against trunk revision dfd8da7. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.util.TestByteArrayManager org.apache.hadoop.hdfs.server.namenode.TestFileTruncate The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestAppendSnapshotTruncate Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9789//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9789//artifact/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9789//console This message is automatically generated.
          Hide
          libo-intel Li Bo added a comment -

          TestByteArrayManager passes on my local machine, TestFileTruncate.testTruncateWithDataNodesRestart is blocked at cluster.triggerBlockReports(). I think the failure of these two unit tests is not caused by patch 003.

          Show
          libo-intel Li Bo added a comment - TestByteArrayManager passes on my local machine, TestFileTruncate.testTruncateWithDataNodesRestart is blocked at cluster.triggerBlockReports() . I think the failure of these two unit tests is not caused by patch 003.
          Hide
          jingzhao Jing Zhao added a comment -

          Thanks for working on this, Bo. So for a findbugs warning, we need to either fix it or add it to the findbugsExcludeFile. More specifically, I think the one in ResponseProcessor.run() should be added to the findbugsExcludeFile, but the findbugs warning on DFSOutputStream.src looks new to me and maybe we should fix it.

          Show
          jingzhao Jing Zhao added a comment - Thanks for working on this, Bo. So for a findbugs warning, we need to either fix it or add it to the findbugsExcludeFile. More specifically, I think the one in ResponseProcessor.run() should be added to the findbugsExcludeFile, but the findbugs warning on DFSOutputStream.src looks new to me and maybe we should fix it.
          Hide
          libo-intel Li Bo added a comment -

          hi, Jing. I download the code from trunk and still find the same warning on DFSOutputStream.src. The field src is read-only except one unit test will modify it in single thread. So I think we can also add it to the findbugsExcludeFile.

          Show
          libo-intel Li Bo added a comment - hi, Jing. I download the code from trunk and still find the same warning on DFSOutputStream.src . The field src is read-only except one unit test will modify it in single thread. So I think we can also add it to the findbugsExcludeFile.
          Hide
          jingzhao Jing Zhao added a comment -

          Some comments so far:

          1. Can we keep currentSeqno, lastQueuedSeqno and bytesCurBlock in DFSOutputStream?
          2. Some fields in DataStreamer can be declared as final, e.g., dfsClient, src, checksum4WriteBlock, progress, stat, blockSize, fileId, etc.
          3. We can move the following code into a single function.
                long usedInLastBlock = stat.getLen() % blockSize;
                int freeInLastBlock = (int)(blockSize - usedInLastBlock);
            
                // calculate the amount of free space in the pre-existing
                // last crc chunk
                int usedInCksum = (int)(stat.getLen() % bytesPerChecksum);
                int freeInCksum = bytesPerChecksum - usedInCksum;
            
                // if there is space in the last block, then we have to
                // append to that block
                if (freeInLastBlock == blockSize) {
                  throw new IOException("The last block for file " +
                      src + " is full.");
                }
            
                if (usedInCksum > 0 && freeInCksum > 0) {
                  // if there is space in the last partial chunk, then
                  // setup in such a way that the next packet will have only
                  // one chunk that fills up the partial chunk.
                  //
                  computePacketChunkSize(0, freeInCksum);
                  setChecksumBufSize(freeInCksum);
                  streamer.setAppendChunk(true);
                } else {
                  // if the remaining space in the block is smaller than
                  // that expected size of of a packet, then create
                  // smaller size packet.
                  //
                  computePacketChunkSize(Math.min(dfsClient.getConf().writePacketSize, freeInLastBlock),
                      bytesPerChecksum);
                }
            
          4. We do not need to compute the checksumSize for a heartbeat packet since it does not contain real data.
              private DFSPacket createHeartbeatPacket() throws InterruptedIOException {
                int checksumSize = checksum4WriteBlock.getChecksumSize();
                if (isLazyPersist(stat) && stat.getReplication() == 1) {
                  checksumSize = 0;
                }
                final byte[] buf = new byte[PacketHeader.PKT_MAX_HEADER_LEN];
                return new DFSPacket(buf, 0, 0, DFSPacket.HEART_BEAT_SEQNO, checksumSize, false);
              }
            
          5. Instead of directly acquiring DataStreamer#dataQueue's monitor and modifying it in DFSOutputStream, we'd better wrap it into a DataStreamer method.
              private void queueCurrentPacket() {
                synchronized (streamer.dataQueue) {
                  if (currentPacket == null) return;
                  streamer.dataQueue.addLast(currentPacket);
                  streamer.setLastQueuedSeqno(currentPacket.getSeqno());
                  if (DFSClient.LOG.isDebugEnabled()) {
                    DFSClient.LOG.debug("Queued packet " + currentPacket.getSeqno());
                  }
                  currentPacket = null;
                  streamer.dataQueue.notifyAll();
                }
              }
            
          6. Similar for DFSOutputStream#waitAndQueueCurrentPacket. In general it is not very clean to directly access streamer's dataqueue and using its monitor or call its wait method from DFSOutputStream.
          7. What is the usage of toTerminate? Can we only use streamerClosed?
          Show
          jingzhao Jing Zhao added a comment - Some comments so far: Can we keep currentSeqno , lastQueuedSeqno and bytesCurBlock in DFSOutputStream? Some fields in DataStreamer can be declared as final, e.g., dfsClient, src, checksum4WriteBlock, progress, stat, blockSize, fileId, etc. We can move the following code into a single function. long usedInLastBlock = stat.getLen() % blockSize; int freeInLastBlock = ( int )(blockSize - usedInLastBlock); // calculate the amount of free space in the pre-existing // last crc chunk int usedInCksum = ( int )(stat.getLen() % bytesPerChecksum); int freeInCksum = bytesPerChecksum - usedInCksum; // if there is space in the last block, then we have to // append to that block if (freeInLastBlock == blockSize) { throw new IOException( "The last block for file " + src + " is full." ); } if (usedInCksum > 0 && freeInCksum > 0) { // if there is space in the last partial chunk, then // setup in such a way that the next packet will have only // one chunk that fills up the partial chunk. // computePacketChunkSize(0, freeInCksum); setChecksumBufSize(freeInCksum); streamer.setAppendChunk( true ); } else { // if the remaining space in the block is smaller than // that expected size of of a packet, then create // smaller size packet. // computePacketChunkSize( Math .min(dfsClient.getConf().writePacketSize, freeInLastBlock), bytesPerChecksum); } We do not need to compute the checksumSize for a heartbeat packet since it does not contain real data. private DFSPacket createHeartbeatPacket() throws InterruptedIOException { int checksumSize = checksum4WriteBlock.getChecksumSize(); if (isLazyPersist(stat) && stat.getReplication() == 1) { checksumSize = 0; } final byte [] buf = new byte [PacketHeader.PKT_MAX_HEADER_LEN]; return new DFSPacket(buf, 0, 0, DFSPacket.HEART_BEAT_SEQNO, checksumSize, false ); } Instead of directly acquiring DataStreamer#dataQueue 's monitor and modifying it in DFSOutputStream, we'd better wrap it into a DataStreamer method. private void queueCurrentPacket() { synchronized (streamer.dataQueue) { if (currentPacket == null ) return ; streamer.dataQueue.addLast(currentPacket); streamer.setLastQueuedSeqno(currentPacket.getSeqno()); if (DFSClient.LOG.isDebugEnabled()) { DFSClient.LOG.debug( "Queued packet " + currentPacket.getSeqno()); } currentPacket = null ; streamer.dataQueue.notifyAll(); } } Similar for DFSOutputStream#waitAndQueueCurrentPacket . In general it is not very clean to directly access streamer's dataqueue and using its monitor or call its wait method from DFSOutputStream. What is the usage of toTerminate ? Can we only use streamerClosed ?
          Hide
          jingzhao Jing Zhao added a comment -

          I think we can also declare DFSOutputStream.src as final. Then for the unit test we can use reflection to change its value.

          Show
          jingzhao Jing Zhao added a comment - I think we can also declare DFSOutputStream.src as final. Then for the unit test we can use reflection to change its value.
          Hide
          libo-intel Li Bo added a comment -

          Thanks for Jing's careful review. I will upload a new patch later.

          1. Yes, we can also keep these fields in DFSOutputStream. But I think it’d better to put them in DataStreamer. DataStreamer is responsible for writing blocks, while DFSOutputStream just puts packets into DataStreamer. And if there’re multiple DataStreamers in DFSOutputStream, it’s not necessary to maintain an array of each field in DFSOutputStream .
          2. Will fix it in new patch.
          3. Will fix it in new patch.
          4. Will fix it in new patch.
          5 & 6 Because DFSOutputStream also needs to add packets to dataQueue, I think it’d better to let DFSOutputStream maintain these two queues and transfer them to DataStreamer via its constructor.
          7. We have moved several fields from DFSOutputStream to DataStreamer, after streamer = null, these fields can’t be accessed. So a new field toTerminate is imported. toTerminate = true equals to streamer = null, if(!toTerminate) equals to if(streamer!=null). If we use streamerClosed instead of toTerminate, I think we may change the original logic and cause new errors(I will run unit tests later to see if the replacement will cause new errors and why).
          8. DFSOupuStream.src: this is a way to avoid the findbugs warning, will fix it in new patch.

          Show
          libo-intel Li Bo added a comment - Thanks for Jing's careful review. I will upload a new patch later. 1. Yes, we can also keep these fields in DFSOutputStream. But I think it’d better to put them in DataStreamer. DataStreamer is responsible for writing blocks, while DFSOutputStream just puts packets into DataStreamer. And if there’re multiple DataStreamers in DFSOutputStream, it’s not necessary to maintain an array of each field in DFSOutputStream . 2. Will fix it in new patch. 3. Will fix it in new patch. 4. Will fix it in new patch. 5 & 6 Because DFSOutputStream also needs to add packets to dataQueue, I think it’d better to let DFSOutputStream maintain these two queues and transfer them to DataStreamer via its constructor. 7. We have moved several fields from DFSOutputStream to DataStreamer, after streamer = null , these fields can’t be accessed. So a new field toTerminate is imported. toTerminate = true equals to streamer = null , if(!toTerminate) equals to if(streamer!=null) . If we use streamerClosed instead of toTerminate , I think we may change the original logic and cause new errors(I will run unit tests later to see if the replacement will cause new errors and why). 8. DFSOupuStream.src : this is a way to avoid the findbugs warning, will fix it in new patch.
          Hide
          jingzhao Jing Zhao added a comment - - edited

          Thanks for the response, Bo. For 5&6, after separating DataStreamer out, DFSOutputStream should not know/use the dataQueue anymore, since this is the internal details hidden inside of DataStreamer.

          Thus DataStreamer should maintain its own queue and provide corresponding queueCurrentPacket and waitAndQueueCurrentPacket methods for DFSOutputStream. My earlier patch also demos this idea. In this way it will be more clean and easier to maintain multiple DataStreamers for a striped DFSOutputStream.

          Show
          jingzhao Jing Zhao added a comment - - edited Thanks for the response, Bo. For 5&6, after separating DataStreamer out, DFSOutputStream should not know/use the dataQueue anymore, since this is the internal details hidden inside of DataStreamer. Thus DataStreamer should maintain its own queue and provide corresponding queueCurrentPacket and waitAndQueueCurrentPacket methods for DFSOutputStream. My earlier patch also demos this idea. In this way it will be more clean and easier to maintain multiple DataStreamers for a striped DFSOutputStream.
          Hide
          libo-intel Li Bo added a comment -

          hi, Jing
          we may encounter some problems in your way of handling of queueCurrentPacket and waitAndQueueCurrentPacket, and also waitForAckedSeqno.
          DFSOutputStream#closed is different with DataStreamer#streamClosed. If we move these three methods to DataStreamer and replace DFSOutputStream#closed with DataStreamer#streamClosed, we have changed some logic of the original code. For example, two threads(t1, t2) share the same DFSOutputStream, if t1 call {{ DFSOutputStream #close()}}, and an exception is thrown in this function before closing streamer, then DFSOutputStream#closed is true and DataStreamer#streamClosed is still false. If t2 calls {{ waitForAckedSeqno }}, originally it will immediately quit this function because DFSOutputStream#closed is true; if we call streamer. waitForAckedSeqno (), now DataStreamer#streamClosed is still false, so t2 will wait for the while loop to exit. I remember that I have encountered such kinds of problems before.

          Because DFSOutputStream also generates packets, I think it’s not bad for DFSOutputStream to maintain a queue shared with a DataStreamer, and it gives less changes to the original code.

          Show
          libo-intel Li Bo added a comment - hi, Jing we may encounter some problems in your way of handling of queueCurrentPacket and waitAndQueueCurrentPacket , and also waitForAckedSeqno . DFSOutputStream#closed is different with DataStreamer#streamClosed . If we move these three methods to DataStreamer and replace DFSOutputStream#closed with DataStreamer#streamClosed , we have changed some logic of the original code. For example, two threads(t1, t2) share the same DFSOutputStream, if t1 call {{ DFSOutputStream #close()}}, and an exception is thrown in this function before closing streamer, then DFSOutputStream#closed is true and DataStreamer#streamClosed is still false. If t2 calls {{ waitForAckedSeqno }}, originally it will immediately quit this function because DFSOutputStream#closed is true; if we call streamer. waitForAckedSeqno () , now DataStreamer#streamClosed is still false, so t2 will wait for the while loop to exit. I remember that I have encountered such kinds of problems before. Because DFSOutputStream also generates packets, I think it’s not bad for DFSOutputStream to maintain a queue shared with a DataStreamer, and it gives less changes to the original code.
          Hide
          libo-intel Li Bo added a comment -

          If we want dataQueue in DataStreamer, we can provide DataStreamer#getDataQueue() but still keep queueCurrentPacket }} , {{waitAndQueueCurrentPacket, waitForAckedSeqno in DFSOutputStream

          Show
          libo-intel Li Bo added a comment - If we want dataQueue in DataStreamer, we can provide DataStreamer#getDataQueue() but still keep queueCurrentPacket }} , {{waitAndQueueCurrentPacket , waitForAckedSeqno in DFSOutputStream
          Hide
          libo-intel Li Bo added a comment -

          Patch 004 includes several changes advised by Jing. Function queueCurrentPacket is moved to class DataStreamer and renamed to queuePacket. Currently waitAndQueueCurrentPacket and waitForAckedSeqno are still kept in DFSOutputStream due to the following considerations: there’re two threads, the main thread and streamer thread, the main thread will wait until the data queue has space; it’s more reasonable to keep the main thread waiting by direct calling {{ waitForAckedSeqno }} than by calling streamer. waitForAckedSeqno }}; {{ waitAndQueueCurrentPacket }} and {{waitForAckedSeqno have to check DFSOutputStream.closed which can’t be substituted by DataStreamer.streamClosed.
          dataQueue is still kept in DFSOutputStream currently and we can also move it to DataStreamer. We can treat the two class as producer and consumer, I think it’s reasonable to let the producer be aware of the sharing pool.

          Show
          libo-intel Li Bo added a comment - Patch 004 includes several changes advised by Jing. Function queueCurrentPacket is moved to class DataStreamer and renamed to queuePacket . Currently waitAndQueueCurrentPacket and waitForAckedSeqno are still kept in DFSOutputStream due to the following considerations: there’re two threads, the main thread and streamer thread, the main thread will wait until the data queue has space; it’s more reasonable to keep the main thread waiting by direct calling {{ waitForAckedSeqno }} than by calling streamer. waitForAckedSeqno }}; {{ waitAndQueueCurrentPacket }} and {{waitForAckedSeqno have to check DFSOutputStream.closed which can’t be substituted by DataStreamer.streamClosed . dataQueue is still kept in DFSOutputStream currently and we can also move it to DataStreamer. We can treat the two class as producer and consumer, I think it’s reasonable to let the producer be aware of the sharing pool.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12704365/HDFS-7854-004.patch
          against trunk revision 387f271.

          -1 patch. Trunk compilation may be broken.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9869//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12704365/HDFS-7854-004.patch against trunk revision 387f271. -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9869//console This message is automatically generated.
          Hide
          libo-intel Li Bo added a comment -

          It seems jenkins has some problems. Patch 004 works well on my local machine. I will try later

          Show
          libo-intel Li Bo added a comment - It seems jenkins has some problems. Patch 004 works well on my local machine. I will try later
          Hide
          zhz Zhe Zhang added a comment -

          004 patch works on my local machine as well. Submitting a duplicate patch to trigger Jenkins.

          Show
          zhz Zhe Zhang added a comment - 004 patch works on my local machine as well. Submitting a duplicate patch to trigger Jenkins.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12704455/HDFS-7854-004-duplicate.patch
          against trunk revision 8180e67.

          -1 patch. Trunk compilation may be broken.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9877//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12704455/HDFS-7854-004-duplicate.patch against trunk revision 8180e67. -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9877//console This message is automatically generated.
          Hide
          libo-intel Li Bo added a comment -

          trigger again

          Show
          libo-intel Li Bo added a comment - trigger again
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12704748/HDFS-7854-004-duplicate2.patch
          against trunk revision 3ff1ba2.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

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

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9895//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9895//artifact/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9895//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12704748/HDFS-7854-004-duplicate2.patch against trunk revision 3ff1ba2. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9895//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9895//artifact/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9895//console This message is automatically generated.
          Hide
          zhz Zhe Zhang added a comment -

          TestRetryCacheWithHA passes locally.

          Show
          zhz Zhe Zhang added a comment - TestRetryCacheWithHA passes locally.
          Hide
          libo-intel Li Bo added a comment -

          The findbugsWarnings's url cannot be opened, trigger Jenkins again.

          Show
          libo-intel Li Bo added a comment - The findbugsWarnings's url cannot be opened, trigger Jenkins again.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12704951/HDFS-7854-004-duplicate3.patch
          against trunk revision 5608520.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          -1 eclipse:eclipse. The patch failed to build with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

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

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9908//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9908//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12704951/HDFS-7854-004-duplicate3.patch against trunk revision 5608520. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. -1 eclipse:eclipse . The patch failed to build with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9908//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9908//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12705528/HDFS-7854-005.patch
          against trunk revision 93d0f4a.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

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

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.balancer.TestBalancer
          org.apache.hadoop.tracing.TestTracing

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9972//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9972//artifact/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9972//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12705528/HDFS-7854-005.patch against trunk revision 93d0f4a. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.balancer.TestBalancer org.apache.hadoop.tracing.TestTracing Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9972//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9972//artifact/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9972//console This message is automatically generated.
          Hide
          zhz Zhe Zhang added a comment -

          TestBalancer passes locally. TestTracing#testWriteTraceHooks looks like a real failure.

          We should also update the following section in hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml

                <Match>
                  <Class name="org.apache.hadoop.hdfs.DFSOutputStream$DataStreamer$ResponseProcessor" />
                  <Method name="run" />
                  <Bug pattern="REC_CATCH_EXCEPTION" />
                </Match>
          
          Show
          zhz Zhe Zhang added a comment - TestBalancer passes locally. TestTracing#testWriteTraceHooks looks like a real failure. We should also update the following section in hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml <Match> < Class name= "org.apache.hadoop.hdfs.DFSOutputStream$DataStreamer$ResponseProcessor" /> <Method name= "run" /> <Bug pattern= "REC_CATCH_EXCEPTION" /> </Match>
          Hide
          libo-intel Li Bo added a comment -

          The failure of TestTracing#testWriteTraceHooks seems caused by HDFS-7054, and HDFS-7963 is just created to fix this problem. I download current code from trunk and also find this test failed.

          Show
          libo-intel Li Bo added a comment - The failure of TestTracing#testWriteTraceHooks seems caused by HDFS-7054 , and HDFS-7963 is just created to fix this problem. I download current code from trunk and also find this test failed.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12705883/HDFS-7854-006.patch
          against trunk revision 8041267.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

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

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.tracing.TestTracing

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10001//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10001//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12705883/HDFS-7854-006.patch against trunk revision 8041267. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.tracing.TestTracing Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10001//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10001//console This message is automatically generated.
          Hide
          zhz Zhe Zhang added a comment -

          Thanks Bo for the updated patch. It looks good to me. It needs to be rebased against the trunk again.

          It seems dataQueue is also moved to DataStreamer. Jing Zhao Do you see other issues in the patch?

          Show
          zhz Zhe Zhang added a comment - Thanks Bo for the updated patch. It looks good to me. It needs to be rebased against the trunk again. It seems dataQueue is also moved to DataStreamer . Jing Zhao Do you see other issues in the patch?
          Hide
          jingzhao Jing Zhao added a comment -

          I quickly went through the latest patch and it looks good to me. I will take another review this weekend and try to get this committed asap.

          Show
          jingzhao Jing Zhao added a comment - I quickly went through the latest patch and it looks good to me. I will take another review this weekend and try to get this committed asap.
          Hide
          zhz Zhe Zhang added a comment -

          Thanks Jing! In that case I'll do the rebase now.

          Show
          zhz Zhe Zhang added a comment - Thanks Jing! In that case I'll do the rebase now.
          Hide
          zhz Zhe Zhang added a comment -

          The rebase is mainly about 3 commits: HDFS-7835 and HDFS-6841. Their changes on DFSOutputStream are relatively simple and I believe I included all related changes in DataStreamer.

          HDFS-7054 was also committed after this refactor started. I took a look and seems Bo has already taken care of it. Colin P. McCabe would be great if you could verify that the new DataStreamer class includes the changes you made.

          Show
          zhz Zhe Zhang added a comment - The rebase is mainly about 3 commits: HDFS-7835 and HDFS-6841 . Their changes on DFSOutputStream are relatively simple and I believe I included all related changes in DataStreamer . HDFS-7054 was also committed after this refactor started. I took a look and seems Bo has already taken care of it. Colin P. McCabe would be great if you could verify that the new DataStreamer class includes the changes you made.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12706092/HDFS-7854-007.patch
          against trunk revision 7f1e2f9.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10016//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12706092/HDFS-7854-007.patch against trunk revision 7f1e2f9. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10016//console This message is automatically generated.
          Hide
          zhz Zhe Zhang added a comment -

          007 patch wasn't correctly generated.

          Show
          zhz Zhe Zhang added a comment - 007 patch wasn't correctly generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12706129/HDFS-7854-008.patch
          against trunk revision e1feb4e.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

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

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.tracing.TestTracing

          The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.namenode.metrics.TestNameNodeMetrics

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10020//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10020//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12706129/HDFS-7854-008.patch against trunk revision e1feb4e. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.tracing.TestTracing The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.namenode.metrics.TestNameNodeMetrics Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10020//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10020//console This message is automatically generated.
          Hide
          zhz Zhe Zhang added a comment -

          I went through 008 patch and found it missed 3 Time.now() -> Time.monotonicNow() changes in DFSOutputStream, introduced by HDFS-6841. 009 patch addresses it.

          Show
          zhz Zhe Zhang added a comment - I went through 008 patch and found it missed 3 Time.now() -> Time.monotonicNow() changes in DFSOutputStream , introduced by HDFS-6841 . 009 patch addresses it.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12706578/HDFS-7854-009.patch
          against trunk revision 82eda77.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

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

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.tracing.TestTracing
          org.apache.hadoop.hdfs.TestFileCreation

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10033//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10033//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12706578/HDFS-7854-009.patch against trunk revision 82eda77. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.tracing.TestTracing org.apache.hadoop.hdfs.TestFileCreation Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10033//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10033//console This message is automatically generated.
          Hide
          zhz Zhe Zhang added a comment -

          TestTracing and TestFileCreation are both unrelated and both passes locally.

          Show
          zhz Zhe Zhang added a comment - TestTracing and TestFileCreation are both unrelated and both passes locally.
          Hide
          jingzhao Jing Zhao added a comment -

          Thanks Bo and Zhe for working on this! The 009 patch looks pretty good to me. Only some nits:

          1. With the HdfsFileStatus passed into DataStreamer, we may no longer need to define blockSize and fileId in DataStreamer. We can directly call stat#getXXX.
          2. There is a tab in DataStreamer#run
          3. In DFSOutputStream#flushOrSync, there is a redundant ";"
            toWaitFor = streamer.getLastQueuedSeqno();;
            
          4. We can move static methods to the beginning of DataStreamer.java
          5. We should remove the item for DFSOutputStream$DataStreamer$ResponseProcessor from findbugsExcludeFile:
          6. It may be more clear to move the adjustPacketChunkSize and streamer.setPipelineInConstruction call into the first "if" section: these are all the actions for appending to the last block.
          7. DFSOutputStream#completeFile's change is unnecessary?
          8. DFSOutputStream#createSocketForPipeline can be removed
          Show
          jingzhao Jing Zhao added a comment - Thanks Bo and Zhe for working on this! The 009 patch looks pretty good to me. Only some nits: With the HdfsFileStatus passed into DataStreamer, we may no longer need to define blockSize and fileId in DataStreamer . We can directly call stat#getXXX . There is a tab in DataStreamer#run In DFSOutputStream#flushOrSync , there is a redundant ";" toWaitFor = streamer.getLastQueuedSeqno();; We can move static methods to the beginning of DataStreamer.java We should remove the item for DFSOutputStream$DataStreamer$ResponseProcessor from findbugsExcludeFile: It may be more clear to move the adjustPacketChunkSize and streamer.setPipelineInConstruction call into the first "if" section: these are all the actions for appending to the last block. DFSOutputStream#completeFile 's change is unnecessary? DFSOutputStream#createSocketForPipeline can be removed
          Hide
          jingzhao Jing Zhao added a comment -

          Since all the comments are just trivial, upload a patch with all the minor changes to save time. Li Bo and Zhe Zhang, please see if the comments and the patch make sense.

          Show
          jingzhao Jing Zhao added a comment - Since all the comments are just trivial, upload a patch with all the minor changes to save time. Li Bo and Zhe Zhang , please see if the comments and the patch make sense.
          Hide
          zhz Zhe Zhang added a comment -

          Thanks Jing! The changes look good to me.

          DFSOutputStream#completeFile's change is unnecessary?

          Right, seems the rebase I did in 009 patch missed this from HDFS-7835. Thanks for spotting it!

          Show
          zhz Zhe Zhang added a comment - Thanks Jing! The changes look good to me. DFSOutputStream#completeFile's change is unnecessary? Right, seems the rebase I did in 009 patch missed this from HDFS-7835 . Thanks for spotting it!
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12706730/HDFS-7854.010.patch
          against trunk revision 972f1f1.

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

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

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

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

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.namenode.TestStartup
          org.apache.hadoop.tracing.TestTracing
          org.apache.hadoop.hdfs.server.blockmanagement.TestDatanodeManager

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10037//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10037//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12706730/HDFS-7854.010.patch against trunk revision 972f1f1. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.namenode.TestStartup org.apache.hadoop.tracing.TestTracing org.apache.hadoop.hdfs.server.blockmanagement.TestDatanodeManager Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10037//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10037//console This message is automatically generated.
          Hide
          libo-intel Li Bo added a comment -

          hi, Jing, thanks for your careful review and improvement of the patch. I check all the changes and they also look good to me.

          Show
          libo-intel Li Bo added a comment - hi, Jing, thanks for your careful review and improvement of the patch. I check all the changes and they also look good to me.
          Hide
          libo-intel Li Bo added a comment -

          TestStartup,TestDatanodeManager pass locally, TestTracing seems unrelated with current patch.

          Show
          libo-intel Li Bo added a comment - TestStartup , TestDatanodeManager pass locally, TestTracing seems unrelated with current patch.
          Hide
          wheat9 Haohui Mai added a comment -

          +1

          Show
          wheat9 Haohui Mai added a comment - +1
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #7422 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7422/)
          HDFS-7854. Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66)

          • hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7422 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7422/ ) HDFS-7854 . Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66) hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          Hide
          jingzhao Jing Zhao added a comment -

          Thanks for the review, Haohui. I've committed this to trunk and branch-2.

          Thanks for the contribution, Li Bo and Zhe Zhang!

          Show
          jingzhao Jing Zhao added a comment - Thanks for the review, Haohui. I've committed this to trunk and branch-2. Thanks for the contribution, Li Bo and Zhe Zhang !
          Hide
          zhz Zhe Zhang added a comment -

          Thanks Jing for the heavy-lifting to push this through! I just rebased HDFS-7285 to include this.

          Also thanks Haohui for the review.

          Show
          zhz Zhe Zhang added a comment - Thanks Jing for the heavy-lifting to push this through! I just rebased HDFS-7285 to include this. Also thanks Haohui for the review.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #143 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/143/)
          HDFS-7854. Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66)

          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java
          • hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #143 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/143/ ) HDFS-7854 . Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Yarn-trunk #877 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/877/)
          HDFS-7854. Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66)

          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #877 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/877/ ) HDFS-7854 . Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #2075 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2075/)
          HDFS-7854. Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66)

          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java
          • hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2075 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2075/ ) HDFS-7854 . Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #134 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/134/)
          HDFS-7854. Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66)

          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #134 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/134/ ) HDFS-7854 . Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #143 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/143/)
          HDFS-7854. Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66)

          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java
          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #143 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/143/ ) HDFS-7854 . Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2093 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2093/)
          HDFS-7854. Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66)

          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2093 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2093/ ) HDFS-7854 . Separate class DataStreamer out of DFSOutputStream. Contributed by Li Bo. (jing9: rev a16bfff71bd7f00e06e1f59bfe5445a154bb8c66) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileCreation.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DataStreamer.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java hadoop-hdfs-project/hadoop-hdfs/dev-support/findbugsExcludeFile.xml

            People

            • Assignee:
              libo-intel Li Bo
              Reporter:
              libo-intel Li Bo
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development