Jackie-Jiang commented on code in PR #14199:
URL: https://github.com/apache/pinot/pull/14199#discussion_r1805546794
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/BrokerRoutingManager.java:
##########
@@ -729,6 +741,8 @@ private static class RoutingEntry {
// Time boundary manager is only available for the offline part of the
hybrid table
transient TimeBoundaryManager _timeBoundaryManager;
+ transient boolean _enabled;
Review Comment:
(minor) Suggest reversing it to `_disabled`. Usually it is more error prune
if a boolean field is `false` by default
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -660,7 +661,22 @@ protected BrokerResponse handleRequest(long requestId,
String query, SqlNodeAndO
List<ProcessingException> exceptions = new ArrayList<>();
if (numUnavailableSegments > 0) {
String errorMessage;
- if (numUnavailableSegments >
MAX_UNAVAILABLE_SEGMENTS_TO_PRINT_IN_QUERY_EXCEPTION) {
+ if (((realtimeTableConfig != null && offlineTableConfig != null)
Review Comment:
It is possible that one table is disabled and the other table has some
unavailable segments. To handle all scenarios, we can add 2 boolean fields
`offlineTableDisabled` and `realtimeTableDisabled` in addition to
`unavailableSegments`. We can skip adding unavailable segments when a table is
disabled
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/BrokerRoutingManager.java:
##########
@@ -603,6 +603,18 @@ public boolean routingExists(String tableNameWithType) {
return _routingEntryMap.containsKey(tableNameWithType);
}
+ /**
+ * Returns whether the given table is enabled
+ * @param tableNameWithType Table name with type
+ * @return Whether the given table is enabled
+ */
+ public boolean isTableEnabled(String tableNameWithType) {
+ if (_routingEntryMap.containsKey(tableNameWithType)) {
Review Comment:
We can save one map lookup by directly calling `get()` and check whether the
return value is `null`
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/BrokerRoutingManager.java:
##########
@@ -747,6 +761,7 @@ private static class RoutingEntry {
_partitionMetadataManager = partitionMetadataManager;
_queryTimeoutMs = queryTimeoutMs;
_segmentZkMetadataFetcher = segmentZkMetadataFetcher;
+ _enabled = true;
Review Comment:
We should initialize this value properly. It is possible that a table is
already disabled when broker creating its routing
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/BrokerRoutingManager.java:
##########
@@ -603,6 +603,18 @@ public boolean routingExists(String tableNameWithType) {
return _routingEntryMap.containsKey(tableNameWithType);
}
+ /**
+ * Returns whether the given table is enabled
+ * @param tableNameWithType Table name with type
+ * @return Whether the given table is enabled
+ */
+ public boolean isTableEnabled(String tableNameWithType) {
Review Comment:
(minor) Similar here, suggest reversing it to `isTableDisabled()`
--
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
