Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-9809 Abstract implementation-specific details from the datanode
  3. HDFS-10636

Modify ReplicaInfo to remove the assumption that replica metadata and data are stored in java.io.File.

    Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0.0-alpha2
    • Component/s: datanode, fs
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Incompatible change

      Description

      Replace java.io.File related APIs from ReplicaInfo, and enable the definition of new ReplicaInfo sub-classes whose metadata and data can be present on external storages (HDFS-9806).

      1. HDFS-10636.001.patch
        165 kB
        Virajith Jalaparti
      2. HDFS-10636.002.patch
        203 kB
        Virajith Jalaparti
      3. HDFS-10636.003.patch
        203 kB
        Virajith Jalaparti
      4. HDFS-10636.004.patch
        209 kB
        Chris Douglas
      5. HDFS-10636.005.patch
        213 kB
        Virajith Jalaparti
      6. HDFS-10636.006.patch
        218 kB
        Virajith Jalaparti
      7. HDFS-10636.007.patch
        221 kB
        Virajith Jalaparti
      8. HDFS-10636.008.patch
        221 kB
        Virajith Jalaparti
      9. HDFS-10636.009.patch
        221 kB
        Virajith Jalaparti
      10. HDFS-10636.010.patch
        220 kB
        Virajith Jalaparti

        Issue Links

          Activity

          Hide
          virajith Virajith Jalaparti added a comment - - edited

          The patch contains the following changes (most text below is aggregated from comments in HDFS-9809).

          1. Moving the java.io.File related APIs in ReplicaInfo (getBlockFile, getMetaFile) to a subclass of ReplicaInfo called LocalReplica. The classes FinalizedReplica, ReplicaInPipeline, ReplicaUnderRecovery, and ReplicaWaitingToBeRecovered are changed to be subclasses of LocalReplica instead of ReplicaInfo. The motivation behind this change is that we can have ReplicaInfo s that point to blocks located in remote stores and as a result don’t have associated java.io.File s.
            We added various functions to ReplicaInfo in order to replace the calls to ReplicaInfo#getBlockFile, and ReplicaInfo#getMetaFile in the rest of the code.
          2. Using ReplicaInfo.getState() to get the state of a ReplicaInfo instead of using instanceof. A related change is to use the class ReplicaInfo to refer to the replica objects instead of the particular subclass (this required adding additional abstract functions to the ReplicaInfo class).
          3. Addition of a ReplicaBuilder and replacing calls to the constructors of different ReplicaInfo subclasses (ReplicaInPipeline, ReplicaBeingWritten, etc.) with calls to the ReplicaBuilder with the appropriate parameters (ReplicaState, blockId etc.) set.
          4. Changes related to ReplicaInPipeline
          • Change the ReplicaInPipeline to LocalReplicaInPipeline, and change ReplicaInPipelineInterface to ReplicaInPipeline.
          • Add a getReplicaInfo function to the (new) ReplicaInPipeline interface.
          • Move the functions related to writer threads (stopWriter, attemptToSetWriter, interruptThread and setWriter) to the new ReplicaInPipeline interface (i.e., the old ReplicaInPipelineInterface), as only ReplicaInPipeline objects will be associated with writer threads.

          The idea behind the changes above is to add a new ProvidedReplica class (an implementation of ReplicaInfo) which can be:
          (a) used to represent replicas stored in a provided storage (described in more detail in the design documentation of HDFS-9806).
          (b) treated as any other ReplicaInfo in the rest of the code. This would avoid changes to the rest of the Datanode as part of HDFS-9806.
          (c) written to using the existing replication pipeline, without implementing a separate write pipeline for HDFS-9806.

          Show
          virajith Virajith Jalaparti added a comment - - edited The patch contains the following changes (most text below is aggregated from comments in HDFS-9809 ). Moving the java.io.File related APIs in ReplicaInfo ( getBlockFile , getMetaFile ) to a subclass of ReplicaInfo called LocalReplica . The classes FinalizedReplica , ReplicaInPipeline , ReplicaUnderRecovery , and ReplicaWaitingToBeRecovered are changed to be subclasses of LocalReplica instead of ReplicaInfo . The motivation behind this change is that we can have ReplicaInfo s that point to blocks located in remote stores and as a result don’t have associated java.io.File s. We added various functions to ReplicaInfo in order to replace the calls to ReplicaInfo#getBlockFile , and ReplicaInfo#getMetaFile in the rest of the code. Using ReplicaInfo.getState() to get the state of a ReplicaInfo instead of using instanceof . A related change is to use the class ReplicaInfo to refer to the replica objects instead of the particular subclass (this required adding additional abstract functions to the ReplicaInfo class). Addition of a ReplicaBuilder and replacing calls to the constructors of different ReplicaInfo subclasses ( ReplicaInPipeline , ReplicaBeingWritten , etc.) with calls to the ReplicaBuilder with the appropriate parameters ( ReplicaState , blockId etc.) set. Changes related to ReplicaInPipeline Change the ReplicaInPipeline to LocalReplicaInPipeline , and change ReplicaInPipelineInterface to ReplicaInPipeline . Add a getReplicaInfo function to the (new) ReplicaInPipeline interface. Move the functions related to writer threads ( stopWriter , attemptToSetWriter , interruptThread and setWriter ) to the new ReplicaInPipeline interface (i.e., the old ReplicaInPipelineInterface ), as only ReplicaInPipeline objects will be associated with writer threads. The idea behind the changes above is to add a new ProvidedReplica class (an implementation of ReplicaInfo ) which can be: (a) used to represent replicas stored in a provided storage (described in more detail in the design documentation of HDFS-9806 ). (b) treated as any other ReplicaInfo in the rest of the code. This would avoid changes to the rest of the Datanode as part of HDFS-9806 . (c) written to using the existing replication pipeline, without implementing a separate write pipeline for HDFS-9806 .
          Hide
          jpallas Joe Pallas added a comment -

          I haven't reviewed every line, but I noticed in FsDatasetUtil.createNullChecksumFile there's a // TODO Auto-generated catch block.

          Overall I like this, and that it addresses some of the issues raised in HDFS-5194.

          One question about naming: In the context of HDFS-9806, the distinction between Provided and Local makes sense. But there might be other replica implementations that use non-File based storage but are still "local". I don't have a concrete alternative to "Local" to propose, though.

          Show
          jpallas Joe Pallas added a comment - I haven't reviewed every line, but I noticed in FsDatasetUtil.createNullChecksumFile there's a // TODO Auto-generated catch block . Overall I like this, and that it addresses some of the issues raised in HDFS-5194 . One question about naming: In the context of HDFS-9806 , the distinction between Provided and Local makes sense. But there might be other replica implementations that use non- File based storage but are still "local". I don't have a concrete alternative to "Local" to propose, though.
          Hide
          virajith Virajith Jalaparti added a comment -

          Uploading a new patch which contains some classes which were missing from the earlier patch and also fixes the TODO pointed out by Joe Pallas.

          Joe Pallas, yes, agreed. If there is an implementation which uses non-File based local replicas, the name of LocalReplicaInfo can be changed to something like FileReplicaInfo. However, that will be a class rename, and need not involve any more extensive changes.

          Show
          virajith Virajith Jalaparti added a comment - Uploading a new patch which contains some classes which were missing from the earlier patch and also fixes the TODO pointed out by Joe Pallas . Joe Pallas , yes, agreed. If there is an implementation which uses non- File based local replicas, the name of LocalReplicaInfo can be changed to something like FileReplicaInfo . However, that will be a class rename, and need not involve any more extensive changes.
          Hide
          virajith Virajith Jalaparti added a comment -

          Fixed accounting for ramdisk replicas in FsDatasetImpl#getBlockInputStream.

          Show
          virajith Virajith Jalaparti added a comment - Fixed accounting for ramdisk replicas in FsDatasetImpl#getBlockInputStream.
          Hide
          chris.douglas Chris Douglas added a comment -

          Fixed some findbugs, checkstyle.

          Joe Pallas generally I agree on using Local*, but it does match a convention in Hadoop (e.g., LocalFileSystem, LocalRunner, LocalDirAllocator, ContainerLocalizer) to signify intra-node scope. Calling these File* matches the implementation, but that's incidental to these being volumes owned/managed by this DN. That said, this convention isn't used broadly in HDFS yet AFAIK, so if there's a better term we could use it.

          • I really like the ReplicaBuilder pattern in this patch. It makes the code easier to read. Breaking up the switch statement in build() avoids a checkstyle complaint? This might be more legible if each (fairly long) case stmt were in a private method.
          • Is DataStorage::getTrashDirectoryForBlockFile now unused, except by tests? BlockPoolSliceStorage::getTrashDirectory(File) could be inlined into the new call, since the direct, File APIs are vestigial.
          • It looks like most of the NativeIO calls are preserved, largely through LocalReplica. The design here looks clean. In this section:
            -      Storage.nativeCopyFileUnbuffered(srcFile, dstFile, true);
            +      // create date of the source replica is not longer preserved!
            +      copyFileBuffered(srcReplica.getDataInputStream(0),
            +          srcReplica.getBlockDataLength(), dstFile);
                 } catch (IOException e) {
            -      throw new IOException("Failed to copy " + srcFile + " to " + dstFile, e);
            +      throw new IOException("Failed to copy " + srcReplica + " block file to "
            +          + dstFile, e);
            

            the call to Storage::nativeCopyFileUnbuffered is removed, so there's no path to the NativeIO libraries. This should be maintained.

          Show
          chris.douglas Chris Douglas added a comment - Fixed some findbugs, checkstyle. Joe Pallas generally I agree on using Local* , but it does match a convention in Hadoop (e.g., LocalFileSystem , LocalRunner , LocalDirAllocator , ContainerLocalizer ) to signify intra-node scope. Calling these File* matches the implementation, but that's incidental to these being volumes owned/managed by this DN. That said, this convention isn't used broadly in HDFS yet AFAIK, so if there's a better term we could use it. I really like the ReplicaBuilder pattern in this patch. It makes the code easier to read. Breaking up the switch statement in build() avoids a checkstyle complaint? This might be more legible if each (fairly long) case stmt were in a private method. Is DataStorage::getTrashDirectoryForBlockFile now unused, except by tests? BlockPoolSliceStorage::getTrashDirectory(File) could be inlined into the new call, since the direct, File APIs are vestigial. It looks like most of the NativeIO calls are preserved, largely through LocalReplica . The design here looks clean. In this section: - Storage.nativeCopyFileUnbuffered(srcFile, dstFile, true); + // create date of the source replica is not longer preserved! + copyFileBuffered(srcReplica.getDataInputStream(0), + srcReplica.getBlockDataLength(), dstFile); } catch (IOException e) { - throw new IOException("Failed to copy " + srcFile + " to " + dstFile, e); + throw new IOException("Failed to copy " + srcReplica + " block file to " + + dstFile, e); the call to Storage::nativeCopyFileUnbuffered is removed, so there's no path to the NativeIO libraries. This should be maintained.
          Hide
          virajith Virajith Jalaparti added a comment -

          Thanks Chris Douglas for the comments and fixing the checkstyle/findbugs.

          I am attaching a new patch to address the above comments.

          Breaking up the switch statement in build() avoids a checkstyle complaint? This might be more legible if each (fairly long) case stmt were in a private method.

          Addressed in the patch – moved each case statement to a private function.

          Is DataStorage::getTrashDirectoryForBlockFile now unused, except by tests?

          Yes, it is no longer used/required. I modified the tests that access DataStorage::getTrashDirectoryForBlockFile, to ensure that it is no longer needed and removed this function. I also modified tests that access BlockPoolSliceStorage::getTrashDirectory(File) to access BlockPoolSliceStorage::getTrashDirectory(ReplicaInfo), and made the former a private function in BlockPoolSliceStorage.

          the call to Storage::nativeCopyFileUnbuffered is removed, so there's no path to the NativeIO libraries. This should be maintained.

          Agree. I have restored this by adding two new abstract functions to ReplicaInfo: copyMetadata and copyBlockdata, and implementing them in LocalReplica to use Storage::nativeCopyFileUnbuffered.

          Show
          virajith Virajith Jalaparti added a comment - Thanks Chris Douglas for the comments and fixing the checkstyle/findbugs. I am attaching a new patch to address the above comments. Breaking up the switch statement in build() avoids a checkstyle complaint? This might be more legible if each (fairly long) case stmt were in a private method. Addressed in the patch – moved each case statement to a private function. Is DataStorage::getTrashDirectoryForBlockFile now unused, except by tests? Yes, it is no longer used/required. I modified the tests that access DataStorage::getTrashDirectoryForBlockFile , to ensure that it is no longer needed and removed this function. I also modified tests that access BlockPoolSliceStorage::getTrashDirectory(File) to access BlockPoolSliceStorage::getTrashDirectory(ReplicaInfo) , and made the former a private function in BlockPoolSliceStorage . the call to Storage::nativeCopyFileUnbuffered is removed, so there's no path to the NativeIO libraries. This should be maintained. Agree. I have restored this by adding two new abstract functions to ReplicaInfo : copyMetadata and copyBlockdata , and implementing them in LocalReplica to use Storage::nativeCopyFileUnbuffered .
          Hide
          eddyxu Lei (Eddy) Xu added a comment -

          Hi, Virajith Jalaparti

          Thanks for working on this. A few questions.

          • In FsDatasetImpl#fetchReplication , please remove the usage LOG.info.
          • In ReplicaInfo.java. The following comments seem irrelevant to the code below.
            /** directory where block & meta files belong. */
            

            Also please do not remove the comments for the constroctors.

          • breakHardlinksIfNeeded, {{ copyMetadata}} and copyBlockdata should not be in ReplicaInfo. Or should not use File as input.
          • ReplicaUnderRecovery. Is there a way to avoid casting ReplicaInfo to LocalReplica?
          • In general, there are places in this patch that return ReplicaInfo for FinalizedReplica. which would makes type system weaker and is not future-proof. Is it necessary to be changed?

          Thanks.

          Show
          eddyxu Lei (Eddy) Xu added a comment - Hi, Virajith Jalaparti Thanks for working on this. A few questions. In FsDatasetImpl#fetchReplication , please remove the usage LOG.info . In ReplicaInfo.java . The following comments seem irrelevant to the code below. /** directory where block & meta files belong. */ Also please do not remove the comments for the constroctors. breakHardlinksIfNeeded , {{ copyMetadata}} and copyBlockdata should not be in ReplicaInfo . Or should not use File as input. ReplicaUnderRecovery . Is there a way to avoid casting ReplicaInfo to LocalReplica ? In general, there are places in this patch that return ReplicaInfo for FinalizedReplica . which would makes type system weaker and is not future-proof. Is it necessary to be changed? Thanks.
          Hide
          virajith Virajith Jalaparti added a comment -

          Hi Lei (Eddy) Xu,

          Thank you for the comments.

          I agree with points 1 and 2, and will fix them.

          breakHardlinksIfNeeded, copyMetadata and copyBlockdata should not be in ReplicaInfo. Or should not use File as input.

          Agree that copyMetadata and copyBlockdata should not have File as a parameter. I will change it to URI to be more general. breakHardLinksIfNeeded has always been in ReplicaInfo. I made it abstract and moved the implementation to LocalReplica.

          ReplicaUnderRecovery. Is there a way to avoid casting ReplicaInfo to LocalReplica?

          The only place where the fact that original is a LocalReplica matters is in ReplicaUnderRecovery::setDir(). One way to address this would be to add the cast only when original.setDir() is called. The other way to deal with this would be to add setDir to ReplicaInfo but to avoid File as a parameter, it should take in an URI. Which do you think is better?

          In general, there are places in this patch that return ReplicaInfo for FinalizedReplica. which would makes type system weaker and is not future-proof. Is it necessary to be changed?

          This was intentional. The way I was thinking about it was that the state of ReplicaInfo should be known using ReplicaInfo::getState(), and not using the type system. The current code does the latter – it uses the type system to ensure that replicas are in a certain state. Not relying on the type system and using the former (use ReplicaInfo::getState()) seems a cleaner way of doing this. What do you think? Also, FinalizedReplica in the current type hierarchy, is a LocalReplica. So, referring to replicas using FinalizedReplica assumes that they are LocalReplica and hence, backed by File s.

          Show
          virajith Virajith Jalaparti added a comment - Hi Lei (Eddy) Xu , Thank you for the comments. I agree with points 1 and 2, and will fix them. breakHardlinksIfNeeded , copyMetadata and copyBlockdata should not be in ReplicaInfo . Or should not use File as input. Agree that copyMetadata and copyBlockdata should not have File as a parameter. I will change it to URI to be more general. breakHardLinksIfNeeded has always been in ReplicaInfo . I made it abstract and moved the implementation to LocalReplica . ReplicaUnderRecovery . Is there a way to avoid casting ReplicaInfo to LocalReplica ? The only place where the fact that original is a LocalReplica matters is in ReplicaUnderRecovery::setDir() . One way to address this would be to add the cast only when original.setDir() is called. The other way to deal with this would be to add setDir to ReplicaInfo but to avoid File as a parameter, it should take in an URI. Which do you think is better? In general, there are places in this patch that return ReplicaInfo for FinalizedReplica . which would makes type system weaker and is not future-proof. Is it necessary to be changed? This was intentional. The way I was thinking about it was that the state of ReplicaInfo should be known using ReplicaInfo::getState() , and not using the type system. The current code does the latter – it uses the type system to ensure that replicas are in a certain state. Not relying on the type system and using the former (use ReplicaInfo::getState() ) seems a cleaner way of doing this. What do you think? Also, FinalizedReplica in the current type hierarchy, is a LocalReplica . So, referring to replicas using FinalizedReplica assumes that they are LocalReplica and hence, backed by File s.
          Hide
          eddyxu Lei (Eddy) Xu added a comment -

          Hi, Virajith Jalaparti

          Thanks a lot for update the patch and address the comments. And appreciate much for taking our comments into consideration.

          The other way to deal with this would be to add setDir to ReplicaInfo but to avoid File as a parameter.

          What I am worried about is that, this constraint might not hold true for wider audience. E.g., If you implement recovery logic in Azure or S3, the input should not be a File based replica.

          it should take in an URI. Which do you think is better?

          Maybe using StorageLocation as input? There was some efforts to abstract File to StorageLocation.

          FinalizedReplica in the current type hierarchy, is a LocalReplica.

          Would that still be the case for Azure? I have saw a few cases that it is not a file-based solution. Also, FinalizedReplica should be the most common class in the ReplicaInfo hierarchy, so I guess many vendors might expect using FinalizedReplica to directly represent the block data.

          The way I was thinking about it was that the state of ReplicaInfo should be known using ReplicaInfo::getState(), and not using the type system.

          I don't have strong option against the solution proposed here. The concerns of getting rid of type system and using

          {int}

          /

          {enum}

          to indicate the class type might make the other developer difficult to have good confident to write correct code, because like dynamic language as python/ruby, the bugs become hard to spot during compile time. If we can come up a good test coverage, the concern can be overcome to some extend.

          Thanks.

          Show
          eddyxu Lei (Eddy) Xu added a comment - Hi, Virajith Jalaparti Thanks a lot for update the patch and address the comments. And appreciate much for taking our comments into consideration. The other way to deal with this would be to add setDir to ReplicaInfo but to avoid File as a parameter. What I am worried about is that, this constraint might not hold true for wider audience. E.g., If you implement recovery logic in Azure or S3, the input should not be a File based replica. it should take in an URI. Which do you think is better? Maybe using StorageLocation as input? There was some efforts to abstract File to StorageLocation . FinalizedReplica in the current type hierarchy, is a LocalReplica. Would that still be the case for Azure? I have saw a few cases that it is not a file-based solution. Also, FinalizedReplica should be the most common class in the ReplicaInfo hierarchy, so I guess many vendors might expect using FinalizedReplica to directly represent the block data. The way I was thinking about it was that the state of ReplicaInfo should be known using ReplicaInfo::getState(), and not using the type system. I don't have strong option against the solution proposed here. The concerns of getting rid of type system and using {int} / {enum} to indicate the class type might make the other developer difficult to have good confident to write correct code, because like dynamic language as python/ruby, the bugs become hard to spot during compile time. If we can come up a good test coverage, the concern can be overcome to some extend. Thanks.
          Hide
          virajith Virajith Jalaparti added a comment -

          Hi Lei (Eddy) Xu,

          E.g., If you implement recovery logic in Azure or S3, the input should not be a File based replica.

          Good point. I agree with this. Having StorageLocation instead of File makes sense (other parts of the patch aim to already do this). I will make the necessary changes.

          Would that still be the case for Azure?

          For HDFS-9806, the idea would be to have a ProvidedReplica which will be used to refer to data in external storages. As shown in HDFS-10675, this class would implement ReplicaInfo and ReplicaInPipeline, so that we can re-use the existing code for building replication pipelines and go through the block life cycle.

          so I guess many vendors might expect using FinalizedReplica to directly represent the block data.

          Are you suggesting this in the context of HDFS-9806? If so, I think they should be using ProvidedReplica (as mentioned in my previous point). If not, are there existing cases where vendors might be using FinalizedReplica for non-File backed data?

          One way to deal with this issue with FinalizedReplica and the usage of class types is to have abstract classes for each replica type, and then have implementations for local and provided replicas. This will end up having a lot of classes which are essentially ReplicaInfo. Having a good test coverage seems a cleaner way to solve this. Thoughts?

          Show
          virajith Virajith Jalaparti added a comment - Hi Lei (Eddy) Xu , E.g., If you implement recovery logic in Azure or S3, the input should not be a File based replica. Good point. I agree with this. Having StorageLocation instead of File makes sense (other parts of the patch aim to already do this). I will make the necessary changes. Would that still be the case for Azure? For HDFS-9806 , the idea would be to have a ProvidedReplica which will be used to refer to data in external storages. As shown in HDFS-10675 , this class would implement ReplicaInfo and ReplicaInPipeline , so that we can re-use the existing code for building replication pipelines and go through the block life cycle. so I guess many vendors might expect using FinalizedReplica to directly represent the block data. Are you suggesting this in the context of HDFS-9806 ? If so, I think they should be using ProvidedReplica (as mentioned in my previous point). If not, are there existing cases where vendors might be using FinalizedReplica for non- File backed data? One way to deal with this issue with FinalizedReplica and the usage of class types is to have abstract classes for each replica type, and then have implementations for local and provided replicas. This will end up having a lot of classes which are essentially ReplicaInfo . Having a good test coverage seems a cleaner way to solve this. Thoughts?
          Hide
          eddyxu Lei (Eddy) Xu added a comment - - edited

          If not, are there existing cases where vendors might be using FinalizedReplica for non-File backed data?

          Yea. I know vendors who have already implemented similar solutions based on FinalizedReplica, because ProvidedReplica did not exist back to the days. But I think it might be OK for them if this patch did not break their code.

          Another option might be that, "Finalized" looks like a state in the pipeline state machine, instead of a "Location" where the replica is stored, IMO. Have you considered about making LocalReplica and ProvidedReplica being subclasses of FinalizedReplica? So that for the pipeline state machine part of code, all the existing code can re-use the same FinalizedReplica as what it is today. Would love to hear your thoughts about it.

          Having a good test coverage seems a cleaner way to solve this.

          Agree. That would be a nice thing to do.

          Show
          eddyxu Lei (Eddy) Xu added a comment - - edited If not, are there existing cases where vendors might be using FinalizedReplica for non-File backed data? Yea. I know vendors who have already implemented similar solutions based on FinalizedReplica , because ProvidedReplica did not exist back to the days. But I think it might be OK for them if this patch did not break their code. Another option might be that, "Finalized" looks like a state in the pipeline state machine, instead of a "Location" where the replica is stored, IMO. Have you considered about making LocalReplica and ProvidedReplica being subclasses of FinalizedReplica ? So that for the pipeline state machine part of code, all the existing code can re-use the same FinalizedReplica as what it is today. Would love to hear your thoughts about it. Having a good test coverage seems a cleaner way to solve this. Agree. That would be a nice thing to do.
          Hide
          virajith Virajith Jalaparti added a comment -

          Hi Lei (Eddy) Xu,

          I know vendors who have already implemented similar solutions based on FinalizedReplica.

          I am trying to understand the use case for FinalizedReplica you mentioned. What properties of FinalizedReplica do you think we should preserve for these implementations to continue working? This question is also related to the following.

          Have you considered about making LocalReplica and ProvidedReplica being subclasses of FinalizedReplica?

          FinalizedReplica, as implemented today, does not have any particular characteristics other than it's state and type. There do not exist functions in FinalizedReplica that are not in ReplicaInfo. Is the goal of your proposal to have FinalizedReplica used as a type, and not just state? Do you see having to do this for other states as well (RUR, RBW, RWR, TEMPORARY)? or having it for FinalizedReplica is more important?

          One concrete proposal for this can be the following: FinalizedReplica will be an abstract subclass of ReplicaInfo (overrides ReplicaInfo::getState()). We can then define LocalFinalizedReplica and ProvidedFinalizedReplica, which can be subclasses of FinalizedReplica. However, this would cause LocalFinalizedReplica to re-implement the functionality of other {{LocalReplica}}s.
          One way to avoid such re-implementation could be to have FinalizedReplica as an interface, with a FinalizedReplica::getReplicaInfo() function to perform all the ReplicaInfo related functions. Then LocalFinalizedReplica can subclass LocalReplica and implement the FinalizedReplica interface.

          Show
          virajith Virajith Jalaparti added a comment - Hi Lei (Eddy) Xu , I know vendors who have already implemented similar solutions based on FinalizedReplica . I am trying to understand the use case for FinalizedReplica you mentioned. What properties of FinalizedReplica do you think we should preserve for these implementations to continue working? This question is also related to the following. Have you considered about making LocalReplica and ProvidedReplica being subclasses of FinalizedReplica ? FinalizedReplica , as implemented today, does not have any particular characteristics other than it's state and type. There do not exist functions in FinalizedReplica that are not in ReplicaInfo . Is the goal of your proposal to have FinalizedReplica used as a type, and not just state? Do you see having to do this for other states as well (RUR, RBW, RWR, TEMPORARY)? or having it for FinalizedReplica is more important? One concrete proposal for this can be the following: FinalizedReplica will be an abstract subclass of ReplicaInfo (overrides ReplicaInfo::getState() ). We can then define LocalFinalizedReplica and ProvidedFinalizedReplica , which can be subclasses of FinalizedReplica . However, this would cause LocalFinalizedReplica to re-implement the functionality of other {{LocalReplica}}s. One way to avoid such re-implementation could be to have FinalizedReplica as an interface, with a FinalizedReplica::getReplicaInfo() function to perform all the ReplicaInfo related functions. Then LocalFinalizedReplica can subclass LocalReplica and implement the FinalizedReplica interface.
          Hide
          jpallas Joe Pallas added a comment -

          Possibly the only requirement to subclass FinalizedReplica today is that it appears in the dataset SPI, so any alternative implementation has to use it. The proposal removes that, so maybe there isn't a strong reason to have it be a type (if I understand the distinction I think Virajith Jalaparti is making between class as type and class as state). There are several places in the current patch where FinalizedReplica becomes ReplicaInfo, and I think that's a reasonable change.

          The concern Lei (Eddy) Xu expressed above about states and types is valid, but, in this case, I think the type system isn't really helping the programmer very much. Existing code using FinalizedReplica is already frequently stuck with only ReplicaInfo known at compile time, so a state check is needed anyway, and using a cast to FinalizedReplica after the state check doesn't add much.

          Show
          jpallas Joe Pallas added a comment - Possibly the only requirement to subclass FinalizedReplica today is that it appears in the dataset SPI, so any alternative implementation has to use it. The proposal removes that, so maybe there isn't a strong reason to have it be a type (if I understand the distinction I think Virajith Jalaparti is making between class as type and class as state). There are several places in the current patch where FinalizedReplica becomes ReplicaInfo , and I think that's a reasonable change. The concern Lei (Eddy) Xu expressed above about states and types is valid, but, in this case, I think the type system isn't really helping the programmer very much. Existing code using FinalizedReplica is already frequently stuck with only ReplicaInfo known at compile time, so a state check is needed anyway, and using a cast to FinalizedReplica after the state check doesn't add much.
          Hide
          virajith Virajith Jalaparti added a comment -

          Posting an updated patch based on Lei (Eddy) Xu's comments (also rebased on most recent trunk).

          Show
          virajith Virajith Jalaparti added a comment - Posting an updated patch based on Lei (Eddy) Xu 's comments (also rebased on most recent trunk).
          Hide
          virajith Virajith Jalaparti added a comment -

          Modified patch to address a few failing test cases.

          Show
          virajith Virajith Jalaparti added a comment - Modified patch to address a few failing test cases.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 22s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 15 new or modified test files.
          +1 mvninstall 7m 18s trunk passed
          +1 compile 0m 47s trunk passed
          +1 checkstyle 0m 37s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 47s trunk passed
          +1 javadoc 0m 58s trunk passed
          +1 mvninstall 0m 50s the patch passed
          +1 compile 0m 45s the patch passed
          +1 javac 0m 45s the patch passed
          -0 checkstyle 0m 34s hadoop-hdfs-project/hadoop-hdfs: The patch generated 25 new + 856 unchanged - 94 fixed = 881 total (was 950)
          +1 mvnsite 0m 52s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          -1 whitespace 0m 0s The patch 1 line(s) with tabs.
          +1 findbugs 1m 53s the patch passed
          +1 javadoc 0m 55s the patch passed
          -1 unit 73m 43s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          94m 26s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.namenode.ha.TestDNFencing



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10636
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12826989/HDFS-10636.007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 3e2ad5721046 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f6c0b75
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16650/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/16650/artifact/patchprocess/whitespace-eol.txt
          whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/16650/artifact/patchprocess/whitespace-tabs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16650/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16650/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16650/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 15 new or modified test files. +1 mvninstall 7m 18s trunk passed +1 compile 0m 47s trunk passed +1 checkstyle 0m 37s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 47s trunk passed +1 javadoc 0m 58s trunk passed +1 mvninstall 0m 50s the patch passed +1 compile 0m 45s the patch passed +1 javac 0m 45s the patch passed -0 checkstyle 0m 34s hadoop-hdfs-project/hadoop-hdfs: The patch generated 25 new + 856 unchanged - 94 fixed = 881 total (was 950) +1 mvnsite 0m 52s the patch passed +1 mvneclipse 0m 10s the patch passed -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply -1 whitespace 0m 0s The patch 1 line(s) with tabs. +1 findbugs 1m 53s the patch passed +1 javadoc 0m 55s the patch passed -1 unit 73m 43s hadoop-hdfs in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 94m 26s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.ha.TestDNFencing Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10636 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12826989/HDFS-10636.007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 3e2ad5721046 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f6c0b75 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16650/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/16650/artifact/patchprocess/whitespace-eol.txt whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/16650/artifact/patchprocess/whitespace-tabs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/16650/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16650/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16650/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          eddyxu Lei (Eddy) Xu added a comment -

          Hi, Virajith Jalaparti, Thanks for the updates. And thanks for Joe Pallas's inputs.

          I have a few questions remained:

          • Should LocalReplica and ProvidedReplica extend FinalizedReplica? It seems the opposite way in the current patch. Similarly concerns for ReplicaBeingWritten and LocalReplicaBeingWritten.
          • ReplicaBuilder#buildLocalReplicaInPipeline can be private.

          The rest looks good to me.

          Show
          eddyxu Lei (Eddy) Xu added a comment - Hi, Virajith Jalaparti , Thanks for the updates. And thanks for Joe Pallas 's inputs. I have a few questions remained: Should LocalReplica and ProvidedReplica extend FinalizedReplica ? It seems the opposite way in the current patch. Similarly concerns for ReplicaBeingWritten and LocalReplicaBeingWritten . ReplicaBuilder#buildLocalReplicaInPipeline can be private. The rest looks good to me.
          Hide
          virajith Virajith Jalaparti added a comment -

          Hi Lei (Eddy) Xu,

          Should LocalReplica and ProvidedReplica extend FinalizedReplica?

          Could you take a look at my previous comment about this? I had a few questions as to what would be the motivation for this.

          ReplicaBuilder#buildLocalReplicaInPipeline can be private.

          This will not be sufficient as the function is used in FsVolumeImpl.

          Show
          virajith Virajith Jalaparti added a comment - Hi Lei (Eddy) Xu , Should LocalReplica and ProvidedReplica extend FinalizedReplica ? Could you take a look at my previous comment about this? I had a few questions as to what would be the motivation for this. ReplicaBuilder#buildLocalReplicaInPipeline can be private. This will not be sufficient as the function is used in FsVolumeImpl .
          Hide
          virajith Virajith Jalaparti added a comment -

          Modified patch fixes failing test case in last Jenkins run.

          Show
          virajith Virajith Jalaparti added a comment - Modified patch fixes failing test case in last Jenkins run.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 15 new or modified test files.
          +1 mvninstall 8m 8s trunk passed
          +1 compile 1m 0s trunk passed
          +1 checkstyle 0m 39s trunk passed
          +1 mvnsite 1m 9s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 54s trunk passed
          +1 javadoc 0m 58s trunk passed
          +1 mvninstall 0m 55s the patch passed
          +1 compile 0m 53s the patch passed
          +1 javac 0m 53s the patch passed
          -0 checkstyle 0m 36s hadoop-hdfs-project/hadoop-hdfs: The patch generated 23 new + 856 unchanged - 94 fixed = 879 total (was 950)
          +1 mvnsite 0m 50s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 48s the patch passed
          +1 javadoc 0m 53s the patch passed
          -1 unit 79m 39s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          101m 49s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.namenode.ha.TestStandbyCheckpoints
            hadoop.hdfs.server.blockmanagement.TestRBWBlockInvalidation



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10636
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827463/HDFS-10636.008.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 2b335f745e06 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / d355573
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16663/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16663/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16663/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16663/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 15 new or modified test files. +1 mvninstall 8m 8s trunk passed +1 compile 1m 0s trunk passed +1 checkstyle 0m 39s trunk passed +1 mvnsite 1m 9s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 54s trunk passed +1 javadoc 0m 58s trunk passed +1 mvninstall 0m 55s the patch passed +1 compile 0m 53s the patch passed +1 javac 0m 53s the patch passed -0 checkstyle 0m 36s hadoop-hdfs-project/hadoop-hdfs: The patch generated 23 new + 856 unchanged - 94 fixed = 879 total (was 950) +1 mvnsite 0m 50s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 48s the patch passed +1 javadoc 0m 53s the patch passed -1 unit 79m 39s hadoop-hdfs in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 101m 49s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.ha.TestStandbyCheckpoints   hadoop.hdfs.server.blockmanagement.TestRBWBlockInvalidation Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10636 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827463/HDFS-10636.008.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 2b335f745e06 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / d355573 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16663/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/16663/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16663/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16663/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 15 new or modified test files.
          +1 mvninstall 8m 55s trunk passed
          +1 compile 0m 53s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 1m 6s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 2m 10s trunk passed
          +1 javadoc 1m 2s trunk passed
          -1 mvninstall 0m 34s hadoop-hdfs in the patch failed.
          -1 compile 0m 34s hadoop-hdfs in the patch failed.
          -1 javac 0m 34s hadoop-hdfs in the patch failed.
          -0 checkstyle 0m 38s hadoop-hdfs-project/hadoop-hdfs: The patch generated 23 new + 856 unchanged - 94 fixed = 879 total (was 950)
          -1 mvnsite 0m 35s hadoop-hdfs in the patch failed.
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 0m 29s hadoop-hdfs in the patch failed.
          +1 javadoc 0m 54s the patch passed
          -1 unit 0m 26s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          21m 25s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10636
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827685/HDFS-10636.009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 7fa7e1d0fd5b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 011f3b2
          Default Java 1.8.0_101
          findbugs v3.0.0
          mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-mvninstall-hadoop-hdfs-project_hadoop-hdfs.txt
          compile https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-compile-hadoop-hdfs-project_hadoop-hdfs.txt
          javac https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-compile-hadoop-hdfs-project_hadoop-hdfs.txt
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          mvnsite https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-mvnsite-hadoop-hdfs-project_hadoop-hdfs.txt
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-findbugs-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16684/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16684/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 15 new or modified test files. +1 mvninstall 8m 55s trunk passed +1 compile 0m 53s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 1m 6s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 2m 10s trunk passed +1 javadoc 1m 2s trunk passed -1 mvninstall 0m 34s hadoop-hdfs in the patch failed. -1 compile 0m 34s hadoop-hdfs in the patch failed. -1 javac 0m 34s hadoop-hdfs in the patch failed. -0 checkstyle 0m 38s hadoop-hdfs-project/hadoop-hdfs: The patch generated 23 new + 856 unchanged - 94 fixed = 879 total (was 950) -1 mvnsite 0m 35s hadoop-hdfs in the patch failed. +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 0m 29s hadoop-hdfs in the patch failed. +1 javadoc 0m 54s the patch passed -1 unit 0m 26s hadoop-hdfs in the patch failed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 21m 25s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10636 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827685/HDFS-10636.009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7fa7e1d0fd5b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 011f3b2 Default Java 1.8.0_101 findbugs v3.0.0 mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-mvninstall-hadoop-hdfs-project_hadoop-hdfs.txt compile https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-compile-hadoop-hdfs-project_hadoop-hdfs.txt javac https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-compile-hadoop-hdfs-project_hadoop-hdfs.txt checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt mvnsite https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-mvnsite-hadoop-hdfs-project_hadoop-hdfs.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-findbugs-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/16684/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16684/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16684/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          virajith Virajith Jalaparti added a comment -

          Fixed issues with FsDatasetImpl#LOG in earlier patch.

          Show
          virajith Virajith Jalaparti added a comment - Fixed issues with FsDatasetImpl#LOG in earlier patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 24s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 15 new or modified test files.
          +1 mvninstall 7m 40s trunk passed
          +1 compile 0m 53s trunk passed
          +1 checkstyle 0m 43s trunk passed
          +1 mvnsite 0m 57s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 53s trunk passed
          +1 javadoc 1m 2s trunk passed
          +1 mvninstall 0m 53s the patch passed
          +1 compile 0m 49s the patch passed
          +1 javac 0m 49s the patch passed
          -0 checkstyle 0m 38s hadoop-hdfs-project/hadoop-hdfs: The patch generated 23 new + 856 unchanged - 94 fixed = 879 total (was 950)
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 2m 2s the patch passed
          +1 javadoc 1m 0s the patch passed
          -1 unit 76m 23s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          98m 40s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestRollingUpgrade



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10636
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827840/HDFS-10636.010.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c5c352b23e5c 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / cba973f
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16694/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16694/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16694/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16694/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 24s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 15 new or modified test files. +1 mvninstall 7m 40s trunk passed +1 compile 0m 53s trunk passed +1 checkstyle 0m 43s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 53s trunk passed +1 javadoc 1m 2s trunk passed +1 mvninstall 0m 53s the patch passed +1 compile 0m 49s the patch passed +1 javac 0m 49s the patch passed -0 checkstyle 0m 38s hadoop-hdfs-project/hadoop-hdfs: The patch generated 23 new + 856 unchanged - 94 fixed = 879 total (was 950) +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 2s the patch passed +1 javadoc 1m 0s the patch passed -1 unit 76m 23s hadoop-hdfs in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 98m 40s Reason Tests Failed junit tests hadoop.hdfs.TestRollingUpgrade Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10636 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827840/HDFS-10636.010.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c5c352b23e5c 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / cba973f Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16694/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/16694/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16694/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16694/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          virajith Virajith Jalaparti added a comment -

          The failed test case seems like a spurious failure (it runs fine on my dev machine). The patch does not affect that code path.

          Show
          virajith Virajith Jalaparti added a comment - The failed test case seems like a spurious failure (it runs fine on my dev machine). The patch does not affect that code path.
          Hide
          chris.douglas Chris Douglas added a comment -

          The latest version of the patch lgtm, +1. Thanks Virajith Jalaparti for seeing this through.

          Lei (Eddy) Xu do you have any other feedback on v10 before commit?

          Show
          chris.douglas Chris Douglas added a comment - The latest version of the patch lgtm, +1. Thanks Virajith Jalaparti for seeing this through. Lei (Eddy) Xu do you have any other feedback on v10 before commit?
          Hide
          eddyxu Lei (Eddy) Xu added a comment -

          Sorry for late reply, Christoph Sturm and Virajith Jalaparti

          I think that my previous thoughts on types might put too much constraints and make things difficult. Apologize for that.

          This patch itself looks OK to me. +0. for the reason described below:

          abstract public class LocalReplica extends ReplicaInfo {
          }
          
          // and 
          
          public class FinalizedReplica extends LocalReplica {
          }
          

          My concern is that in the future, where in the class hierarchy is ProvidedFinalizedReplica, should it inherent from FinalizedReplica or LocalReplica? In such cases, is ProvidedFinalizedReplica a LocalReplica?

          Show
          eddyxu Lei (Eddy) Xu added a comment - Sorry for late reply, Christoph Sturm and Virajith Jalaparti I think that my previous thoughts on types might put too much constraints and make things difficult. Apologize for that. This patch itself looks OK to me. +0. for the reason described below: abstract public class LocalReplica extends ReplicaInfo { } // and public class FinalizedReplica extends LocalReplica { } My concern is that in the future, where in the class hierarchy is ProvidedFinalizedReplica , should it inherent from FinalizedReplica or LocalReplica ? In such cases, is ProvidedFinalizedReplica a LocalReplica ?
          Hide
          virajith Virajith Jalaparti added a comment -

          Hi Lei (Eddy) Xu,
          That is a very valid concern. Our solution is to have a ProvidedReplica class which inherits ReplicaInfo, and then have ProvidedFinalizedReplica inherit ProvidedReplica. Not only this, we will need to have a hierarchy for ProvidedReplica that mirrors the hierarchy for LocalReplica in the patch. This is somewhat forced on us as different classes are used to implicitly implement the replica state machine (for example, ReplicaInfo::getBytesOnDisk() must resolve correctly in FsDatasetImpl). Thus, we will also need to have ProvidedReplicaInPipeline, ProvidedReplicaUnderRecovery and others which inherit ProvidedReplica.

          One way to avoid implementing such shadow classes for ProvidedReplica would be to explicitly implement a state machine for the replicas. This would be a much more disruptive change, with significant amount of new code and changes required to several parts of FsDatasetImpl, and FsVolumeImpl. The current patch keeps most of the code paths intact with relatively minor changes to the APIs of ReplicaInfo.

          Show
          virajith Virajith Jalaparti added a comment - Hi Lei (Eddy) Xu , That is a very valid concern. Our solution is to have a ProvidedReplica class which inherits ReplicaInfo , and then have ProvidedFinalizedReplica inherit ProvidedReplica . Not only this, we will need to have a hierarchy for ProvidedReplica that mirrors the hierarchy for LocalReplica in the patch. This is somewhat forced on us as different classes are used to implicitly implement the replica state machine (for example, ReplicaInfo::getBytesOnDisk() must resolve correctly in FsDatasetImpl ). Thus, we will also need to have ProvidedReplicaInPipeline , ProvidedReplicaUnderRecovery and others which inherit ProvidedReplica . One way to avoid implementing such shadow classes for ProvidedReplica would be to explicitly implement a state machine for the replicas. This would be a much more disruptive change, with significant amount of new code and changes required to several parts of FsDatasetImpl , and FsVolumeImpl . The current patch keeps most of the code paths intact with relatively minor changes to the APIs of ReplicaInfo .
          Hide
          eddyxu Lei (Eddy) Xu added a comment -

          Hi, Virajith Jalaparti Thanks for the insights.

          Our solution is to have a ProvidedReplica class which inherits ReplicaInfo, and then have ProvidedFinalizedReplica inherit ProvidedReplica.

          It makes sense now.

          +1. Will commit shortly

          Show
          eddyxu Lei (Eddy) Xu added a comment - Hi, Virajith Jalaparti Thanks for the insights. Our solution is to have a ProvidedReplica class which inherits ReplicaInfo, and then have ProvidedFinalizedReplica inherit ProvidedReplica. It makes sense now. +1. Will commit shortly
          Hide
          eddyxu Lei (Eddy) Xu added a comment -

          Committed to trunk.

          Show
          eddyxu Lei (Eddy) Xu added a comment - Committed to trunk.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10438 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10438/)
          HDFS-10636. Modify ReplicaInfo to remove the assumption that replica (lei: rev 86c9862bec0248d671e657aa56094a2919b8ac14)

          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/RamDiskAsyncLazyPersistService.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNodeFaultInjector.java
          • (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaBuilder.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/TestExternalDataset.java
          • (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/LocalReplicaInPipeline.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaBeingWritten.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestCrcCorruption.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockPoolSliceStorage.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaUnderRecovery.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/ExternalReplicaInPipeline.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DirectoryScanner.java
          • (delete) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaInPipelineInterface.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeRollingUpgrade.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestTransferRbw.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaInfo.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/FinalizedReplica.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/BlockPoolSlice.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestWriteToReplica.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockPoolSliceStorage.java
          • (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/LocalReplica.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImplTestUtils.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetUtil.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestClientProtocolForPipelineRecovery.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetAsyncDiskService.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaHandler.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaInPipeline.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDirectoryScanner.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/FsDatasetSpi.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsVolumeList.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockSender.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/ExternalDatasetImpl.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockReceiver.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsVolumeImpl.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaWaitingToBeRecovered.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10438 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10438/ ) HDFS-10636 . Modify ReplicaInfo to remove the assumption that replica (lei: rev 86c9862bec0248d671e657aa56094a2919b8ac14) (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockRecovery.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/RamDiskAsyncLazyPersistService.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNodeFaultInjector.java (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaBuilder.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/TestExternalDataset.java (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/LocalReplicaInPipeline.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaBeingWritten.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestCrcCorruption.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockPoolSliceStorage.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaUnderRecovery.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/ExternalReplicaInPipeline.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DirectoryScanner.java (delete) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaInPipelineInterface.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeRollingUpgrade.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestTransferRbw.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaInfo.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/FinalizedReplica.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/BlockPoolSlice.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetTestUtil.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestWriteToReplica.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockPoolSliceStorage.java (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/LocalReplica.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImplTestUtils.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetUtil.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestClientProtocolForPipelineRecovery.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetAsyncDiskService.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaHandler.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaInPipeline.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDirectoryScanner.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/FsDatasetSpi.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsVolumeList.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockSender.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/ExternalDatasetImpl.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockReceiver.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsVolumeImpl.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ReplicaWaitingToBeRecovered.java
          Hide
          virajith Virajith Jalaparti added a comment -

          Great! Thanks Lei (Eddy) Xu.

          Show
          virajith Virajith Jalaparti added a comment - Great! Thanks Lei (Eddy) Xu .
          Hide
          andrew.wang Andrew Wang added a comment -

          Question, is this JIRA actually incompatible? I did a quick skim, and it only seems to change things in the DN that are unannotated (thus defacto Private) or annotated Private.

          Show
          andrew.wang Andrew Wang added a comment - Question, is this JIRA actually incompatible? I did a quick skim, and it only seems to change things in the DN that are unannotated (thus defacto Private) or annotated Private.
          Hide
          virajith Virajith Jalaparti added a comment -

          Hi Andrew Wang, Yes, you are right in that all changes are to the Private APIs in the datanode. However, Lei (Eddy) Xu mentioned that some vendors might have implemented solutions based on FinalizedReplica (see this comment). I think this was the reason the JIRA was marked incompatible.

          Show
          virajith Virajith Jalaparti added a comment - Hi Andrew Wang , Yes, you are right in that all changes are to the Private APIs in the datanode. However, Lei (Eddy) Xu mentioned that some vendors might have implemented solutions based on FinalizedReplica (see this comment). I think this was the reason the JIRA was marked incompatible.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks Virajith Jalaparti! Lei (Eddy) Xu could you add a release note to this JIRA explaining the potential incompatibility?

          Show
          andrew.wang Andrew Wang added a comment - Thanks Virajith Jalaparti ! Lei (Eddy) Xu could you add a release note to this JIRA explaining the potential incompatibility?

            People

            • Assignee:
              virajith Virajith Jalaparti
              Reporter:
              virajith Virajith Jalaparti
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development