You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@uniffle.apache.org by ro...@apache.org on 2023/06/29 06:01:29 UTC
[incubator-uniffle] branch master updated: [#961] refactor(coordinator): Improve Coordinator Log Format. (#962)
This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git
The following commit(s) were added to refs/heads/master by this push:
new add5da2c [#961] refactor(coordinator): Improve Coordinator Log Format. (#962)
add5da2c is described below
commit add5da2c914bf46ad98ec0c4ac391388a6e1ed19
Author: slfan1989 <55...@users.noreply.github.com>
AuthorDate: Thu Jun 29 14:01:24 2023 +0800
[#961] refactor(coordinator): Improve Coordinator Log Format. (#962)
### What changes were proposed in this pull request?
While reading the code and completing the PR, I came across some log messages in the coordinator module that use string concatenation. We are already using slf4j, and it would be better to use placeholder syntax.
### Why are the changes needed?
log format.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
No need.
Co-authored-by: slfan1989 <louj1988@@>
---
.../uniffle/coordinator/ApplicationManager.java | 4 ++--
.../uniffle/coordinator/ClientConfManager.java | 4 ++--
.../coordinator/CoordinatorGrpcService.java | 22 ++++++++++------------
.../apache/uniffle/coordinator/QuotaManager.java | 2 +-
.../uniffle/coordinator/SimpleClusterManager.java | 8 ++++----
.../access/checker/AccessClusterLoadChecker.java | 2 +-
.../assignment/BasicAssignmentStrategy.java | 2 +-
.../PartitionBalanceAssignmentStrategy.java | 2 +-
.../coordinator/SimpleClusterManagerTest.java | 2 +-
9 files changed, 23 insertions(+), 25 deletions(-)
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/ApplicationManager.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/ApplicationManager.java
index 84885914..955b0d7b 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/ApplicationManager.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/ApplicationManager.java
@@ -284,9 +284,9 @@ public class ApplicationManager implements Closeable {
}
}
}
- LOG.info("Start to check status for " + appIds.size() + " applications");
+ LOG.info("Start to check status for {} applications.", appIds.size());
for (String appId : expiredAppIds) {
- LOG.info("Remove expired application:" + appId);
+ LOG.info("Remove expired application : {}.", appId);
appIds.remove(appId);
if (appIdToRemoteStorageInfo.containsKey(appId)) {
decRemoteStorageCounter(appIdToRemoteStorageInfo.get(appId).getPath());
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/ClientConfManager.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/ClientConfManager.java
index 1756b1fe..5a689384 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/ClientConfManager.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/ClientConfManager.java
@@ -71,7 +71,7 @@ public class ClientConfManager implements Closeable {
throw new IllegalStateException(msg);
}
updateClientConfInternal();
- LOG.info("Load client conf from " + pathStr + " successfully");
+ LOG.info("Load client conf from {} successfully", pathStr);
int updateIntervalS = conf.getInteger(CoordinatorConf.COORDINATOR_DYNAMIC_CLIENT_CONF_UPDATE_INTERVAL_SEC);
updateClientConfSES = ThreadUtils.getDaemonSingleThreadScheduledExecutor("ClientConfManager");
@@ -93,7 +93,7 @@ public class ClientConfManager implements Closeable {
LOG.warn("Client conf file not found with {}", path);
}
} catch (Exception e) {
- LOG.warn("Error when update client conf with " + path, e);
+ LOG.warn("Error when update client conf with {}.", path, e);
}
}
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java
index 1ab782d7..5dfec20a 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java
@@ -120,12 +120,10 @@ public class CoordinatorGrpcService extends CoordinatorServerGrpc.CoordinatorSer
final int requiredShuffleServerNumber = request.getAssignmentShuffleServerNumber();
final int estimateTaskConcurrency = request.getEstimateTaskConcurrency();
- LOG.info("Request of getShuffleAssignments for appId[" + appId
- + "], shuffleId[" + shuffleId + "], partitionNum[" + partitionNum
- + "], partitionNumPerRange[" + partitionNumPerRange + "], replica[" + replica
- + "], requiredTags[" + requiredTags
- + "], requiredShuffleServerNumber[" + requiredShuffleServerNumber + "]"
- );
+ LOG.info("Request of getShuffleAssignments for appId[{}], shuffleId[{}], partitionNum[{}], "
+ + " partitionNumPerRange[{}], replica[{}], requiredTags[{}], requiredShuffleServerNumber[{}]",
+ appId, shuffleId, partitionNum, partitionNumPerRange, replica,
+ requiredTags, requiredShuffleServerNumber);
GetShuffleAssignmentsResponse response;
try {
@@ -168,7 +166,7 @@ public class CoordinatorGrpcService extends CoordinatorServerGrpc.CoordinatorSer
.setRetMsg("")
.setStatus(StatusCode.SUCCESS)
.build();
- LOG.debug("Got heartbeat from " + serverNode);
+ LOG.debug("Got heartbeat from {}", serverNode);
responseObserver.onNext(response);
responseObserver.onCompleted();
}
@@ -209,7 +207,7 @@ public class CoordinatorGrpcService extends CoordinatorServerGrpc.CoordinatorSer
StreamObserver<AppHeartBeatResponse> responseObserver) {
String appId = request.getAppId();
coordinatorServer.getApplicationManager().refreshAppId(appId);
- LOG.debug("Got heartbeat from application: " + appId);
+ LOG.debug("Got heartbeat from application: {}", appId);
AppHeartBeatResponse response = AppHeartBeatResponse
.newBuilder()
.setRetMsg("")
@@ -233,7 +231,7 @@ public class CoordinatorGrpcService extends CoordinatorServerGrpc.CoordinatorSer
String appId = request.getAppId();
String user = request.getUser();
coordinatorServer.getApplicationManager().registerApplicationInfo(appId, user);
- LOG.debug("Got a registered application info: " + appId);
+ LOG.debug("Got a registered application info: {}", appId);
ApplicationInfoResponse response = ApplicationInfoResponse
.newBuilder()
.setRetMsg("")
@@ -334,7 +332,7 @@ public class CoordinatorGrpcService extends CoordinatorServerGrpc.CoordinatorSer
} catch (Exception e) {
status = StatusCode.INTERNAL_ERROR;
response = FetchRemoteStorageResponse.newBuilder().setStatus(status).build();
- LOG.error("Error happened when get remote storage for appId[" + appId + "]", e);
+ LOG.error("Error happened when get remote storage for appId[{}]", appId, e);
}
if (Context.current().isCancelled()) {
@@ -357,8 +355,8 @@ public class CoordinatorGrpcService extends CoordinatorServerGrpc.CoordinatorSer
}
}
if (!nodeIds.isEmpty()) {
- LOG.info("Shuffle Servers of assignment for appId[" + appId + "], shuffleId["
- + shuffleId + "] are " + nodeIds);
+ LOG.info("Shuffle Servers of assignment for appId[{}], shuffleId[{}] are {}",
+ appId, shuffleId, nodeIds);
}
}
}
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java
index 54456827..3bca6738 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java
@@ -66,7 +66,7 @@ public class QuotaManager {
try {
hadoopFileSystem = HadoopFilesystemProvider.getFilesystem(new Path(quotaFilePath), new Configuration());
} catch (Exception e) {
- LOG.error("Cannot init remoteFS on path : " + quotaFilePath, e);
+ LOG.error("Cannot init remoteFS on path : {}", quotaFilePath, e);
}
// Threads that update the number of submitted applications
ScheduledExecutorService scheduledExecutorService =
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java
index 6729e8f3..60daf469 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java
@@ -131,7 +131,7 @@ public class SimpleClusterManager implements ClusterManager {
long timestamp = System.currentTimeMillis();
for (ServerNode sn : servers.values()) {
if (timestamp - sn.getTimestamp() > heartbeatTimeout) {
- LOG.warn("Heartbeat timeout detect, " + sn + " will be removed from node list.");
+ LOG.warn("Heartbeat timeout detect, {} will be removed from node list.", sn);
sn.setStatus(ServerStatus.LOST);
lostNodes.add(sn);
unhealthyNodes.remove(sn);
@@ -191,7 +191,7 @@ public class SimpleClusterManager implements ClusterManager {
} catch (FileNotFoundException fileNotFoundException) {
excludeNodes = Sets.newConcurrentHashSet();
} catch (Exception e) {
- LOG.warn("Error when updating exclude nodes, the exclude nodes file path: " + path, e);
+ LOG.warn("Error when updating exclude nodes, the exclude nodes file path: {}.", path, e);
}
int newlyExcludeNodesNumber = excludeNodes.size();
if (newlyExcludeNodesNumber != originalExcludeNodesNumber) {
@@ -212,7 +212,7 @@ public class SimpleClusterManager implements ClusterManager {
}
// update exclude nodes and last modify time
excludeNodes = nodes;
- LOG.info("Updated exclude nodes and " + excludeNodes.size() + " nodes were marked as exclude nodes");
+ LOG.info("Updated exclude nodes and {} nodes were marked as exclude nodes", excludeNodes.size());
}
@Override
@@ -369,7 +369,7 @@ public class SimpleClusterManager implements ClusterManager {
public void reconfigure(RssConf conf) {
int nodeMax = conf.getInteger(CoordinatorConf.COORDINATOR_SHUFFLE_NODES_MAX);
if (nodeMax != shuffleNodesMax) {
- LOG.warn("Coordinator update new shuffleNodesMax " + nodeMax);
+ LOG.warn("Coordinator update new shuffleNodesMax {}", nodeMax);
shuffleNodesMax = nodeMax;
}
}
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/access/checker/AccessClusterLoadChecker.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/access/checker/AccessClusterLoadChecker.java
index 093709a3..a5925eb9 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/access/checker/AccessClusterLoadChecker.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/access/checker/AccessClusterLoadChecker.java
@@ -123,7 +123,7 @@ public class AccessClusterLoadChecker extends AbstractAccessChecker implements R
public void reconfigure(RssConf conf) {
int nodeMax = conf.get(CoordinatorConf.COORDINATOR_SHUFFLE_NODES_MAX);
if (nodeMax != defaultRequiredShuffleServerNumber) {
- LOG.warn("Coordinator update new defaultRequiredShuffleServerNumber " + nodeMax);
+ LOG.warn("Coordinator update new defaultRequiredShuffleServerNumber {}.", nodeMax);
defaultRequiredShuffleServerNumber = nodeMax;
}
}
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/strategy/assignment/BasicAssignmentStrategy.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/strategy/assignment/BasicAssignmentStrategy.java
index 47557e13..12c3750f 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/strategy/assignment/BasicAssignmentStrategy.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/strategy/assignment/BasicAssignmentStrategy.java
@@ -67,7 +67,7 @@ public class BasicAssignmentStrategy extends AbstractAssignmentStrategy {
Collections.shuffle(servers);
Collections.sort(servers);
if (expectedNum > servers.size()) {
- LOG.warn("Can't get expected servers [" + expectedNum + "] and found only [" + servers.size() + "]");
+ LOG.warn("Can't get expected servers [{}] and found only [{}]", expectedNum, servers.size());
return servers;
}
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/strategy/assignment/PartitionBalanceAssignmentStrategy.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/strategy/assignment/PartitionBalanceAssignmentStrategy.java
index 6a6458f3..e82cc71f 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/strategy/assignment/PartitionBalanceAssignmentStrategy.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/strategy/assignment/PartitionBalanceAssignmentStrategy.java
@@ -122,7 +122,7 @@ public class PartitionBalanceAssignmentStrategy extends AbstractAssignmentStrate
}
if (nodes.size() < expectNum) {
- LOG.warn("Can't get expected servers [" + expectNum + "] and found only [" + nodes.size() + "]");
+ LOG.warn("Can't get expected servers [{}] and found only [{}]", expectNum, nodes.size());
expectNum = nodes.size();
}
diff --git a/coordinator/src/test/java/org/apache/uniffle/coordinator/SimpleClusterManagerTest.java b/coordinator/src/test/java/org/apache/uniffle/coordinator/SimpleClusterManagerTest.java
index 1457847f..c3b48e6c 100644
--- a/coordinator/src/test/java/org/apache/uniffle/coordinator/SimpleClusterManagerTest.java
+++ b/coordinator/src/test/java/org/apache/uniffle/coordinator/SimpleClusterManagerTest.java
@@ -304,7 +304,7 @@ public class SimpleClusterManagerTest {
private void addNode(String id, SimpleClusterManager clusterManager) {
ServerNode node = new ServerNode(id, "ip", 0, 100L, 50L, 30L, 10, testTags);
- LOG.info("Add node " + node.getId() + " " + node.getTimestamp());
+ LOG.info("Add node {} {}", node.getId(), node.getTimestamp());
clusterManager.add(node);
}