sk0x50 commented on code in PR #6692:
URL: https://github.com/apache/ignite-3/pull/6692#discussion_r2409978355
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryStorageEngine.java:
##########
@@ -353,6 +355,15 @@ public void destroyMvTable(int tableId) {
}
}
+ @Override
+ public void addTableMetrics(StorageTableDescriptor tableDescriptor,
Consumer<Metric> metricConsumer) {
+ PersistentPageMemoryDataRegion region =
regions.get(tableDescriptor.getStorageProfile());
+
+ assert region != null : "Adding metrics to the table with non-existent
data region: " + tableDescriptor;
Review Comment:
IMHO, it would be nice to use only one approach to prepare a message that is
used for logs, asserts, etc.
https://cwiki.apache.org/confluence/display/IGNITE/Java+Logging+Guidelines#JavaLoggingGuidelines-MessageFormat
```
Adding metrics to the table with non-existent data region [tableDescriptor="
+ tablecDescriptor + ']';
```
Anyway, it's up to you.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -3611,9 +3611,14 @@ private void addTableToZone(int zoneId, TableImpl table)
throws IgniteInternalEx
}
}
- private TableMetricSource createAndRegisterMetricsSource(QualifiedName
tableName) {
+ private TableMetricSource
createAndRegisterMetricsSource(StorageTableDescriptor tableDescriptor,
QualifiedName tableName) {
TableMetricSource source = new TableMetricSource(tableName);
+ StorageEngine engine =
dataStorageMgr.engineByStorageProfile(tableDescriptor.getStorageProfile());
+ if (engine != null) {
Review Comment:
Is it really possible that the required engine can be `null`?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/StorageEngine.java:
##########
@@ -73,6 +75,16 @@ public interface StorageEngine {
*/
void destroyMvTable(int tableId);
+ /**
+ * Adds metrics related to the table to the given metric consumer.
+ *
+ * @param tableDescriptor Table descriptor.
+ * @param metricConsumer Metric consumer.
+ */
+ default void addTableMetrics(StorageTableDescriptor tableDescriptor,
Consumer<Metric> metricConsumer) {
Review Comment:
I think I understand the reasoning behind this decision (I mean, using
`Consumer`), but still, it seems a bit odd.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryDataRegion.java:
##########
@@ -437,13 +440,13 @@ private void checkDataRegionStarted() {
/** Adds a table storage to the data region. */
@VisibleForTesting
public void addTableStorage(PersistentPageMemoryTableStorage tableStorage)
{
- boolean add = tableStorages.add(tableStorage);
+ PersistentPageMemoryTableStorage old =
tableStorages.put(tableStorage.getTableId(), tableStorage);
- assert add : tableStorage.getTableId();
+ assert old == null : tableStorage.getTableId();
Review Comment:
Even if this method is only used in tests, it would still be nice to add a
meaningful message.
--
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]
