yifan-c commented on code in PR #117:
URL: https://github.com/apache/cassandra-sidecar/pull/117#discussion_r1579876627
##########
src/main/java/org/apache/cassandra/sidecar/utils/SSTableImporter.java:
##########
@@ -623,4 +645,38 @@ public ImportOptions build()
}
}
}
+
+ /**
+ * Key used for the map of queues
+ */
+ protected static class ImportKey
Review Comment:
1. Maybe call it `ImportId`?
2. Can change the access modifier to `private`? I think it is unexpected to
create its subclasses. `protected` adds maintenance burden
##########
src/main/java/org/apache/cassandra/sidecar/snapshots/SnapshotPathBuilder.java:
##########
@@ -298,14 +295,13 @@ public boolean equals(Object object)
return size == that.size
&& dataDirectoryIndex == that.dataDirectoryIndex
&& Objects.equals(name, that.name)
- && Objects.equals(path, that.path)
&& Objects.equals(tableId, that.tableId);
}
@Override
public int hashCode()
{
- return Objects.hash(name, path, size, dataDirectoryIndex, tableId);
+ return Objects.hash(name, size, dataDirectoryIndex, tableId);
Review Comment:
similar nit: the `hashcode` can be pre-computed.
##########
src/main/java/org/apache/cassandra/sidecar/utils/SSTableImporter.java:
##########
@@ -623,4 +645,38 @@ public ImportOptions build()
}
}
}
+
+ /**
+ * Key used for the map of queues
+ */
+ protected static class ImportKey
+ {
+ private final String host;
+ private final String keyspace;
+ private final String table;
+
+ public ImportKey(String host, String keyspace, String table)
+ {
+ this.host = host;
+ this.keyspace = keyspace;
+ this.table = table;
+ }
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ ImportKey importKey = (ImportKey) o;
+ return Objects.equals(host, importKey.host)
+ && Objects.equals(keyspace, importKey.keyspace)
+ && Objects.equals(table, importKey.table);
+ }
+
+ @Override
+ public int hashCode()
+ {
+ return Objects.hash(host, keyspace, table);
Review Comment:
NIT: The object is only going to be used as map keys. We can store the
hashcode as an instance variable. (so compute only once)
##########
src/main/java/org/apache/cassandra/sidecar/utils/SSTableImporter.java:
##########
@@ -310,6 +307,31 @@ private void cleanup(ImportOptions options)
cause));
}
+ /**
+ * Aggregates pending imports per host from multiple keyspaces and tables
+ */
+ private void reportPendingImportAggregates()
+ {
+ Map<String, Integer> aggregates = new HashMap<>();
+ for (Map.Entry<ImportKey, ImportQueue> entry :
importQueuePerHost.entrySet())
+ {
+ aggregates.compute(entry.getKey().host, (k, v) ->
entry.getValue().size() + (v == null ? 0 : v));
+ }
+
+ aggregates.forEach((host, count) -> {
+ try
+ {
+ // Report aggregate metrics for the queues
+ InstanceMetadata instance = metadataFetcher.instance(host);
+
instance.metrics().sstableImport().pendingImports.metric.setValue(count);
+ }
+ catch (Exception e)
+ {
+ LOGGER.warn("Unable to report metrics for host={}
pendingImports={}", host, count, e);
+ }
+ });
Review Comment:
also a nit
```suggestion
importQueuePerHost
.entrySet()
.stream()
.collect(Collectors.groupingBy(e -> e.getKey().host,
Collectors.counting()))
.forEach((host, count) -> {
try
{
// Report aggregate metrics for the queues
InstanceMetadata instance = metadataFetcher.instance(host);
instance.metrics().sstableImport().pendingImports.metric.setValue(count.intValue());
}
catch (Exception e)
{
LOGGER.warn("Unable to report metrics for host={}
pendingImports={}", host, count, e);
}
});
```
##########
src/main/java/org/apache/cassandra/sidecar/utils/SSTableImporter.java:
##########
@@ -310,6 +307,31 @@ private void cleanup(ImportOptions options)
cause));
}
+ /**
+ * Aggregates pending imports per host from multiple keyspaces and tables
+ */
+ private void reportPendingImportAggregates()
Review Comment:
nit: the method name is a bit confusing with "Aggregates". How about
`reportPendingImportPerHost`?
--
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]
