cbrentharris commented on a change in pull request #5745:
URL: https://github.com/apache/incubator-pinot/pull/5745#discussion_r460111885
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QueryConfig.java
##########
@@ -37,15 +37,23 @@
// If the server times out, it will directly interrupt the query execution.
The server response does not matter much
// because by the time the server times out, the broker should already timed
out and returned the response.
private final Long _timeoutMs;
+ private final Boolean _serveOfflineSegmentsImmediately;
Review comment:
Can you beef up the documentation to this config in the java class.
Also, are the pinot docs in repo as well? Should be updated to reflect the new
type of config you can set.
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -122,19 +122,16 @@
public BaseBrokerRequestHandler(PinotConfiguration config, RoutingManager
routingManager,
AccessControlFactory accessControlFactory, QueryQuotaManager
queryQuotaManager, BrokerMetrics brokerMetrics,
- ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ TableCache tableCache) {
_config = config;
_routingManager = routingManager;
_accessControlFactory = accessControlFactory;
_queryQuotaManager = queryQuotaManager;
_brokerMetrics = brokerMetrics;
+ _tableCache = tableCache;
_enableCaseInsensitive =
_config.getProperty(CommonConstants.Helix.ENABLE_CASE_INSENSITIVE_KEY, false);
- if (_enableCaseInsensitive) {
- _tableCache = new TableCache(propertyStore);
Review comment:
Why was previously the cache only instantiated if case insensitivity was
enabled, but now it is regardless? What impact will that have?
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1087,6 +1084,22 @@ private void attachTimeBoundary(String rawTableName,
BrokerRequest brokerRequest
}
}
+ /**
+ * The time boundary filter string splits queries to offline and realtime
pinot tables based on the current high
+ * watermark of available segments in the offline table. By default, Pinot
serves all segments from offline up to
+ * the latest segment (exclusive), and everything else from realtime.
However, one can configure Pinot to serve
+ * the latest offline segment inclusively via config.
+ */
+ private String getTimeBoundaryFilter(String offlineTableName, String
timeValue, boolean isOfflineRequest) {
+ boolean shouldServeOfflineSegmentsImmediately =
Optional.ofNullable(_tableCache.getTableConfig(offlineTableName).getQueryConfig())
+ .map(QueryConfig::getServeOfflineSegmentsImmediately)
+ .orElse(false);
+ String open = isOfflineRequest || !shouldServeOfflineSegmentsImmediately ?
"(" : "[";
Review comment:
Perhaps a little more self documenting:
```
boolean openRangeInclusive = !isOfflineRequest || shouldServeOfflineSegments;
String open = openRangeInclusive ? INCLUSIVE_OPEN_RANGE :
EXCLUSIVE_OPEN_RANGE;
String range = isOfflineRequest ? ALL_VALUES_BEFORE + timeValue : timeValue
+ ALL_VALUES_AFTER;
boolean closeRangeInclusive = isOfflineRequest &&
!shouldServeOfflineSegmentsImmediately;
```
Etc
As rightt now this is a little confusing because I assume that `]` is
inclusive and `)` is exclusive, but `)` seems to be inclusive when its offline
and not configured to serve immediately, which seems counter intuitive.
----------------------------------------------------------------
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.
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
