Baunsgaard commented on code in PR #1641:
URL: https://github.com/apache/systemds/pull/1641#discussion_r905098797
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/models/NodeEntityModel.java:
##########
@@ -68,13 +83,43 @@ public void setStats(final List<BaseEntityModel> stats) {
_stats = stats;
}
+ public double getJitCompileTime() {
+ return _jitCompileTime;
+ }
+
+ public void setJitCompileTime(final double jitCompileTime) {
+ _jitCompileTime = jitCompileTime;
+ }
+
+ public String getRequestTypeCount() {
+ List<String> requestTypeCountStrArr = new ArrayList<>();
+
+ if (_requestTypeCount != null) {
+ for(var entry : _requestTypeCount) {
+ requestTypeCountStrArr.add(String.format("{" +
Review Comment:
this is not nice, make the formatting string outside the for loop (i do not
trust the java compiler to fix this.)
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/models/NodeEntityModel.java:
##########
@@ -68,13 +83,43 @@ public void setStats(final List<BaseEntityModel> stats) {
_stats = stats;
}
+ public double getJitCompileTime() {
+ return _jitCompileTime;
+ }
+
+ public void setJitCompileTime(final double jitCompileTime) {
+ _jitCompileTime = jitCompileTime;
+ }
+
+ public String getRequestTypeCount() {
+ List<String> requestTypeCountStrArr = new ArrayList<>();
+
+ if (_requestTypeCount != null) {
+ for(var entry : _requestTypeCount) {
+ requestTypeCountStrArr.add(String.format("{" +
+ "\"type\": \"%s\"," +
+ "\"count\": %d" +
+ "}",
entry.getLeft().toString(), entry.getRight()));
+ }
+ }
+
+ return String.join(",", requestTypeCountStrArr);
+ }
+
+ public void setRequestTypeCount(final
List<Pair<FederatedRequest.RequestType, Long>> requestTypeCount) {
+ _requestTypeCount = requestTypeCount;
+ }
+
@Override
public String toString() {
return String.format("{" +
"\"id\": %d," +
"\"name\": \"%s\"," +
"\"address\": \"%s\"," +
+ "\"isOnline\": %b," +
+ "\"jitCompileTime\": %.2f," +
Review Comment:
same here, make a static string or something different.
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/models/StatsEntityModel.java:
##########
@@ -103,21 +134,22 @@ public void setHeavyHitterInstructions(final String
heavyHitterInstructionsJsonS
}
public String getTransferredBytes() {
- if (_transferredBytes.isEmpty() || _transferredBytes.isBlank())
{
- StringBuilder sb = new StringBuilder();
+ if ((_transferredBytes.isEmpty() ||
_transferredBytes.isBlank()) && _transferredBytesObj != null) {
+
+ List<String> transferredBytesStrArr = new ArrayList<>();
- sb.append("{");
for (var entry: _transferredBytesObj) {
- sb.append(String.format("{" +
- "\"datetime\": %s," +
+ transferredBytesStrArr.add(String.format("{" +
+ "\"datetime\": \"%s\"," +
Review Comment:
format string outside for loop
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/models/StatsEntityModel.java:
##########
@@ -74,25 +97,33 @@ public double getMemoryUsage() {
public void setMemoryUsage(final double memoryUsage) {
_memoryUsage = memoryUsage;
}
+ public double getJitCompileTime() {
+ return _jitCompileTime;
+ }
+
+ public List<Pair<FederatedRequest.RequestType, Long>>
getRequestTypeCount() {
+ return _requestTypeCount;
+ }
public String getHeavyHitterInstructions() {
- if (_heavyHitterInstructions.isEmpty() ||
_heavyHitterInstructions.isBlank()) {
- StringBuilder sb = new StringBuilder();
+ if ((_heavyHitterInstructions.isEmpty() ||
_heavyHitterInstructions.isBlank()) && _heavyHitterInstructionsObj != null) {
+
+ List<String> heavyHittersStrArr = new ArrayList<>();
- sb.append("{");
for(Map.Entry<String, Pair<Long, Double>> entry :
_heavyHitterInstructionsObj.entrySet()) {
String instruction = entry.getKey();
Long count = entry.getValue().getLeft();
double duration = entry.getValue().getRight();
- sb.append(String.format("{" +
- "\"instruction\": %s," +
- "\"count\": \"%d\"," +
- "\"duration\": \"%.2f\"," +
- "},", instruction, count, duration));
+ heavyHittersStrArr.add(String.format("{" +
+ "\"instruction\": \"%s\"," +
Review Comment:
make format string outside loop
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/repositories/DerbyRepository.java:
##########
@@ -43,17 +43,21 @@ public class DerbyRepository implements IRepository {
private static final String ENTITY_SCHEMA_CREATE_STATS_STMT = "CREATE
TABLE %s " +
"(id INTEGER PRIMARY KEY GENERATED ALWAYS AS IDENTITY
(START WITH 1, INCREMENT BY 1), " +
"%s INTEGER, " +
Review Comment:
this is a good example of pre-computing your format string
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/services/StatsService.java:
##########
@@ -45,13 +50,20 @@ public static BaseEntityModel getWorkerStatistics(Long id,
String address) {
aggFedStats.aggregate((FederatedStatistics.FedStatsCollection)tmp[0]);
parsedStats = new StatsEntityModel(
- id, aggFedStats.cpuUsage,
aggFedStats.memoryUsage,
- aggFedStats.heavyHitters,
aggFedStats.coordinatorsTrafficBytes);
+ id,
+ new
Timestamp(System.currentTimeMillis()),
+ aggFedStats.cpuUsage,
+ aggFedStats.memoryUsage,
+ aggFedStats.jitCompileTime,
+ aggFedStats.heavyHitters,
+
aggFedStats.coordinatorsTrafficBytes,
+ aggFedStats.requestTypeCount);
}
- } catch(DMLRuntimeException dre) {
+ } catch (DMLRuntimeException dre) {
// silently ignore -> caused by offline federated
workers
+ log.error("Worker offline: " + dre.getMessage());
} catch (Exception e) {
- throw new RuntimeException(e);
+ log.error("Error: " + e.getMessage());
Review Comment:
this is super dangerous.
Please verify that this is the actual behavior you want,
can you limit the exception caught to something else?
And if it really is the functionality you want add a comment saying that we
do not handle the exception but just log it, and that it is fine.
--
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]
