Skip to content

HDDS-4064. Show container verbose info with verbose option #1290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xiaoyuyao merged 8 commits into from
Sep 8, 2020
Merged

HDDS-4064. Show container verbose info with verbose option #1290

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
142947d
HDDS-4064. Show container verbose info with verbose option
maobaolong Aug 5, 2020
b175bc5
Use GenericParentCommand to get the verbose mode
maobaolong Aug 27, 2020
33c9d49
Use GenericParentCommand to get the verbose mode
maobaolong Aug 27, 2020
f4c0dda
Merge remote-tracking branch 'origin/master' into infocontainer
adoroszlai Aug 27, 2020
738c834
HDDS-4064. Add robot test for verbose mode
adoroszlai Aug 27, 2020
ee4b45c
HDDS-4064. Fix checkstyle
adoroszlai Aug 27, 2020
4dffd17
trigger new CI check
adoroszlai Aug 27, 2020
1d05504
Merge remote-tracking branch 'upstream' into infocontainer
maobaolong Aug 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.common.base.Preconditions;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import picocli.CommandLine;
import picocli.CommandLine.Command;
import picocli.CommandLine.Parameters;
import picocli.CommandLine.ParentCommand;
Expand All @@ -52,6 +53,10 @@ public class InfoSubcommand implements Callable<Void> {
@Parameters(description = "Decimal id of the container.")
private long containerID;

@CommandLine.Option(names = {"--verbose"},
Copy link
Contributor

@xiaoyuyao xiaoyuyao Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leverage the existing "--verbose" option defined in parent class GenericCli here?

Copy link
Member Author

@maobaolong maobaolong Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaoyuyao

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

Copy link
Contributor

@adoroszlai adoroszlai Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@xiaoyuyao xiaoyuyao Aug 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@maobaolong maobaolong Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adoroszlai Thanks for your tips, I commit a new PR, PTAL.

Copy link
Contributor

@adoroszlai adoroszlai Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@elek elek Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @xiaoyuyao.

Please let me know if you are happy with this approach.

description = "Show detailed info of the container.")
private boolean verbose;

@Override
public Void call() throws Exception {
try (ScmClient scmClient = parent.getParent().createScmClient()) {
Expand All @@ -61,7 +66,11 @@ public Void call() throws Exception {

// Print container report info.
LOG.info("Container id: {}", containerID);
LOG.info("Pipeline id: {}", container.getPipeline().getId().getId());
if (verbose) {
LOG.info("Pipeline Info: {}", container.getPipeline());
} else {
LOG.info("Pipeline id: {}", container.getPipeline().getId().getId());
}
LOG.info("Container State: {}", container.getContainerInfo().getState());

// Print pipeline of an existing container.
Expand Down