HDDS-4064. Show container verbose info with verbose option #1290
Conversation
xiaoyuyao
left a comment
xiaoyuyao
left a comment
There was a problem hiding this comment.
LGTM overall. Just few minor comments inline...
| @Parameters(description = "Decimal id of the container.") | ||
| private long containerID; | ||
|
|
||
| @CommandLine.Option(names = {"--verbose"}, |
There was a problem hiding this comment.
Can we leverage the existing "--verbose" option defined in parent class GenericCli here?
There was a problem hiding this comment.
boolean verbose = false;
if (parent.getParent() instanceof GenericCli) {
if (((GenericCli) parent.getParent()).isVerbose()) {
verbose = true;
}
}Do you mean that i should add this code to L66
There was a problem hiding this comment.
Note that parent --verbose also enables exception stack trace logging in case of failures. Thus for non-existent container, we would not only get the error message, but the complete stack trace:
$ ozone admin --verbose container info 1
org.apache.hadoop.ipc.RemoteException(org.apache.hadoop.hdds.scm.container.ContainerNotFoundException): Container with id #1 not found.
at org.apache.hadoop.hdds.scm.container.states.ContainerStateMap.checkIfContainerExist(ContainerStateMap.java:542)
at org.apache.hadoop.hdds.scm.container.states.ContainerStateMap.getContainerInfo(ContainerStateMap.java:188)
at org.apache.hadoop.hdds.scm.container.ContainerStateManager.getContainer(ContainerStateManager.java:499)
at org.apache.hadoop.hdds.scm.container.SCMContainerManager.getContainer(SCMContainerManager.java:201)
at org.apache.hadoop.hdds.scm.server.SCMClientProtocolServer.getContainerWithPipelineCommon(SCMClientProtocolServer.java:227)
at org.apache.hadoop.hdds.scm.server.SCMClientProtocolServer.getContainerWithPipeline(SCMClientProtocolServer.java:267)
at org.apache.hadoop.hdds.scm.protocol.StorageContainerLocationProtocolServerSideTranslatorPB.getContainerWithPipeline(StorageContainerLocationProtocolServerSideTranslatorPB.java:307)
at org.apache.hadoop.hdds.scm.protocol.StorageContainerLocationProtocolServerSideTranslatorPB.processRequest(StorageContainerLocationProtocolServerSideTranslatorPB.java:154)
at org.apache.hadoop.hdds.server.OzoneProtocolMessageDispatcher.processRequest(OzoneProtocolMessageDispatcher.java:74)
at org.apache.hadoop.hdds.scm.protocol.StorageContainerLocationProtocolServerSideTranslatorPB.submitRequest(StorageContainerLocationProtocolServerSideTranslatorPB.java:128)
at org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos$StorageContainerLocationProtocolService$2.callBlockingMethod(StorageContainerLocationProtocolProtos.java:35691)
at org.apache.hadoop.ipc.ProtobufRpcEngine$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine.java:528)
at org.apache.hadoop.ipc.RPC$Server.call(RPC.java:1070)
at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:999)
at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:927)
at java.base/java.security.AccessController.doPrivileged(Native Method)
at java.base/javax.security.auth.Subject.doAs(Subject.java:423)
at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1730)
at org.apache.hadoop.ipc.Server$Handler.run(Server.java:2915)
at org.apache.hadoop.ipc.Client.getRpcResponse(Client.java:1545)
at org.apache.hadoop.ipc.Client.call(Client.java:1491)
at org.apache.hadoop.ipc.Client.call(Client.java:1388)
at org.apache.hadoop.ipc.ProtobufRpcEngine$Invoker.invoke(ProtobufRpcEngine.java:233)
at org.apache.hadoop.ipc.ProtobufRpcEngine$Invoker.invoke(ProtobufRpcEngine.java:118)
at com.sun.proxy.$Proxy15.submitRequest(Unknown Source)
at org.apache.hadoop.hdds.scm.protocolPB.StorageContainerLocationProtocolClientSideTranslatorPB.submitRpcRequest(StorageContainerLocationProtocolClientSideTranslatorPB.java:130)
at org.apache.hadoop.hdds.scm.protocolPB.StorageContainerLocationProtocolClientSideTranslatorPB.submitRequest(StorageContainerLocationProtocolClientSideTranslatorPB.java:121)
at org.apache.hadoop.hdds.scm.protocolPB.StorageContainerLocationProtocolClientSideTranslatorPB.getContainerWithPipeline(StorageContainerLocationProtocolClientSideTranslatorPB.java:195)
at org.apache.hadoop.hdds.scm.cli.ContainerOperationClient.getContainerWithPipeline(ContainerOperationClient.java:440)
at org.apache.hadoop.hdds.scm.cli.container.InfoSubcommand.call(InfoSubcommand.java:61)
at org.apache.hadoop.hdds.scm.cli.container.InfoSubcommand.call(InfoSubcommand.java:41)
at picocli.CommandLine.execute(CommandLine.java:1173)
at picocli.CommandLine.access$800(CommandLine.java:141)
at picocli.CommandLine$RunLast.handle(CommandLine.java:1367)
at picocli.CommandLine$RunLast.handle(CommandLine.java:1335)
at picocli.CommandLine$AbstractParseResultHandler.handleParseResult(CommandLine.java:1243)
at picocli.CommandLine.parseWithHandlers(CommandLine.java:1526)
at picocli.CommandLine.parseWithHandler(CommandLine.java:1465)
at org.apache.hadoop.hdds.cli.GenericCli.execute(GenericCli.java:96)
at org.apache.hadoop.hdds.cli.GenericCli.run(GenericCli.java:87)
at org.apache.hadoop.ozone.admin.OzoneAdmin.main(OzoneAdmin.java:96)
In case you still want to leverage it, I suggest this:
boolean verbose = parent.getParent() instanceof GenericParentCommand &&
((GenericParentCommand) parent.getParent()).isVerbose();
In addition to being simpler, it also uses the GenericParentCommand interface, which defines isVerbose(), instead of the concrete GenericCli class.
There was a problem hiding this comment.
I agree uses GenericParentCommand seems better. Also, I would suggest we change printError to printStackTrace only if there is no meaningful error message? This way, we will have a clean verbose output as long as the error message is meaningful.
There was a problem hiding this comment.
@adoroszlai Thanks for your tips, I commit a new PR, PTAL.
There was a problem hiding this comment.
Also, I would suggest we change printError to printStackTrace only if there is no meaningful error message? This way, we will have a clean verbose output as long as the error message is meaningful.
Good idea @xiaoyuyao. I think we can split this to a separate improvement issue, so I created HDDS-4160.
There was a problem hiding this comment.
Ping @xiaoyuyao.
Please let me know if you are happy with this approach.
|
Sorry @maobaolong, my recent change for HDDS-4056 caused a conflict. I went ahead and resolved it for you. I also added an acceptance test case for the verbose mode. |
|
@adoroszlai Thanks for resolve the conflict for me. I check the failed misc acceptance test, which caused by |
|
@adoroszlai Please take another look. |
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @maobaolong for updating the patch.
@xiaoyuyao can you please take another look?
|
LGTM, +1. I will merge it shortly. |
|
@adoroszlai @xiaoyuyao @elek Thanks for your review. |
* master: (47 commits) HDDS-4104. Provide a way to get the default value and key of java-based-configuration easily (apache#1369) HDDS-4250. Fix wrong logger name (apache#1429) HDDS-4244. Container deleted wrong replica cause mis-replicated. (apache#1423) HDDS-4053. Volume space: add quotaUsageInBytes and update it when write and delete key. (apache#1296) HDDS-4210. ResolveBucket during checkAcls fails. (apache#1398) HDDS-4075. Retry request on different OM on AccessControlException (apache#1303) HDDS-4166. Documentation index page redirects to the wrong address (apache#1372) HDDS-4039. Reduce the number of fields in hdds.proto to improve performance (apache#1289) HDDS-4155. Directory and filename can end up with same name in a path. (apache#1361) HDDS-3927. Rename Ozone OM,DN,SCM runtime options to conform to naming conventions (apache#1401) HDDS-4119. Improve performance of the BufferPool management of Ozone client (apache#1336) HDDS-4217.Remove test TestOzoneContainerRatis (apache#1408) HDDS-4218.Remove test TestRatisManager (apache#1409) HDDS-4129. change MAX_QUOTA_IN_BYTES to Long.MAX_VALUE. (apache#1337) HDDS-4228: add field 'num' to ALLOCATE_BLOCK of scm audit log. (apache#1413) HDDS-4196. Add an endpoint in Recon to query Prometheus (apache#1390) HDDS-4211. [OFS] Better owner and group display for listing Ozone volumes and buckets (apache#1397) HDDS-4150. recon.api.TestEndpoints test is flaky (apache#1396) HDDS-4170 - Fix typo in method description. (apache#1406) HDDS-4064. Show container verbose info with verbose option (apache#1290) ...
4 participants
What changes were proposed in this pull request?
Show container verbose info with verbose option
What is the link to the Apache JIRA
HDDS-4064
How was this patch tested?
bash-4.2$ ozone admin container info 1 --verbose Container id: 1 Pipeline Info: Pipeline[ Id: 63fe01d7-45a0-42b9-ad4c-05993a8d7998, Nodes: e8c01f98-4355-43a8-9158-de658e9b62f2{ip: 172.22.0.8, host: ozone_datanode_2.ozone_default, networkLocation: /default-rack, certSerialId: null}c1455dca-5c03-48ac-a1ef-64ceb00b9cb9{ip: 172.22.0.2, host: ozone_datanode_1.ozone_default, networkLocation: /default-rack, certSerialId: null}aaa5575f-37c7-485f-bb5c-cae2b1fa52cd{ip: 172.22.0.7, host: ozone_datanode_3.ozone_default, networkLocation: /default-rack, certSerialId: null}, Type:RATIS, Factor:THREE, State:OPEN, leaderId:e8c01f98-4355-43a8-9158-de658e9b62f2, CreationTimestamp2020-08-05T09:35:25.102Z] Container State: OPEN Datanodes: [e8c01f98-4355-43a8-9158-de658e9b62f2/ozone_datanode_2.ozone_default, c1455dca-5c03-48ac-a1ef-64ceb00b9cb9/ozone_datanode_1.ozone_default, aaa5575f-37c7-485f-bb5c-cae2b1fa52cd/ozone_datanode_3.ozone_default]