github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4080855865
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -463,8 +468,15 @@ && hasLanceIdentityKeyChange(oldProperties, newProperties)
CatalogLog log = new CatalogLog();
log.setCatalogId(catalog.getId());
log.setNewProps(newProperties);
- replayAlterCatalogProps(log, oldProperties, false);
-
Env.getCurrentEnv().getEditLog().logCatalogLog(OperationType.OP_ALTER_CATALOG_PROPS,
log);
+ try {
+ replayAlterCatalogProps(log, oldProperties, false);
+ } finally {
+ // Durably record a committed property change even when the
post-publish reset cleanup
+ // throws. A rolled-back validation leaves the old properties
in place, so it does not log.
+ if (newProperties.equals(catalog.getProperties())) {
Review Comment:
[P1] Detect a committed delta without comparing it to the full map.
`newProperties` is only the SQL `SET PROPERTIES` delta, while
`modifyCatalogProps` merges it with `putAll` and `getProperties()` returns
every catalog property. On a normal catalog with any other existing key this
condition is false even after a successful, non-throwing ALTER, so
`OP_ALTER_CATALOG_PROPS` is never logged and followers/restarts keep the old
target. (The existing credential/unrelated-key test already uses such a partial
map and expects one log; the new cleanup test hides this by stubbing the full
map to equal the delta.) Track whether publication occurred or compare with
`oldProperties + newProperties` instead.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -274,6 +306,10 @@ public void refreshPartitions(String catalogName, String
dbName, String tableNam
if (!(catalog instanceof ExternalCatalog)) {
throw new DdlException("Only support ExternalCatalog");
}
+ // Partition events are already committed remotely. Fence the cache by
cached identity
+ // before any database/table reload can fail and make the
ignored-not-found path return.
+ Env.getCurrentEnv().getExtMetaCacheMgr()
+ .invalidateTableByNameOrWider(catalog.getId(), dbName,
tableName);
Review Comment:
[P2] Keep the warm partition path selective. This helper does more than the
row-count pre-fence: when the table object is resident it calls
`invalidateTableCache`, and Hive drops that table's schema, partition-values,
partition, and file entries before this method invalidates just the named
partitions. ADD/DROP take the same new path (and a partition rename takes it
twice), so the later selective helpers see an absent partition-values entry and
a one-partition event forces a full metadata/file reload for the table. Use a
row-count-only fence before the fallible lookup, and route the wider engine
invalidation only from the actual DB/table-miss branches; cover a warm real
Hive cache with unrelated entries primed.
--
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]
