[jira] [Commented] (HADOOP-10376) Refactor refresh*Protocols into a single generic refreshConfigProtocol

["Arpit Agarwal (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HADOOP-10376?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14015753#comment-14015753 ] 

Arpit Agarwal commented on HADOOP-10376:
----------------------------------------

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?


> Refactor refresh*Protocols into a single generic refreshConfigProtocol
> ----------------------------------------------------------------------
>
>                 Key: HADOOP-10376
>                 URL: https://issues.apache.org/jira/browse/HADOOP-10376
>             Project: Hadoop Common
>          Issue Type: Improvement
>            Reporter: Chris Li
>            Assignee: Chris Li
>            Priority: Minor
>         Attachments: HADOOP-10376.patch, HADOOP-10376.patch, RefreshFrameworkProposal.pdf
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.2#6252)