HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose #1214

maobaolong · 2020-07-17T16:56:03Z

What changes were proposed in this pull request?

When I profile the s3g performance, i find in some log of XceiverClientGrpc, I cannot find the related datanode ip, so i use datanode instead of the UUID and networkFullPath.

Also, i add a debug level log to show the request has been executed and output the try counts.

What is the link to the Apache JIRA

HDDS-3981

How was this patch tested?

No need, all changes only related to log.

…rpose

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

xiaoyuyao · 2020-07-28T18:42:42Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

@@ -339,6 +343,11 @@ private XceiverClientReply sendCommandWithRetry(
        // in case these don't exist for the specific datanode.
        reply.addDatanode(dn);
        responseProto = sendCommandAsync(request, dn).getResponse().get();
+        if (LOG.isDebugEnabled()) {


In sendCommandAsync() below we already have a LOG.debug(cmdType, Dn). Can you move this there to avoid duplicate debug log?

@xiaoyuyao I hope so, but I think it would be difficult, because, i want to log the retry index and cost time.
when the sendCommandAsync return back, the client still doesn't get the result from datanode. the result return until the get return.

If i have something wrong, please correct me.

I mean if we could move the LOG for latency around Line 464 where the container op latency metrics are updated. This way, we can piggyback LOG with the existing metrics update.

metrics.addContainerOpsLatency(request.getCmdType(),
System.nanoTime() - requestTime);

@xiaoyuyao Thanks you for your reply and suggestion. I tried to add the LOG into the L464, like following

public void onNext(ContainerCommandResponseProto value) { replyFuture.complete(value); metrics.decrPendingContainerOpsMetrics(request.getCmdType()); long cost = System.nanoTime() - requestTime; metrics.addContainerOpsLatency(request.getCmdType(), cost); if (LOG.isDebugEnabled()) { LOG.debug("Executed command {} on datanode {}, cost = {}, " + "retryCount = {}" + ", cmdType = {}", request, dn, cost, index, request.getCmdType()); } semaphore.release(); }

But, there are some question

It cannot print the Index which stand for the retried datanode index of the datanodeList.

I should add the same log logic to the onError also? Otherwise, when we met error, the onNext won't be executed.

As index can be calculate from the log context, I drop it.

xiaoyuyao

LGTM overall, a few comments inline...

maobaolong · 2020-07-30T03:02:17Z

@xiaoyuyao Thanks to review my PR, i reply to your comments, please check it again, thanks.

xiaoyuyao · 2020-08-04T01:41:12Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

@@ -433,7 +442,7 @@ public XceiverClientReply sendCommandAsync(
    UUID dnId = dn.getUuid();
    if (LOG.isDebugEnabled()) {
      LOG.debug("Send command {} to datanode {}",
-          request.getCmdType(), dn.getNetworkFullPath());
+          request.getCmdType(), dn);


Correct me if I'm wrong, I think we have already print the same debug message on Line 338-340 from the caller. Print just the IP address should be sufficient.

OK, you are right, the redundancy log content is useless, I've change it to dn.getIpAddress().

maobaolong · 2020-08-10T02:20:49Z

@xiaoyuyao Thank you for the review suggestion, PTAL.

elek · 2020-09-07T08:45:16Z

@bharatviswa504 What do we need to make it possible to merge this commit?

xiaoyuyao

Thanks @maobaolong for the update. One more comment added inline.

maobaolong · 2020-09-15T01:31:58Z

@xiaoyuyao Thanks @xiaoyuyao for the suggestion, I pushed a new commit to address your comment, I reply your comment and ask some question. PTAL.

xiaoyuyao · 2020-09-17T22:21:33Z

Thanks @maobaolong for the update. LGTM, +1. Will merge it shortly.

* master: HDDS-4102. Normalize Keypath for lookupKey. (apache#1328) HDDS-4263. ReplicatiomManager shouldn't consider origin node Id for CLOSED containers. (apache#1438) HDDS-4282. Improve the emptyDir syntax (apache#1450) HDDS-4194. Create a script to check AWS S3 compatibility (apache#1383) HDDS-4270. Add more reusable byteman scripts to debug ofs/o3fs performance (apache#1443) HDDS-2660. Create insight point for datanode container protocol (apache#1272) HDDS-3297. Enable TestOzoneClientKeyGenerator. (apache#1442) HDDS-4324. Add important comment to ListVolumes logic (apache#1417) HDDS-4236. Move "Om*Codec.java" to new project hadoop-ozone/interface-storage (apache#1424) HDDS-4254. Bucket space: add usedBytes and update it when create and delete key. (apache#1431) HDDS-2766. security/SecuringDataNodes.md (apache#1175) HDDS-4206. Attempt pipeline creation more frequently in acceptance tests (apache#1389) HDDS-4233. Interrupted exeception printed out from DatanodeStateMachine (apache#1416) HDDS-3947: Sort DNs for client when the key is a file for #getFileStatus #listStatus APIs (apache#1385) HDDS-3102. ozone getconf command should use the GenericCli parent class (apache#1410) HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose (apache#1214) HDDS-4255. Remove unused Ant and Jdiff dependency versions (apache#1433) HDDS-4247. Fixed log4j usage in some places (apache#1426) HDDS-4241. Support HADOOP_TOKEN_FILE_LOCATION for Ozone token CLI. (apache#1422)

* HDDS-4122-remove-code-consolidation: (21 commits) Restore files that had deduplicated code from master Revert other delete request/response files back to their original states on master HDDS-4102. Normalize Keypath for lookupKey. (apache#1328) HDDS-4263. ReplicatiomManager shouldn't consider origin node Id for CLOSED containers. (apache#1438) HDDS-4282. Improve the emptyDir syntax (apache#1450) HDDS-4194. Create a script to check AWS S3 compatibility (apache#1383) HDDS-4270. Add more reusable byteman scripts to debug ofs/o3fs performance (apache#1443) HDDS-2660. Create insight point for datanode container protocol (apache#1272) HDDS-3297. Enable TestOzoneClientKeyGenerator. (apache#1442) HDDS-4324. Add important comment to ListVolumes logic (apache#1417) HDDS-4236. Move "Om*Codec.java" to new project hadoop-ozone/interface-storage (apache#1424) HDDS-4254. Bucket space: add usedBytes and update it when create and delete key. (apache#1431) HDDS-2766. security/SecuringDataNodes.md (apache#1175) HDDS-4206. Attempt pipeline creation more frequently in acceptance tests (apache#1389) HDDS-4233. Interrupted exeception printed out from DatanodeStateMachine (apache#1416) HDDS-3947: Sort DNs for client when the key is a file for #getFileStatus #listStatus APIs (apache#1385) HDDS-3102. ozone getconf command should use the GenericCli parent class (apache#1410) HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose (apache#1214) HDDS-4255. Remove unused Ant and Jdiff dependency versions (apache#1433) HDDS-4247. Fixed log4j usage in some places (apache#1426) ...

maobaolong added 6 commits July 18, 2020 00:48

HDDS-3981. Add more debug level log to XceiverClientGrpc for debug pu…

0729631

…rpose

trigger new ci check

39a4825

trigger new ci check

a49773e

trigger new ci check

e1d7e9a

trigger new ci check

9e13176

trigger new ci check

6a3d7b3

xiaoyuyao reviewed Jul 28, 2020

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java Show resolved Hide resolved

xiaoyuyao reviewed Jul 28, 2020

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java Show resolved Hide resolved

xiaoyuyao reviewed Jul 28, 2020

View reviewed changes

xiaoyuyao reviewed Aug 4, 2020

View reviewed changes

Address comments, log the ip address o dn only.

6a7a413

maobaolong requested a review from xiaoyuyao August 11, 2020 08:14

xiaoyuyao reviewed Sep 10, 2020

View reviewed changes

Address comment, move the debug log into sendCommandAsync

0f027d8

maobaolong requested a review from xiaoyuyao September 15, 2020 01:33

xiaoyuyao merged commit 68d1ab0 into apache:master Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose #1214

HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose #1214

maobaolong commented Jul 17, 2020

xiaoyuyao Jul 28, 2020

maobaolong Jul 30, 2020

xiaoyuyao Sep 10, 2020

maobaolong Sep 15, 2020 •

edited

xiaoyuyao left a comment

maobaolong commented Jul 30, 2020

xiaoyuyao Aug 4, 2020

maobaolong Aug 10, 2020

maobaolong commented Aug 10, 2020

elek commented Sep 7, 2020

xiaoyuyao left a comment

maobaolong commented Sep 15, 2020

xiaoyuyao commented Sep 17, 2020

HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose #1214

HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose #1214

Conversation

maobaolong commented Jul 17, 2020

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

xiaoyuyao Jul 28, 2020

Choose a reason for hiding this comment

maobaolong Jul 30, 2020

Choose a reason for hiding this comment

xiaoyuyao Sep 10, 2020

Choose a reason for hiding this comment

maobaolong Sep 15, 2020 • edited

Choose a reason for hiding this comment

xiaoyuyao left a comment

Choose a reason for hiding this comment

maobaolong commented Jul 30, 2020

xiaoyuyao Aug 4, 2020

Choose a reason for hiding this comment

maobaolong Aug 10, 2020

Choose a reason for hiding this comment

maobaolong commented Aug 10, 2020

elek commented Sep 7, 2020

xiaoyuyao left a comment

Choose a reason for hiding this comment

maobaolong commented Sep 15, 2020

xiaoyuyao commented Sep 17, 2020

maobaolong Sep 15, 2020 •

edited