HDDS-2240. Command line tool for OM HA. #1586
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
It will be better to use entrySet() instead of keySet() for performance as it would avoid the lookup on L61. Although this method will not be doing extensive amount of work, I believe it is still worth making this change.
There was a problem hiding this comment.
Declare this class as final.
There was a problem hiding this comment.
NIT: Have we named this command omha because we have plans in future to also add similar command for SCM?
If not, how about naming the command as haadmin , omadmin, admin.
Not a deal breaker but was just thinking if we can keep it similar to hdfs.
There was a problem hiding this comment.
Changed the command to "ozone admin om".
SCM HA is also in the pipeline sometime in the future.
As Marton also suggested, we can have all admin commands under this. This will include SCMCli also.
dineshchitlangia
left a comment
dineshchitlangia
left a comment
There was a problem hiding this comment.
Thanks @hanishakoneru for working on this.
Posted comments inline requesting changes.
Also, there are checkstyle/findbugs violation related to patch.
elek
left a comment
elek
left a comment
There was a problem hiding this comment.
Thanks the patch @hanishakoneru.
Overall it looks good to me, but I have usability suggestions (not strong opinions). And sorry if they seem to be bikeshedding.
-
Until now we put more and more admin commands to
ozone scmlcli. Recently we discussed to rename it to something more generic (ozone adminis suggested in HDDS-2184). If it will be a generic admin tool I think it would be better to put this helper to there. (ozone admin omha ....). -
As half of my life spent in linux command line, I would prefer to use the generic cli conventions (use
--long-parameterinstead of--longParameter(avoid camelCase) and use simplesubcommandfor the name of the action instead of--subcommand). In short: try to follow the convention ofgit.
Eg. use ozone admin om getservicestate --service-id
But it could be only my preference ;-)
- Recently I spent a little time to unify all the RPC interfaces to use the same pattern.
message OMRequest {
required Type cmdType = 1; // Type of the command
// A string that identifies this command, we generate Trace ID in Ozone
// frontend and this allows us to trace that command all over ozone.
optional string traceID = 2;
required string clientId = 3;
optional UserInfo userInfo = 4;
optional CreateVolumeRequest createVolumeRequest = 11;
optional SetVolumePropertyRequest setVolumePropertyRequest = 12;
optional CheckVolumeAccessRequest checkVolumeAccessRequest = 13;
optional InfoVolumeRequest infoVolumeRequest = 14;
I called it message based pattern: we have one method and the calls are separated by the message type (in proto2, in proto3 it will be nicer). I know that it's not the perfect approach (adding typesafe new method to the rpc service -- as you did -- has some visibility benefit), but this the convention which is started by a other proto files in ozone. I would prefer to follow a common pattern even if this is not the best one.
And if we use this convention the OzoneProtocolMessageDispatcher would provide logging, metrics, ozone-insight and opentracing support for free. Therefore I suggest to add new call as a new type to the OMRequest (submitRequest(OMRequest)).
- I guess your motivation was to separate the admin commands and client commands and I 100% agree with you. But currently in ozone scm the admin commands and client commands both in
ScmContainerLocationRequest. Which is not perfect, I agree. But for good separation (IMHO) we need two separated RPC service/port (one for the client one for the admin commands, and the port of the admin commands can be disabled/guarded).
So I suggest to put it to the OzoneProtocol.proto as part of the OMRequest (as a short term solution). Or create a new RPC server (OzoneManagerAdminProtocol) and follow the exisisting message based approach there.
(and sorry if my comments were too opinionated)
There was a problem hiding this comment.
What is the use case for this RPC? it is used only from the OM HA tool? or is some thing that clients need to know when communicating with the OM? if clients need to know this info, then clients already make a call called getServiceList and this info is more appropriate to be part of that call? Then both clients and tools like OM HA can get to that information pretty easily.
There was a problem hiding this comment.
Clients do not need this information.
I have added a new OMRequest to get the OM server roles. Did not add it to getServiceList api because getServiceList is called when creating RpcClient (so all clients make this call when starting up). But adding the server roles also to this api would mean one extra Ratis call to get the server roles which is not required by all clients. Hence, kept the two calls separate.
There was a problem hiding this comment.
Sorry did not see this comment. How can a client communicate to OM? Does it not need to send the request to the leader ?
There was a problem hiding this comment.
Also just wondering, is there a reason to add a new RPC and not use the submitRequet pattern -- that is add the message to the OMRequest / OMResponse pattern?
There was a problem hiding this comment.
You are right. There is no need for new RPC. I thought we could keep the admin protocols separate from the client protocols. But I think we can just use the OMRequest for now. Updated it.
|
Thank you very much to open this pull request. During the weekend the Ozone source code has been moved out from apache/hadoop repository to apache/hadoop-ozone repository. This git commits are rewritten, but the branch of this pull request is also transformed (state of Saturday morning), you can use the new, migrated branch to recreate this pull request. Your pull request is important for us: Can you please re-create your pull request in the new repository? 1. Create a new fork of https://github.com/apache/hadoop-ozone 2. Clone it and have both your fork and the apache repo as remotes: 3. Fetch your migrated branch and push it to your fork. 4. And create the new pull request on the new repository. If you need more information, please check this wiki page or contact with me (my github user name + apache.org). Thank you, and sorry for the inconvenience. |
hanishakoneru
force-pushed
the
HDDS-2240
branch
from
a241c07 to
626688d
Compare
|
@elek Thank you for your comments and suggestions.
|
|
Thank you @dineshchitlangia @elek and @anuengineer for the review and comments. |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks the migration @hanishakoneru (and BTW, thanks the update, I will check it). I am closing this one as we have the new one. |
5 participants
A command line tool (ozone omha) to get information related to OM HA.
This Jira proposes to add the getServiceState option for OM HA which lists all the OMs in the service and their corresponding Ratis server roles (LEADER/ FOLLOWER).
We can later add more options to this tool.