[MINOR] improvement(server) Add context to rpc audit log to output necessary context #2088
Conversation
Test Results 2 840 files - 50 2 840 suites - 50 5h 32m 0s ⏱️ - 24m 35s For more details on these errors, see this check. Results for commit 5681cf6. ± Comparison against base commit 3b981ed. ♻️ This comment has been updated with latest results. |
maobaolong
force-pushed
the
auditContext
branch
from
c2f14a4 to
a651b0b
Compare
maobaolong
force-pushed
the
auditContext
branch
from
a651b0b to
0414673
Compare
| if (context == null) { | ||
| context = contextPart; | ||
| } else { | ||
| this.context += ", " + contextPart; |
There was a problem hiding this comment.
This looks strange, from my thought, If want to record the whole up/downstream context tips. Maybe we need to introduce the dedicated Context class to record the all children context?
There was a problem hiding this comment.
Like this?
class Context {
string contextName;
List<Context> children;
}
| getBlockIdsByPartitionId(requestPartitions, bitmap, res, blockIdLayout); | ||
| RpcAuditContext.getRpcAuditContext() | ||
| .ifPresent( |
There was a problem hiding this comment.
ifPresent is a good practise, because this avoids unnessary cpu cost if having the much string operations.
| @@ -906,7 +908,7 @@ public void getShuffleResultForMultiPart( | |||
|
|
|||
| auditContext.withAppId(appId).withShuffleId(shuffleId); | |||
| auditContext.withArgs( | |||
| "partitionsListSize=" + partitionsList.size() + ", blockIdLayout=" + blockIdLayout); | |||
| "partitionsList=" + partitionsList + ", blockIdLayout=" + blockIdLayout); | |||
There was a problem hiding this comment.
Concern that the partitionsList will cost too much time. Right?
| @@ -18,24 +18,33 @@ | |||
| package org.apache.uniffle.common.audit; | |||
|
|
|||
| import java.io.Closeable; | |||
| import java.util.Optional; | |||
|
|
|||
| import org.slf4j.Logger; | |||
|
|
|||
| import org.apache.uniffle.common.rpc.StatusCode; | |||
|
|
|||
| /** Context for rpc audit logging. */ | |||
| public abstract class RpcAuditContext implements Closeable { | |||
There was a problem hiding this comment.
If having performance degression, can we disable this by config? @maobaolong I see some time consuming operations in the hotspot path.
There was a problem hiding this comment.
Yes, we can disable the rpc audit log by rss.server.rpc.audit.log.enabled and rss.coordinator.rpc.audit.log.enabled
What changes were proposed in this pull request?
(Please outline the changes and how this PR fixes the issue.)
Why are the changes needed?
(Please clarify why the changes are needed. For instance,
Fix: # (issue)
Does this PR introduce any user-facing change?
(Please list the user-facing changes introduced by your change, including
No.
How was this patch tested?
(Please test your changes, and provide instructions on how to test it: