Jackie-Jiang commented on a change in pull request #8358:
URL: https://github.com/apache/pinot/pull/8358#discussion_r834636118
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java
##########
@@ -125,6 +125,14 @@ public void setTotalDocs(long totalDocs) {
setNonNegativeValue(Segment.TOTAL_DOCS, totalDocs);
}
+ public void setSizeInBytes(long sizeInBytes) {
+ setNonNegativeValue(Segment.SIZE_IN_BYTES, sizeInBytes);
+ }
+
+ public long getSizeInBytes() {
+ return _znRecord.getLongField(Segment.SIZE_IN_BYTES, 0);
Review comment:
Default to -1 if not available
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerGauge.java
##########
@@ -92,7 +94,10 @@
DROPPED_MINION_INSTANCES("droppedMinionInstances", true),
// Number of online minion instances
- ONLINE_MINION_INSTANCES("onlineMinionInstances", true);
+ ONLINE_MINION_INSTANCES("onlineMinionInstances", true),
+
+ // Total size of compressed segments per table
+ TABLE_COMPRESSED_SIZE("bytes", false);
Review comment:
Same here
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3363,6 +3367,26 @@ public TableStats getTableStats(String
tableNameWithType) {
return hosts;
}
+ /**
+ * Returns the number of replicas for a given table config
+ */
+ public int getNumReplicas(TableConfig tableConfig) {
+ if (tableConfig.isDimTable()) {
+ // If the table is a dimension table then fetch the tenant config and
get the number of server belonging
+ // to the tenant
+ TenantConfig tenantConfig = tableConfig.getTenantConfig();
+ Set<String> serverInstances =
getAllInstancesForServerTenant(tenantConfig.getServer());
+ return serverInstances.size();
+ }
+
+ // TODO check for LLC table
+ // if (tableConfig.getTableType() == TableType.REALTIME && llc) {
Review comment:
Use `if (ReplicationUtils.useReplicasPerPartition(tableConfig))`
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerGauge.java
##########
@@ -72,6 +72,8 @@
// Size of the last uploaded offline segment
LAST_PUSHED_SEGMENT_SIZE("LastPushedSegmentSize", false),
+ // Estimated size of a table per replica. Counts both offline and realtime.
+ TABLE_ESTIMATED_SIZE_PER_REPLICA("bytes", false),
Review comment:
Consider following the existing convention for the unit to ensure the
expected behavior. Not sure how the metric is handled on the client side. We
may clean them up in a separate PR if needed.
```suggestion
TABLE_ESTIMATED_SIZE_PER_REPLICA("TableEstimatedSizePerReplica", false),
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java
##########
@@ -57,21 +62,30 @@
private static final long DISABLED_TABLE_LOG_INTERVAL_MS =
TimeUnit.DAYS.toMillis(1);
private static final ZNRecordSerializer RECORD_SERIALIZER = new
ZNRecordSerializer();
+ private static final int TABLE_CHECKER_TIMEOUT_MS = 30_000;
+
private final int _waitForPushTimeSeconds;
private long _lastDisabledTableLogTimestamp = 0;
+ private TableSizeReader _tableSizeReader;
+ private ExecutorService _executorService;
Review comment:
(minor) make it final, or remove it as it is not needed as member
variable
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/util/TableSizeReader.java
##########
@@ -78,36 +79,41 @@ public TableSizeDetails getTableSizeDetails(@Nonnull String
tableName, @Nonnegat
Preconditions.checkNotNull(tableName, "Table name should not be null");
Preconditions.checkArgument(timeoutMsec > 0, "Timeout value must be
greater than 0");
- boolean hasRealtimeTable = false;
- boolean hasOfflineTable = false;
- TableType tableType =
TableNameBuilder.getTableTypeFromTableName(tableName);
+ TableConfig offlineTableConfig =
+
ZKMetadataProvider.getOfflineTableConfig(_helixResourceManager.getPropertyStore(),
tableName);
+ TableConfig realtimeTableConfig =
+
ZKMetadataProvider.getRealtimeTableConfig(_helixResourceManager.getPropertyStore(),
tableName);
- if (tableType != null) {
- hasRealtimeTable = tableType == TableType.REALTIME;
- hasOfflineTable = tableType == TableType.OFFLINE;
- } else {
- hasRealtimeTable = _helixResourceManager.hasRealtimeTable(tableName);
- hasOfflineTable = _helixResourceManager.hasOfflineTable(tableName);
- }
-
- if (!hasOfflineTable && !hasRealtimeTable) {
+ if (offlineTableConfig == null && realtimeTableConfig == null) {
return null;
}
TableSizeDetails tableSizeDetails = new TableSizeDetails(tableName);
-
- if (hasRealtimeTable) {
+ long sizeInBytesPerReplica = 0;
+ if (realtimeTableConfig != null) {
String realtimeTableName =
TableNameBuilder.REALTIME.tableNameWithType(tableName);
tableSizeDetails._realtimeSegments =
getTableSubtypeSize(realtimeTableName, timeoutMsec);
tableSizeDetails._reportedSizeInBytes +=
tableSizeDetails._realtimeSegments._reportedSizeInBytes;
tableSizeDetails._estimatedSizeInBytes +=
tableSizeDetails._realtimeSegments._estimatedSizeInBytes;
+
+ // Get size in bytes per replica
+ sizeInBytesPerReplica +=
+ tableSizeDetails._estimatedSizeInBytes /
_helixResourceManager.getNumReplicas(realtimeTableConfig);
}
- if (hasOfflineTable) {
+ if (offlineTableConfig != null) {
String offlineTableName =
TableNameBuilder.OFFLINE.tableNameWithType(tableName);
tableSizeDetails._offlineSegments =
getTableSubtypeSize(offlineTableName, timeoutMsec);
tableSizeDetails._reportedSizeInBytes +=
tableSizeDetails._offlineSegments._reportedSizeInBytes;
tableSizeDetails._estimatedSizeInBytes +=
tableSizeDetails._offlineSegments._estimatedSizeInBytes;
+
+ // Get size in bytes per replica
+ sizeInBytesPerReplica +=
+ tableSizeDetails._estimatedSizeInBytes /
_helixResourceManager.getNumReplicas(offlineTableConfig);
}
+
+
_controllerMetrics.setValueOfTableGauge(TableNameBuilder.extractRawTableName(tableName),
Review comment:
Let's keep offline and realtime table separately for easier tracking
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerGauge.java
##########
@@ -72,6 +72,8 @@
// Size of the last uploaded offline segment
LAST_PUSHED_SEGMENT_SIZE("LastPushedSegmentSize", false),
+ // Estimated size of a table per replica. Counts both offline and realtime.
+ TABLE_ESTIMATED_SIZE_PER_REPLICA("bytes", false),
Review comment:
I'm thinking keeping 2 metrics (estimated is a little bit misleading
IMO):
`TABLE_TOTAL_SIZE_ON_SERVER` and `TABLE_SIZE_PER_REPLICA_ON_SERVER`
We may deprecate the `OFFLINE_TABLE_ESTIMATED_SIZE` and use
`TABLE_TOTAL_SIZE_ON_SERVER` instead because it covers both offline and
realtime tables
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java
##########
@@ -198,6 +219,9 @@ private void updateSegmentMetrics(String tableNameWithType,
Context context) {
// Push is not finished yet, skip the segment
continue;
}
+ if (segmentZKMetadata != null) {
+ tableCompressedSize += segmentZKMetadata.getSizeInBytes();
Review comment:
Add only if `getSizeInBytes() > 0`
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1910,7 +1910,7 @@ public void addNewSegment(String tableNameWithType,
SegmentMetadata segmentMetad
// NOTE: must first set the segment ZK metadata before assigning segment
to instances because segment assignment
// might need them to determine the partition of the segment, and server
will need them to download the segment
SegmentZKMetadata segmentZkmetadata =
- constructZkMetadataForNewSegment(tableNameWithType, segmentMetadata,
downloadUrl, crypter);
+ constructZkMetadataForNewSegment(tableNameWithType, segmentMetadata,
downloadUrl, crypter, 0);
Review comment:
Put `-1` if the size is unknown
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
