zuston commented on code in PR #2088:
URL:
https://github.com/apache/incubator-uniffle/pull/2088#discussion_r1740359574
##########
common/src/main/java/org/apache/uniffle/common/audit/RpcAuditContext.java:
##########
@@ -119,6 +128,21 @@ public RpcAuditContext withFrom(String from) {
return this;
}
+ /**
+ * Sets context field, context can be concat by invoke multiply time.
+ *
+ * @param contextPart the new context part
+ * @return this {@link RpcAuditContext} instance
+ */
+ public RpcAuditContext withContext(String contextPart) {
+ if (context == null) {
+ context = contextPart;
+ } else {
+ this.context += ", " + contextPart;
Review 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?
##########
common/src/main/java/org/apache/uniffle/common/audit/RpcAuditContext.java:
##########
@@ -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 {
Review Comment:
If having performance degression, can we disable this by config? @maobaolong
I see some time consuming operations in the hotspot path.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -641,9 +642,20 @@ public byte[] getFinishedBlockIds(
Roaring64NavigableMap res = Roaring64NavigableMap.bitmapOf();
for (Map.Entry<Integer, Set<Integer>> entry :
bitmapIndexToPartitions.entrySet()) {
Set<Integer> requestPartitions = entry.getValue();
- Roaring64NavigableMap bitmap = blockIds[entry.getKey()];
+ int bitmapIndex = entry.getKey();
+ Roaring64NavigableMap bitmap = blockIds[bitmapIndex];
getBlockIdsByPartitionId(requestPartitions, bitmap, res, blockIdLayout);
+ RpcAuditContext.getRpcAuditContext()
+ .ifPresent(
Review Comment:
ifPresent is a good practise, because this avoids unnessary cpu cost if
having the much string operations.
##########
common/src/main/java/org/apache/uniffle/common/audit/RpcAuditContext.java:
##########
@@ -119,6 +128,21 @@ public RpcAuditContext withFrom(String from) {
return this;
}
+ /**
+ * Sets context field, context can be concat by invoke multiply time.
+ *
+ * @param contextPart the new context part
+ * @return this {@link RpcAuditContext} instance
+ */
+ public RpcAuditContext withContext(String contextPart) {
+ if (context == null) {
+ context = contextPart;
+ } else {
+ this.context += ", " + contextPart;
Review Comment:
Like this?
```
class Context {
string contextName;
List<Context> children;
}
```
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -906,7 +908,7 @@ public void getShuffleResultForMultiPart(
auditContext.withAppId(appId).withShuffleId(shuffleId);
auditContext.withArgs(
- "partitionsListSize=" + partitionsList.size() + ", blockIdLayout=" +
blockIdLayout);
+ "partitionsList=" + partitionsList + ", blockIdLayout=" +
blockIdLayout);
Review Comment:
Concern that the partitionsList will cost too much time. Right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
