Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: HA branch (HDFS-1623)
    • Fix Version/s: HA branch (HDFS-1623)
    • Component/s: ha, hdfs-client
    • Labels:
      None

      Description

      Presentely ConfiguredFailoverProxyProvider supports ClinetProtocol.

      It should support NameNodeProtocol also, because Balancer uses NameNodeProtocol for getting blocks.

      1. HDFS-2767.patch
        23 kB
        Uma Maheswara Rao G
      2. HDFS-2767.patch
        22 kB
        Uma Maheswara Rao G
      3. HDFS-2767.patch
        9 kB
        Uma Maheswara Rao G
      4. hdfs-2767-what-todd-had.txt
        13 kB
        Todd Lipcon

        Issue Links

          Activity

          Uma Maheswara Rao G created issue -
          Todd Lipcon made changes -
          Field Original Value New Value
          Assignee Todd Lipcon [ tlipcon ]
          Uma Maheswara Rao G made changes -
          Attachment HDFS-2767.patch [ 12510137 ]
          Hide
          Uma Maheswara Rao G added a comment -

          Hi Todd, I worked on this as part of Balancer issue.
          Included common class FailoverProxyProvider interface also in this one.
          Added one setter method for setting the protocol and created the corresponding proxy instance based on protocol.

          Thanks
          Uma

          Show
          Uma Maheswara Rao G added a comment - Hi Todd, I worked on this as part of Balancer issue. Included common class FailoverProxyProvider interface also in this one. Added one setter method for setting the protocol and created the corresponding proxy instance based on protocol. Thanks Uma
          Hide
          Uma Maheswara Rao G added a comment -

          If you agree with this approach, i can just file on issue in common for FailoverProxyProvider interface method.
          If you have any other approach which works better, please suggest i can make the changes.

          Show
          Uma Maheswara Rao G added a comment - If you agree with this approach, i can just file on issue in common for FailoverProxyProvider interface method. If you have any other approach which works better, please suggest i can make the changes.
          Hide
          Todd Lipcon added a comment -

          Hi Uma. I had started working on this before you posted your patch, but looks like we went a similar direction. The only suggestion I have is to make the interface an argument of the constructor rather than calling a setter after it's instantiated. I'll upload what I have - do you think you could make that change in your patch?

          Also, regarding this section:

          +        // TODO(HA): Need other way to create the proxy instance based on
          +        // protocol here.
          +        if (protocol != null && NamenodeProtocol.class.equals(protocol)) {
          +          current.namenode = DFSUtil.createNamenodeWithNNProtocol(
          +              current.address, conf);
          +        } else {
          

          I think you can remove the TODO and change the else to an else if to check for ClientProtocol, with a final else clause that throws an AssertionError or IllegalStateException.

          Lastly, I think we do need to wire the ugi parameter in to createNamenodeWithNNProtocol or else after a failover the user accessing HDFS might accidentally switch!

          Show
          Todd Lipcon added a comment - Hi Uma. I had started working on this before you posted your patch, but looks like we went a similar direction. The only suggestion I have is to make the interface an argument of the constructor rather than calling a setter after it's instantiated. I'll upload what I have - do you think you could make that change in your patch? Also, regarding this section: + // TODO(HA): Need other way to create the proxy instance based on + // protocol here. + if (protocol != null && NamenodeProtocol.class.equals(protocol)) { + current.namenode = DFSUtil.createNamenodeWithNNProtocol( + current.address, conf); + } else { I think you can remove the TODO and change the else to an else if to check for ClientProtocol, with a final else clause that throws an AssertionError or IllegalStateException. Lastly, I think we do need to wire the ugi parameter in to createNamenodeWithNNProtocol or else after a failover the user accessing HDFS might accidentally switch!
          Todd Lipcon made changes -
          Attachment hdfs-2767-what-todd-had.txt [ 12510157 ]
          Todd Lipcon made changes -
          Link This issue blocks HDFS-2592 [ HDFS-2592 ]
          Hide
          Uma Maheswara Rao G added a comment -

          Thanks a lot Todd, for the comments.
          I will check and update accordingly. Mean.while can you please take a look on Balancer issue also?

          Show
          Uma Maheswara Rao G added a comment - Thanks a lot Todd, for the comments. I will check and update accordingly. Mean.while can you please take a look on Balancer issue also?
          Uma Maheswara Rao G made changes -
          Assignee Todd Lipcon [ tlipcon ] Uma Maheswara Rao G [ umamaheswararao ]
          Uma Maheswara Rao G made changes -
          Attachment HDFS-2767.patch [ 12510210 ]
          Hide
          Uma Maheswara Rao G added a comment -

          Todd, updated the patch by addressing your comments.

          Show
          Uma Maheswara Rao G added a comment - Todd, updated the patch by addressing your comments.
          Hide
          Todd Lipcon added a comment -
          +    } catch (RuntimeException rte) {
          +      throw rte;
          

          doesn't serve any purpose


          • The functions createFailoverProxyWithNamenodeProtocol and createFailoverProxyWithClientProtocol are identical except for the class they take, right? Why not just make it take the interface class as a third parameter?
          • Can you update NameNodeConnector.createNamenode to call createNNProxyWithNamenodeProtocol in this patch, just so we don't introduce new dup code here?

          Thanks!

          Show
          Todd Lipcon added a comment - + } catch (RuntimeException rte) { + throw rte; doesn't serve any purpose The functions createFailoverProxyWithNamenodeProtocol and createFailoverProxyWithClientProtocol are identical except for the class they take, right? Why not just make it take the interface class as a third parameter? Can you update NameNodeConnector.createNamenode to call createNNProxyWithNamenodeProtocol in this patch, just so we don't introduce new dup code here? Thanks!
          Hide
          Uma Maheswara Rao G added a comment -

          Todd, I just confused with the last point. I should include NameNodeConnector class in Balancer patch right?
          You have reviewed both patches together here?

          Show
          Uma Maheswara Rao G added a comment - Todd, I just confused with the last point. I should include NameNodeConnector class in Balancer patch right? You have reviewed both patches together here?
          Hide
          Todd Lipcon added a comment -

          I meant that, in this patch, you can make NameNodeConnector use the new API createNNProxyWithNamenodeProtocol - as it's just refactoring. Then, in the other patch, you can integrate it with the failover proxy creation.

          Show
          Todd Lipcon added a comment - I meant that, in this patch, you can make NameNodeConnector use the new API createNNProxyWithNamenodeProtocol - as it's just refactoring. Then, in the other patch, you can integrate it with the failover proxy creation.
          Uma Maheswara Rao G made changes -
          Attachment HDFS-2767.patch [ 12510784 ]
          Hide
          Uma Maheswara Rao G added a comment -

          Thanks Todd, for the clarification.

          + } catch (RuntimeException rte) {
          + throw rte;

          doesn't serve any purpose

          Oops..did not realize while merging from the patch.

          • The functions createFailoverProxyWithNamenodeProtocol and createFailoverProxyWithClientProtocol are identical except for the class they take, right? Why not just make it take the interface class as a third parameter?

          Done.

          Can you update NameNodeConnector.createNamenode to call createNNProxyWithNamenodeProtocol in this patch, just so we don't introduce new dup code here?

          Done.

          Thanks
          Uma

          Show
          Uma Maheswara Rao G added a comment - Thanks Todd, for the clarification. + } catch (RuntimeException rte) { + throw rte; doesn't serve any purpose Oops..did not realize while merging from the patch. The functions createFailoverProxyWithNamenodeProtocol and createFailoverProxyWithClientProtocol are identical except for the class they take, right? Why not just make it take the interface class as a third parameter? Done. Can you update NameNodeConnector.createNamenode to call createNNProxyWithNamenodeProtocol in this patch, just so we don't introduce new dup code here? Done. Thanks Uma
          Hide
          Todd Lipcon added a comment -

          Committed to branch, thanks Uma.

          Show
          Todd Lipcon added a comment - Committed to branch, thanks Uma.
          Todd Lipcon made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Target Version/s HA branch (HDFS-1623) [ 12317568 ]
          Fix Version/s HA branch (HDFS-1623) [ 12317568 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-HAbranch-build #50 (See https://builds.apache.org/job/Hadoop-Hdfs-HAbranch-build/50/)
          HDFS-2767. ConfiguredFailoverProxyProvider should support NameNodeProtocol. Contributed by Uma Maheswara Rao G.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1232284
          Files :

          • /hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/DefaultFailoverProxyProvider.java
          • /hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java
          • /hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
          • /hadoop/common/branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-1623.txt
          • /hadoop/common/branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • /hadoop/common/branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
          • /hadoop/common/branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
          • /hadoop/common/branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/balancer/NameNodeConnector.java
          • /hadoop/common/branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ConfiguredFailoverProxyProvider.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-HAbranch-build #50 (See https://builds.apache.org/job/Hadoop-Hdfs-HAbranch-build/50/ ) HDFS-2767 . ConfiguredFailoverProxyProvider should support NameNodeProtocol. Contributed by Uma Maheswara Rao G. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1232284 Files : /hadoop/common/branches/ HDFS-1623 /hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/DefaultFailoverProxyProvider.java /hadoop/common/branches/ HDFS-1623 /hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java /hadoop/common/branches/ HDFS-1623 /hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/branches/ HDFS-1623 /hadoop-hdfs-project/hadoop-hdfs/CHANGES. HDFS-1623 .txt /hadoop/common/branches/ HDFS-1623 /hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/common/branches/ HDFS-1623 /hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java /hadoop/common/branches/ HDFS-1623 /hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java /hadoop/common/branches/ HDFS-1623 /hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/balancer/NameNodeConnector.java /hadoop/common/branches/ HDFS-1623 /hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ConfiguredFailoverProxyProvider.java

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development