Jackie-Jiang commented on code in PR #19221:
URL: https://github.com/apache/pinot/pull/19221#discussion_r3770625966
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerGauge.java:
##########
@@ -107,7 +107,27 @@ public enum BrokerGauge implements AbstractMetrics.Gauge {
/// signals a leak in the ZK listener / drop path.
MATERIALIZED_VIEW_CACHE_ENTRY_COUNT("materializedViewCacheEntries", true),
// Workload config fetch status: 1 = success, 0 = failure
- WORKLOAD_CONFIG_FETCH_STATUS("status", true);
+ WORKLOAD_CONFIG_FETCH_STATUS("status", true),
+
+ /// Replica availability of a table as observed by this broker's routing:
the smallest percentage of
+ /// assigned replicas that are actually routable, across all the table's
segments. `100` means every segment can be
+ /// served from every replica the ideal state assigns to it; `0` means at
least one segment cannot be served at all.
+ ///
+ /// Segments still classified new (see
+ ///
[org.apache.pinot.spi.utils.CommonConstants.Broker#CONFIG_OF_NEW_SEGMENT_EXPIRATION_SECONDS])
are
+ /// excluded because they are commonly not yet loaded everywhere
+ PERCENT_OF_REPLICAS("percent", false),
+
+ /// Number of the table's segments that this broker currently cannot route
to any server. Segments assigned a
+ /// single replica are included
+ UNAVAILABLE_SEGMENTS("segments", false),
+
+ /// Number of the table's segments that are down to their last routable
replica, or have none left.
+ /// Segments assigned a single replica are excluded, since they never had
redundancy to lose. Recently created
+ /// segments are excluded on the same terms as [#PERCENT_OF_REPLICAS].
+ SEGMENTS_WITHOUT_REDUNDANCY("segments", false);
+
+
Review Comment:
(nit) Remove extra empty lines
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerGauge.java:
##########
@@ -107,7 +107,27 @@ public enum BrokerGauge implements AbstractMetrics.Gauge {
/// signals a leak in the ZK listener / drop path.
MATERIALIZED_VIEW_CACHE_ENTRY_COUNT("materializedViewCacheEntries", true),
// Workload config fetch status: 1 = success, 0 = failure
- WORKLOAD_CONFIG_FETCH_STATUS("status", true);
+ WORKLOAD_CONFIG_FETCH_STATUS("status", true),
+
+ /// Replica availability of a table as observed by this broker's routing:
the smallest percentage of
+ /// assigned replicas that are actually routable, across all the table's
segments. `100` means every segment can be
+ /// served from every replica the ideal state assigns to it; `0` means at
least one segment cannot be served at all.
+ ///
+ /// Segments still classified new (see
+ ///
[org.apache.pinot.spi.utils.CommonConstants.Broker#CONFIG_OF_NEW_SEGMENT_EXPIRATION_SECONDS])
are
+ /// excluded because they are commonly not yet loaded everywhere
+ PERCENT_OF_REPLICAS("percent", false),
+
+ /// Number of the table's segments that this broker currently cannot route
to any server. Segments assigned a
+ /// single replica are included
+ UNAVAILABLE_SEGMENTS("segments", false),
+
+ /// Number of the table's segments that are down to their last routable
replica, or have none left.
+ /// Segments assigned a single replica are excluded, since they never had
redundancy to lose. Recently created
+ /// segments are excluded on the same terms as [#PERCENT_OF_REPLICAS].
+ SEGMENTS_WITHOUT_REDUNDANCY("segments", false);
Review Comment:
What is the purpose of this gauge? I feel `PERCENT_OF_REPLICAS` should be
good enough
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -96,17 +105,29 @@ public abstract class BaseInstanceSelector implements
InstanceSelector {
protected InstanceSelectorConfig _config;
protected long _newSegmentExpirationTimeInSeconds;
protected boolean _emitSinglePoolSegmentsMetric;
+ protected boolean _emitReplicaHealthMetrics;
protected int _tableNameHashForFixedReplicaRouting;
+ /// Whether the table is disabled, from the ideal state. A disabled table
has all of its replicas taken
+ /// offline deliberately, so its replica health is not reported at all.
+ protected boolean _tableDisabled;
// These 3 variables are the cached states to help accelerate the change
processing
protected Set<String> _enabledInstances;
// For old segments, all candidates are online
// Reduce this map to reduce garbage
protected final Map<String, List<SegmentInstanceCandidate>>
_oldSegmentCandidatesMap = new HashMap<>();
protected Map<String, NewSegmentState> _newSegmentStateMap;
+ /// Number of ONLINE/CONSUMING instances in the ideal state, for old
segments that have fewer
+ /// candidates than that. Only used for metrics
+ ///
+ /// Kept sparse deliberately: an absent entry means "as many candidates as
the ideal state assigns",
+ /// so a healthy table stores nothing here. Read it through
[#getExpectedReplicas].
+ protected final Map<String, Integer> _oldSegmentExpectedReplicasMap = new
HashMap<>();
Review Comment:
Do we need to track this map? Can we update the min percentage as we add
segment to the routing?
--
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]
