vrajat commented on code in PR #15515:
URL: https://github.com/apache/pinot/pull/15515#discussion_r2062879617
##########
pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java:
##########
@@ -144,6 +172,17 @@ public String getActualTableName(String tableName) {
}
}
+ /**
+ * Returns the actual logical table name for the given table name, or {@code
null} if table does not exist.
+ * @param logicalTableName Logical table name
+ * @return Actual logical table name
+ */
Review Comment:
(nit) Add `@Nullable` annotation
##########
pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java:
##########
@@ -144,6 +172,17 @@ public String getActualTableName(String tableName) {
}
}
+ /**
+ * Returns the actual logical table name for the given table name, or {@code
null} if table does not exist.
+ * @param logicalTableName Logical table name
+ * @return Actual logical table name
+ */
+ public String getActualLogicalTableName(String logicalTableName) {
Review Comment:
Is this function only used in tests ? No other use right now ? I checked the
usages of `getActualTableName` and it is used in a bunch of places. Some of
them are out of scope for this PR. Can you re-verify though ?
##########
pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java:
##########
@@ -390,6 +514,22 @@ private List<Schema> getSchemas() {
return schemas;
}
+ public boolean isLogicalTable(String logicalTableName) {
Review Comment:
Can this function use `getActualLogicalTableName` and check for null ?
##########
pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java:
##########
@@ -476,6 +581,49 @@ public synchronized void handleDataDeleted(String path) {
}
}
+ private class ZkLogicalTableChangeListener implements IZkChildListener,
IZkDataListener {
+
+ @Override
+ public synchronized void handleChildChange(String path, List<String>
logicalTableNames) {
+ if (CollectionUtils.isEmpty(logicalTableNames)) {
+ return;
+ }
+
+ // Only process new added logical tables. Changed/removed logical tables
are handled by other callbacks.
+ List<String> pathsToAdd = new ArrayList<>();
+ for (String logicalTableName : logicalTableNames) {
+ if (!_logicalTableNameMap.containsKey(logicalTableName)) {
+ pathsToAdd.add(LOGICAL_TABLE_PATH_PREFIX + logicalTableName);
+ }
+ }
+ if (!pathsToAdd.isEmpty()) {
+ addLogicalTables(pathsToAdd);
+ }
+ notifyLogicalTableChangeListeners();
+ }
+
+ @Override
+ public synchronized void handleDataChange(String path, Object data) {
+ if (data != null) {
+ ZNRecord znRecord = (ZNRecord) data;
+ try {
+ putLogicalTable(znRecord);
+ } catch (Exception e) {
+ LOGGER.error("Caught exception while refreshing logical table for
ZNRecord: {}", znRecord.getId(), e);
+ }
+ notifyLogicalTableChangeListeners();
+ }
+ }
+
+ @Override
+ public synchronized void handleDataDeleted(String path) {
+ // NOTE: The path here is the absolute ZK path instead of the relative
path to the property store.
+ String logicalTableName = path.substring(path.lastIndexOf('/') + 1);
+ removeLogicalTable(LOGICAL_TABLE_PATH_PREFIX + logicalTableName);
+ notifyLogicalTableChangeListeners();
+ }
+ }
+
private static class TableConfigInfo {
Review Comment:
I checked a subsequent PR. The common part is expression override map. The
two options for sharing code for expression override are:
* BaseTableInfo with the expression override code
* Move expression override to a private helper method.
In the draft PR you had chosen 2nd one. Couple of q's
* Option 1 or Option 2?
* If Option 1, then should we consider adding support to set/unset query
options in this PR itself ?
My concern is that the PR is adding a dummy class and we havent had a chance
to discuss the right option to organize the code.
--
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
