Hadoop Common
  1. Hadoop Common
  2. HADOOP-10376

Refactor refresh*Protocols into a single generic refreshConfigProtocol

    Details

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

      Description

      See https://issues.apache.org/jira/browse/HADOOP-10285

      There are starting to be too many refresh*Protocols We can refactor them to use a single protocol with a variable payload to choose what to do.

      Thereafter, we can return an indication of success or failure.

      1. HADOOP-10376.patch
        49 kB
        Chris Li
      2. HADOOP-10376.patch
        49 kB
        Chris Li
      3. HADOOP-10376.patch
        49 kB
        Chris Li
      4. HADOOP-10376.patch
        44 kB
        Chris Li
      5. HADOOP-10376.patch
        41 kB
        Chris Li
      6. RefreshFrameworkProposal.pdf
        63 kB
        Chris Li

        Issue Links

          Activity

          Hide
          Ming Ma added a comment -

          Interesting work. There is org.apache.hadoop.conf.Reconfigurable defined to support config update at run time. Should we deprecate that interface and provide reconfiguration based on RefreshHandler?

          Show
          Ming Ma added a comment - Interesting work. There is org.apache.hadoop.conf.Reconfigurable defined to support config update at run time. Should we deprecate that interface and provide reconfiguration based on RefreshHandler?
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1799 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1799/)
          HADOOP-10376. Refactor refresh*Protocols into a single generic refreshConfigProtocol. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602055)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/GenericRefreshProtocol.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshHandler.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshResponse.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolClientSideTranslatorPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolServerSideTranslatorPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/proto/GenericRefreshProtocol.proto
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HDFSPolicyProvider.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamenodeProtocols.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/TestGenericRefresh.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1799 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1799/ ) HADOOP-10376 . Refactor refresh*Protocols into a single generic refreshConfigProtocol. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602055 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/GenericRefreshProtocol.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshResponse.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolClientSideTranslatorPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolServerSideTranslatorPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/proto/GenericRefreshProtocol.proto /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HDFSPolicyProvider.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamenodeProtocols.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/TestGenericRefresh.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Hdfs-trunk #1772 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1772/)
          HADOOP-10376. Refactor refresh*Protocols into a single generic refreshConfigProtocol. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602055)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/GenericRefreshProtocol.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshHandler.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshResponse.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolClientSideTranslatorPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolServerSideTranslatorPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/proto/GenericRefreshProtocol.proto
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HDFSPolicyProvider.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamenodeProtocols.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/TestGenericRefresh.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1772 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1772/ ) HADOOP-10376 . Refactor refresh*Protocols into a single generic refreshConfigProtocol. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602055 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/GenericRefreshProtocol.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshResponse.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolClientSideTranslatorPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolServerSideTranslatorPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/proto/GenericRefreshProtocol.proto /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HDFSPolicyProvider.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamenodeProtocols.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/TestGenericRefresh.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Yarn-trunk #581 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/581/)
          HADOOP-10376. Refactor refresh*Protocols into a single generic refreshConfigProtocol. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602055)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/GenericRefreshProtocol.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshHandler.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshResponse.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolClientSideTranslatorPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolServerSideTranslatorPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/proto/GenericRefreshProtocol.proto
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HDFSPolicyProvider.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamenodeProtocols.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/TestGenericRefresh.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #581 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/581/ ) HADOOP-10376 . Refactor refresh*Protocols into a single generic refreshConfigProtocol. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602055 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/GenericRefreshProtocol.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshResponse.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolClientSideTranslatorPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolServerSideTranslatorPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/proto/GenericRefreshProtocol.proto /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HDFSPolicyProvider.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamenodeProtocols.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/TestGenericRefresh.java
          Hide
          Chris Li added a comment -

          Hmm, how about duplicating the functionality, then? So that users can begin to use the new command (which may uncover shortcomings and needed improvements).

          Show
          Chris Li added a comment - Hmm, how about duplicating the functionality, then? So that users can begin to use the new command (which may uncover shortcomings and needed improvements).
          Hide
          Arpit Agarwal added a comment -

          I don't think there is a immediate need to update the existing refresh calls since we cannot remove them in 2.x anyway for wire compatibility.

          However it is good to have the generic refresh facility for future use so we don't need to add more protocols.

          Show
          Arpit Agarwal added a comment - I don't think there is a immediate need to update the existing refresh calls since we cannot remove them in 2.x anyway for wire compatibility. However it is good to have the generic refresh facility for future use so we don't need to add more protocols.
          Hide
          Chris Li added a comment -

          Thanks for reviewing!

          I think the next step is to start moving other refreshProtos towards the generic proto, marking things as deprecated along the way. I'll start with RefreshCallQueue since it's the newest. HADOOP-10685 to track

          Show
          Chris Li added a comment - Thanks for reviewing! I think the next step is to start moving other refreshProtos towards the generic proto, marking things as deprecated along the way. I'll start with RefreshCallQueue since it's the newest. HADOOP-10685 to track
          Hide
          Arpit Agarwal added a comment -

          +1 for the latest patch.

          I committed it to trunk and branch-2. Thanks for the contribution Chris!

          Show
          Arpit Agarwal added a comment - +1 for the latest patch. I committed it to trunk and branch-2. Thanks for the contribution Chris!
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #5687 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5687/)
          HADOOP-10376. Refactor refresh*Protocols into a single generic refreshConfigProtocol. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602055)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/GenericRefreshProtocol.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshHandler.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshResponse.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolClientSideTranslatorPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolServerSideTranslatorPB.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/proto/GenericRefreshProtocol.proto
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HDFSPolicyProvider.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamenodeProtocols.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/TestGenericRefresh.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5687 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5687/ ) HADOOP-10376 . Refactor refresh*Protocols into a single generic refreshConfigProtocol. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602055 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/GenericRefreshProtocol.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RefreshResponse.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolClientSideTranslatorPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/protocolPB/GenericRefreshProtocolServerSideTranslatorPB.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/proto/GenericRefreshProtocol.proto /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HDFSPolicyProvider.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/NamenodeProtocols.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/TestGenericRefresh.java
          Hide
          Hadoop QA added a comment -

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

          +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 does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

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

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12649882/HADOOP-10376.patch against trunk revision . +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 does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4048//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4048//console This message is automatically generated.
          Hide
          Chris Li added a comment -

          Ah oops, I accidentally reverted that change with git on my end. Thanks for catching that. `identifier` should now be optional

          Show
          Chris Li added a comment - Ah oops, I accidentally reverted that change with git on my end. Thanks for catching that. `identifier` should now be optional
          Hide
          Arpit Agarwal added a comment - - edited

          Hi Chris,

          • GenericRefreshRequestProto.identifier is still required (should be optional). Looks unintentional since you added a check for hasIdentifier in the server side translator.

          +1 otherwise. Thanks for addressing the feedback.

          Show
          Arpit Agarwal added a comment - - edited Hi Chris, GenericRefreshRequestProto.identifier is still required (should be optional). Looks unintentional since you added a check for hasIdentifier in the server side translator. +1 otherwise. Thanks for addressing the feedback.
          Hide
          Hadoop QA added a comment -

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

          +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 does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

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

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12649678/HADOOP-10376.patch against trunk revision . +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 does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4041//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4041//console This message is automatically generated.
          Hide
          Chris Li added a comment -

          Benoy Antony good catch, made mutators synchronized. Also on name, I'm okay with RefreshHandlerRegistry if people think it's more clear. I do like that RefreshRegistry is concise though

          Zesheng Wu sounds like a good idea when we replace old refreshprotos in later patches with rewritten ones.

          Show
          Chris Li added a comment - Benoy Antony good catch, made mutators synchronized. Also on name, I'm okay with RefreshHandlerRegistry if people think it's more clear. I do like that RefreshRegistry is concise though Zesheng Wu sounds like a good idea when we replace old refreshprotos in later patches with rewritten ones.
          Hide
          Benoy Antony added a comment -

          Would RefreshHandlerRegistry be a better name than _ RefreshRegistry_ ?

          Show
          Benoy Antony added a comment - Would RefreshHandlerRegistry be a better name than _ RefreshRegistry_ ?
          Hide
          Benoy Antony added a comment -

          In class RefreshRegistry, the state could be updated/accessed by different threads and so you need to synchronize the mutators and accessors.

          Show
          Benoy Antony added a comment - In class RefreshRegistry , the state could be updated/accessed by different threads and so you need to synchronize the mutators and accessors.
          Hide
          Zesheng Wu added a comment -

          Hi Chris,
          The proposal and patch both look great to me, and fix my doubt of why namenode having so many refresh*Protocols.
          One more minor suggestion: should we mark the old refresh* functions as deprecated?

          Show
          Zesheng Wu added a comment - Hi Chris, The proposal and patch both look great to me, and fix my doubt of why namenode having so many refresh*Protocols. One more minor suggestion: should we mark the old refresh* functions as deprecated?
          Hide
          Hadoop QA added a comment -

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

          +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 does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

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

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12649493/HADOOP-10376.patch against trunk revision . +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 does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4036//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4036//console This message is automatically generated.
          Hide
          Chris Li added a comment -

          Uploaded new patch.

          I decided to add support for multi registration again. The reasoning is that this feature simplifies things by allowing any class to register as a refresh handler, even if it doesn't know about its server's port (eg what ID to register itself as), and the whole benefit to having a registry like this is to allow for a more decentralized approach.

          Show
          Chris Li added a comment - Uploaded new patch. I decided to add support for multi registration again. The reasoning is that this feature simplifies things by allowing any class to register as a refresh handler, even if it doesn't know about its server's port (eg what ID to register itself as), and the whole benefit to having a registry like this is to allow for a more decentralized approach.
          Hide
          Arpit Agarwal added a comment -

          Perhaps it can offer both? It would ask the user to provide the service to refresh as host:port, or Namenode/ResourceManager which would refresh on all the machines in HA or any future configurations such as a quorum. Maybe that's also another patch.

          Yes this sounds good long term but we can address it separately. For now it is okay to leave host:port as required.

          Show
          Arpit Agarwal added a comment - Perhaps it can offer both? It would ask the user to provide the service to refresh as host:port, or Namenode/ResourceManager which would refresh on all the machines in HA or any future configurations such as a quorum. Maybe that's also another patch. Yes this sounds good long term but we can address it separately. For now it is okay to leave host:port as required.
          Hide
          Chris Li added a comment -

          Hi Arapit,

          If we make this feature namenode only, it would remove the requirement to specify the target, but it would leave certain situations ambiguous, such as what should happen in the case of HA. I suppose it's a matter of philosophy, but I wanted to make this intentionally agnostic, so it would work regardless of what service is targeted, whether HA is enabled for RMs or NNs or other things are developed in the future.

          Perhaps it can offer both? It would ask the user to provide the service to refresh as host:port, or Namenode/ResourceManager which would refresh on all the machines in HA or any future configurations such as a quorum. Maybe that's also another patch.

          I'll also switch it back to 1-to-1 mappings, since the added complexity turns out to be too much after I tried it.

          Show
          Chris Li added a comment - Hi Arapit, If we make this feature namenode only, it would remove the requirement to specify the target, but it would leave certain situations ambiguous, such as what should happen in the case of HA. I suppose it's a matter of philosophy, but I wanted to make this intentionally agnostic, so it would work regardless of what service is targeted, whether HA is enabled for RMs or NNs or other things are developed in the future. Perhaps it can offer both? It would ask the user to provide the service to refresh as host:port, or Namenode/ResourceManager which would refresh on all the machines in HA or any future configurations such as a quorum. Maybe that's also another patch. I'll also switch it back to 1-to-1 mappings, since the added complexity turns out to be too much after I tried it.
          Hide
          Arpit Agarwal added a comment -

          Just to clarify I am also okay with leaving multiple handlers in place if the earlier feedback is addressed.

          Show
          Arpit Agarwal added a comment - Just to clarify I am also okay with leaving multiple handlers in place if the earlier feedback is addressed.
          Hide
          Arpit Agarwal added a comment -

          One use case of multiple handlers is for refreshing something on two servers running on two ports.

          For this use case it is not worth the added complexity. We can add it later if necessary but for now one handler/identifier sounds good to me.

          Not sure if it needs to be there, but other refresh calls are able to infer the host:port based on the protocol used (and the refresh protocols they use are specific to NN or DN) whereas a generic protocol would not.

          I don't think there are any existing refresh calls that target DNs.

          Show
          Arpit Agarwal added a comment - One use case of multiple handlers is for refreshing something on two servers running on two ports. For this use case it is not worth the added complexity. We can add it later if necessary but for now one handler/identifier sounds good to me. Not sure if it needs to be there, but other refresh calls are able to infer the host:port based on the protocol used (and the refresh protocols they use are specific to NN or DN) whereas a generic protocol would not. I don't think there are any existing refresh calls that target DNs.
          Hide
          Chris Li added a comment -

          Hi Arapit, thanks for the suggestions.

          Optional identifier sounds good.

          RefreshResponse:

          One use case of multiple handlers is for refreshing something on two servers running on two ports. We would ideally like to return a repeated custom message type which includes returnCode, userMessage, and senderName (which becomes important when you have multiple handlers). And then that opens another issue with how to have a user-readable senderName.

          Perhaps supporting multiple handlers isn't worth the hassle?

          RefreshRegistry:

          Changes sound good if we need to support multiple handlers.

          DFSAdmin:

          Args are sent as an array of strings, no post-processing done.

          Not sure if it needs to be there, but other refresh calls are able to infer the host:port based on the protocol used (and the refresh protocols they use are specific to NN or DN) whereas a generic protocol would not.

          -------

          It seems like a handful of these changes depend on whether or not it supports one identifier to many handler mappings. In the process the code gets more complex since we're now dealing with collections of responses.

          Let me know if the feature sounds useful, otherwise I can remove it to maintain simplicity.

          Show
          Chris Li added a comment - Hi Arapit, thanks for the suggestions. Optional identifier sounds good. RefreshResponse: One use case of multiple handlers is for refreshing something on two servers running on two ports. We would ideally like to return a repeated custom message type which includes returnCode, userMessage, and senderName (which becomes important when you have multiple handlers). And then that opens another issue with how to have a user-readable senderName. Perhaps supporting multiple handlers isn't worth the hassle? RefreshRegistry: Changes sound good if we need to support multiple handlers. DFSAdmin: Args are sent as an array of strings, no post-processing done. Not sure if it needs to be there, but other refresh calls are able to infer the host:port based on the protocol used (and the refresh protocols they use are specific to NN or DN) whereas a generic protocol would not. ------- It seems like a handful of these changes depend on whether or not it supports one identifier to many handler mappings. In the process the code gets more complex since we're now dealing with collections of responses. Let me know if the feature sounds useful, otherwise I can remove it to maintain simplicity.
          Hide
          Arpit Agarwal added a comment -

          Hi Chris, my comments below. The approach looks fine to me.

          GenericRefreshRequest:

          • Please consider making identifier optional. required can be a hassle when rev-ing interfaces.

          GenericRefreshProtocolServerSideTranslatorPB:

          • Once identifier is an optional field, please add a corresponding check for request.hasIdentifier. For now you can just throw an exception if no identifier was supplied.

          GenericRefreshProtocolClientSideTranslatorPB:

          • userMessage is optional, so check for its presence with resp.hasUserMessage? I realize the server will pass an empty string if the handler did not return a message but that is a detail the client doesn't need to know of.

          RefreshResponse:

          • Do you have a use case in mind for multiple handlers per identifier? If we need to pass multiple messages, instead of formatting the responses into a string on the server, why not pass each string to the client in the RPC response? i.e. in GenericRefreshProtocol.proto, make userMessage a repeated field. This lets the client choose the presentation.

          RefreshRegistry:

          • Consider using Guava MultiMap for handlerTable. If you want to continue using the ‘map of sets’ approach then the following should be fixed:
            • unregisterAll - Why not just remove identifier and its corresponding value?
            • getHandlers should return an unmodifiableSet. Also the return type should be an abstract Set.

          DFSAdmin:

          • All other args after are sent to the host - clarify they are sent as uninterpreted strings?
          • Does host:port need to be a required argument? None of the other refresh calls require it.

          TestGenericRefresh:

          • testMultipleHandlers - add verification that there was interaction with both mocks?
          Show
          Arpit Agarwal added a comment - Hi Chris, my comments below. The approach looks fine to me. GenericRefreshRequest: Please consider making identifier optional . required can be a hassle when rev-ing interfaces. GenericRefreshProtocolServerSideTranslatorPB: Once identifier is an optional field, please add a corresponding check for request.hasIdentifier . For now you can just throw an exception if no identifier was supplied. GenericRefreshProtocolClientSideTranslatorPB: userMessage is optional, so check for its presence with resp.hasUserMessage ? I realize the server will pass an empty string if the handler did not return a message but that is a detail the client doesn't need to know of. RefreshResponse: Do you have a use case in mind for multiple handlers per identifier? If we need to pass multiple messages, instead of formatting the responses into a string on the server, why not pass each string to the client in the RPC response? i.e. in GenericRefreshProtocol.proto, make userMessage a repeated field. This lets the client choose the presentation. RefreshRegistry: Consider using Guava MultiMap for handlerTable . If you want to continue using the ‘map of sets’ approach then the following should be fixed: unregisterAll - Why not just remove identifier and its corresponding value? getHandlers should return an unmodifiableSet. Also the return type should be an abstract Set. DFSAdmin: All other args after are sent to the host - clarify they are sent as uninterpreted strings? Does host:port need to be a required argument? None of the other refresh calls require it. TestGenericRefresh: testMultipleHandlers - add verification that there was interaction with both mocks?
          Hide
          Hadoop QA added a comment -

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

          +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 does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

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

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12647271/HADOOP-10376.patch against trunk revision . +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 does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3980//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3980//console This message is automatically generated.
          Hide
          Chris Li added a comment -

          Updated patch with support for many handlers mapping to a single identifier.

          Show
          Chris Li added a comment - Updated patch with support for many handlers mapping to a single identifier.
          Hide
          Chris Li added a comment -

          Hi Arpit Agarwal, sounds good. I went ahead and uploaded a patch.

          Most of it is pretty typical stuff for adding a new protocol (which shows how painful it is today), the interesting parts are the 3 new files: RefreshRegistry, RefreshHandler, RefreshResponse.

          A useful new capability is being able to send text and exit status to the user on success (today you can either return 0 and have no text, or throw an exception with a message and return -1)

          Authorization is coarse in this patch: users can be opted in or out of refreshing any of the registered refresh handlers. Future versions would allow more fine permissions.

          Show
          Chris Li added a comment - Hi Arpit Agarwal , sounds good. I went ahead and uploaded a patch. Most of it is pretty typical stuff for adding a new protocol (which shows how painful it is today), the interesting parts are the 3 new files: RefreshRegistry, RefreshHandler, RefreshResponse. A useful new capability is being able to send text and exit status to the user on success (today you can either return 0 and have no text, or throw an exception with a message and return -1) Authorization is coarse in this patch: users can be opted in or out of refreshing any of the registered refresh handlers. Future versions would allow more fine permissions.
          Hide
          Arpit Agarwal added a comment -

          Hi Chris, I see nothing objectionable in the proposal but it would be good to have more detail on the registry/dispatch mechanism.

          Alternatively you could just a patch.

          Show
          Arpit Agarwal added a comment - Hi Chris, I see nothing objectionable in the proposal but it would be good to have more detail on the registry/dispatch mechanism. Alternatively you could just a patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12642725/RefreshFrameworkProposal.pdf
          against trunk revision .

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

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3880//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12642725/RefreshFrameworkProposal.pdf against trunk revision . -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3880//console This message is automatically generated.
          Hide
          Chris Li added a comment -

          Attached brief proposal for this feature

          Show
          Chris Li added a comment - Attached brief proposal for this feature
          Hide
          Benoy Antony added a comment -

          Great idea. I assume that the goal is to enable refresh capability for a feature in a generic fashion.
          Based on our discussion, like to suggest the following approach for consideration.

          Create a RefreshProtocol which accepts a feature-id and a variable payload.
          A server which supports the protocol loads a set of classes which implements an interface , say RefreshHandler
          The RefreshHandler implementations are specified in Configuration.

          RefreshHandler.java
          public interface RefreshHandler {
          	
          	public String getId();
          	
          	public boolean refresh (String payload) ;
          
          }
          

          When handling the refresh requests, the server delegates it to matching RefreshHandler based on feature_id.
          The feature_id can also be a full class name in which case, the server loads the class and passes the payload.

          The client issues commands of the form hadoop dfsadmin -refresh feature-id arg1 arg2...

          Show
          Benoy Antony added a comment - Great idea. I assume that the goal is to enable refresh capability for a feature in a generic fashion. Based on our discussion, like to suggest the following approach for consideration. Create a RefreshProtocol which accepts a feature-id and a variable payload. A server which supports the protocol loads a set of classes which implements an interface , say RefreshHandler The RefreshHandler implementations are specified in Configuration. RefreshHandler.java public interface RefreshHandler { public String getId(); public boolean refresh ( String payload) ; } When handling the refresh requests, the server delegates it to matching RefreshHandler based on feature_id. The feature_id can also be a full class name in which case, the server loads the class and passes the payload. The client issues commands of the form hadoop dfsadmin -refresh feature-id arg1 arg2...
          Hide
          Hadoop QA added a comment -

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

          +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 patch appears to cause the build to fail.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3674//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12634636/HADOOP-10376.patch against trunk revision . +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 patch appears to cause the build to fail. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3674//console This message is automatically generated.
          Hide
          Chris Li added a comment -

          Attached my patch to create a generic refresh protocol.

          Motivation: currently there's too much duplication in the different refresh*Protocols, and the process of adding a new one involves making changes in 10+ files.

          There were a couple different ways I thought of doing this:
          Method 1: add multiple methods to the GenericRefreshProtocol interface
          Method 2: have an identifier passed in, and have the receiving objects dispatch accordingly

          Method 1:

          • Most type safe
          • Requires that each implementer implement all of its refresh methods (will basically limit this for use on the namenode)
          • Requires users to modify 3 additional files to add a new refresh item, adding refresh* methods

          Method 2

          • Receiver can make best effort to respond to requests to refresh
          • Users need only modify 1 additional file to add a new refresh item, user only needs to add a static String to the interface
          • Request types not compiler enforced

          The patch I've submitted uses method 2, preferring its flexibility and ease of adding new protocols.

          Show
          Chris Li added a comment - Attached my patch to create a generic refresh protocol. Motivation: currently there's too much duplication in the different refresh*Protocols, and the process of adding a new one involves making changes in 10+ files. There were a couple different ways I thought of doing this: Method 1: add multiple methods to the GenericRefreshProtocol interface Method 2: have an identifier passed in, and have the receiving objects dispatch accordingly Method 1: Most type safe Requires that each implementer implement all of its refresh methods (will basically limit this for use on the namenode) Requires users to modify 3 additional files to add a new refresh item, adding refresh* methods Method 2 Receiver can make best effort to respond to requests to refresh Users need only modify 1 additional file to add a new refresh item, user only needs to add a static String to the interface Request types not compiler enforced The patch I've submitted uses method 2, preferring its flexibility and ease of adding new protocols.

            People

            • Assignee:
              Chris Li
              Reporter:
              Chris Li
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development