github-actions[bot] commented on code in PR #68603:
URL: https://github.com/apache/doris/pull/68603#discussion_r4129370002
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogProperty.java:
##########
@@ -107,37 +109,43 @@ public Map<String, String> getProperties() {
}
/**
- * @return true if varbinary mapping is enabled, false otherwise
+ * @return true because external binary types always retain their binary
semantics
*/
+ @Deprecated
public boolean getEnableMappingVarbinary() {
- return Boolean.parseBoolean(getOrDefault(ENABLE_MAPPING_VARBINARY,
"false"));
+ return true;
Review Comment:
[P1] During a rolling upgrade, this unconditional `true` also takes effect
on a newly upgraded follower while the master is still old. Legacy false or
absent markers are preserved on replay, and the ALTER migration is journaled
only in `transferToMaster`, so the old master and readable new follower can
expose STRING/DATETIME versus VARBINARY/TIMESTAMPTZ for the same catalog. Gate
follower mapping on a durable migration marker, or prevent it from serving the
new schema before the replicated transition.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -433,6 +445,42 @@ public void alterCatalogProps(String catalogName,
Map<String, String> newPropert
}
}
+ /**
+ * Migrate legacy markers after fenced master replay, before accepting
queries or starting checkpoints.
+ */
+ public void migrateVarbinaryMappingProperties() throws DdlException {
+ writeLock();
+ try {
+ for (CatalogIf catalog : idToCatalog.values()) {
+ if (!(catalog instanceof ExternalCatalog)) {
+ continue;
+ }
+ ExternalCatalog externalCatalog = (ExternalCatalog) catalog;
+ Map<String, String> migratedProperties = Maps.newHashMap();
+ for (String marker : new String[]
{CatalogProperty.ENABLE_MAPPING_VARBINARY,
+ CatalogProperty.ENABLE_MAPPING_TIMESTAMP_TZ}) {
+ if
(!Boolean.parseBoolean(externalCatalog.getProperties().get(marker))) {
+ migratedProperties.put(marker, "true");
+ }
+ }
+ if (migratedProperties.isEmpty()) {
+ continue;
+ }
+ CatalogLog log = new CatalogLog();
+ log.setCatalogId(catalog.getId());
+ log.setNewProps(migratedProperties);
+ // Use the existing ALTER format so running older followers
can replay the change.
+ // Journal first: a failed write must leave the marker
eligible for a retry.
+
Env.getCurrentEnv().getEditLog().logCatalogLog(OperationType.OP_ALTER_CATALOG_PROPS,
log);
+ // Migration must not revalidate unrelated legacy connection
properties or contact
+ // the external system while the master is still becoming
ready.
+ replayAlterCatalogProps(log, null, true);
Review Comment:
[P2] This migration already holds `CatalogMgr`'s write lock when it calls
`replayAlterCatalogProps`. That method releases only its nested lock hold
before running deferred access-controller cleanup, so an initialized catalog's
owned `AuthorizationPlugin.close()` still runs under the outer global lock. A
slow close stalls promotion and catalog operations; a close waiting for catalog
work can deadlock. Collect the cleanup and run it after the outer migration
lock is released.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogProperty.java:
##########
@@ -107,37 +109,43 @@ public Map<String, String> getProperties() {
}
/**
- * @return true if varbinary mapping is enabled, false otherwise
+ * @return true because external binary types always retain their binary
semantics
*/
+ @Deprecated
public boolean getEnableMappingVarbinary() {
- return Boolean.parseBoolean(getOrDefault(ENABLE_MAPPING_VARBINARY,
"false"));
Review Comment:
[P1] During a rolling upgrade, this unconditional `true` also takes effect
on a newly upgraded follower while the master is still old. Legacy false or
absent markers are preserved on replay, and the ALTER migration is journaled
only in `transferToMaster`, so the old master and readable new follower can
expose STRING/DATETIME versus VARBINARY/TIMESTAMPTZ for the same catalog. Gate
follower mapping on a durable migration marker, or prevent it from serving the
new schema before the replicated transition.
--
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]
