github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4089270304
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -463,8 +468,18 @@ && 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. modifyCatalogProps merges the delta into the
existing properties, so compare
+ // against the merged map; a rolled-back validation leaves the
old properties in place.
+ Map<String, String> committedProperties = new
java.util.HashMap<>(oldProperties);
+ committedProperties.putAll(newProperties);
+ if (committedProperties.equals(catalog.getProperties())) {
Review Comment:
[P2] Do not infer commit from map equality for same-value deltas. If
validation rejects a delta that merely re-supplies an existing value, the live
properties remain (or are rolled back to) `oldProperties`, so
`committedProperties` still equals them and this finally journals an operation
that returned an error. A follower will then reset connector state and retire
caches for a DDL the leader rejected. Carry an explicit publication/commit
outcome out of the replay helper so committed changes are logged even when
cleanup throws, while pre-publication validation failures are not; cover both
rejected and successful same-value ALTERs.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -226,8 +252,9 @@ public void replayRefreshTable(ExternalObjectLog log) {
// Partition-level cache invalidation, only for hive
catalog
HiveExternalMetaCache cache =
Env.getCurrentEnv().getExtMetaCacheMgr()
.hive(catalog.getId());
- cache.refreshAffectedPartitionsCache(
- (HMSExternalTable) table.get(), modifiedPartNames,
newPartNames);
+
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateRowCountCache(table.get());
+ cache.refreshAffectedPartitionsCache((HMSExternalTable)
table.get(), modifiedPartNames,
Review Comment:
[P1] Fall back to full invalidation when partition replay fails. This
journal record represents an already-committed insert, but
`refreshAffectedPartitionsCache` can throw while resolving new-partition column
types or after only part of the selective mutation. `replayRefreshSafely` then
swallows the exception and the follower advances without retry, while this
row-count fence does not retire the still-resident Hive partition/file entries.
The leader already handles the analogous failure by calling
`invalidateTableCache` and emitting a full-refresh log; give this warm replay
path the same conservative table fallback and extend the throwing replay test
to verify it.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -883,15 +898,19 @@ private void
alterExternalCatalogPropsFenced(ExternalCatalog externalCatalog, Ca
Integer[] sec = {metadataRefreshIntervalSec,
metadataRefreshIntervalSec};
Env.getCurrentEnv().getRefreshManager().addToRefreshMap(catalogId,
sec);
}
- externalCatalog.modifyCatalogProps(newProps);
// The commit reset the catalog's execution context and closed its SDK
resources. Cached
// base generations and projections are bound to the replaced context;
retire them now so
// the next statement loads a generation the planning fences accept,
instead of retrying
- // against an unplannable cached generation until managed refresh.
+ // against an unplannable cached generation until managed refresh. The
properties are
+ // published before the reset's throwable cleanup, so retirement must
run either way.
Env currentEnv = Env.getCurrentEnv();
ExternalMetaCacheMgr cacheMgr = currentEnv == null ? null :
currentEnv.getExtMetaCacheMgr();
- if (cacheMgr != null) {
-
cacheMgr.onCatalogOperationalContextChanged(externalCatalog.getId());
+ try {
+ externalCatalog.modifyCatalogProps(newProps);
Review Comment:
[P1] Complete the property-specific transition when reset cleanup throws.
The properties have already been published here, and the outer finally now
journals them, but a JDBC `closeClient()` exception exits
`notifyPropertiesUpdated` before `JdbcExternalCatalog.resetToUninitialized`
rebuilds `identifierMapping` and before the generic hook removes cache groups
for schema TTL, catalog weight, or `meta.cache.*` changes. This fallback only
empties entries, so the leader can keep old name-mapping semantics and old
cache policy for a durably committed ALTER (while a follower that replays
cleanly uses the new values). Preserve the cleanup exception if needed, but
still run the connector-derived and property-specific post-publication
transitions; add a throwing-close regression that verifies the new mapping and
rebuilt policy group.
--
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]
