github-actions[bot] commented on code in PR #66913:
URL: https://github.com/apache/doris/pull/66913#discussion_r3836043009
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -228,15 +244,30 @@ public void invalidateCatalogByEngine(long catalogId,
String engine) {
}
public void removeCatalog(long catalogId) {
- routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "removeCatalog",
- () -> cache.invalidateCatalog(catalogId)));
+ synchronized (catalogLifecycleLock(catalogId)) {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "removeCatalog",
+ () -> cache.invalidateCatalog(catalogId)));
+ }
}
public void removeCatalogByEngine(long catalogId, String engine) {
- routeSpecifiedEngine(engine, cache -> safeInvalidate(
- cache, catalogId, "removeCatalogByEngine",
- () -> cache.invalidateCatalog(catalogId)));
+ synchronized (catalogLifecycleLock(catalogId)) {
+ routeSpecifiedEngine(engine, cache -> safeInvalidate(
+ cache, catalogId, "removeCatalogByEngine",
+ () -> cache.invalidateCatalog(catalogId)));
+ }
+ }
+
+ /**
+ * Fences a catalog runtime transition against lazy cache-group
initialization. The transition callback
+ * must cover both cache removal and the catalog property/runtime
mutation; otherwise an accessor can
+ * snapshot the retiring properties after removal and publish that group
into the new generation.
+ */
+ public void runCatalogLifecycle(long catalogId, Runnable transition) {
Review Comment:
[P1] Include tentative ALTER validation in this fence
The real legacy validation path in `CatalogMgr.replayAlterCatalogProps()`
calls `tryModifyCatalogProps(newProps)` and `checkProperties()` before the
final modify/reset, without holding this lifecycle lock; query lookups do not
take the catalog-manager write lock. A query can therefore enter
`prepareCatalogByEngine()` during validation, snapshot candidate G2 properties,
and load/cache a table using the still-live G1 catalog/ops/executor. If
validation fails, `rollBackCatalogProps()` restores G1 properties but performs
no cache removal or runtime reset, so the mixed generation remains reachable.
Validate against a detached property object, or place tentative mutation,
validation, rollback/final publication, and reset under this same fence.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java:
##########
@@ -984,21 +984,96 @@ public PlSqlOperation getPlSqlOperation() {
// with "Split source X is released". These executors are finalized when
the next query starts
// on this connection, or when the connection is torn down. See #62259.
private final List<StmtExecutor> flightSqlDeferredExecutors = new
ArrayList<>();
+ private boolean flightSqlDeferredExecutorsSealed;
+ private int flightSqlResultPublishers;
- public void addFlightSqlDeferredExecutor(StmtExecutor executor) {
+ public boolean addFlightSqlDeferredExecutor(StmtExecutor executor) {
synchronized (flightSqlDeferredExecutors) {
+ if (flightSqlDeferredExecutorsSealed) {
+ return false;
+ }
flightSqlDeferredExecutors.add(executor);
+ return true;
+ }
+ }
+
+ /** Linearizes GetFlightInfo publication with the terminal session seal. */
+ public boolean canPublishFlightSqlResult() {
+ synchronized (flightSqlDeferredExecutors) {
+ return !flightSqlDeferredExecutorsSealed;
+ }
+ }
+
+ public boolean beginFlightSqlResultPublication() {
+ synchronized (flightSqlDeferredExecutors) {
+ if (flightSqlDeferredExecutorsSealed) {
+ return false;
+ }
+ flightSqlResultPublishers++;
+ return true;
+ }
+ }
+
+ public boolean endFlightSqlResultPublication() {
+ List<StmtExecutor> toClose = null;
+ boolean published;
+ synchronized (flightSqlDeferredExecutors) {
+ published = !flightSqlDeferredExecutorsSealed;
+ if (--flightSqlResultPublishers == 0 &&
flightSqlDeferredExecutorsSealed) {
+ toClose = drainFlightSqlDeferredExecutors();
+ flightSqlDeferredExecutors.notifyAll();
+ }
}
+ finalizeFlightSqlDeferredExecutors(toClose);
+ return published;
}
public void closeFlightSqlDeferredExecutors() {
- List<StmtExecutor> toClose;
+ closeFlightSqlDeferredExecutors(false);
+ }
+
+ /** Prevents a session teardown race from accepting an executor after the
final drain. */
+ public void sealAndCloseFlightSqlDeferredExecutors() {
+ closeFlightSqlDeferredExecutors(true);
+ }
+
+ private void closeFlightSqlDeferredExecutors(boolean seal) {
+ List<StmtExecutor> toClose = null;
synchronized (flightSqlDeferredExecutors) {
- if (flightSqlDeferredExecutors.isEmpty()) {
- return;
+ if (seal) {
+ flightSqlDeferredExecutorsSealed = true;
+ // The result channel is destroyed immediately after this
method returns. Wait until every
+ // admitted publisher has either committed or observed the
seal, so a losing local-result
+ // publisher cannot insert Arrow buffers after the channel's
one-time invalidation.
+ boolean interrupted = false;
+ while (flightSqlResultPublishers != 0) {
Review Comment:
[P1] Avoid a wait-before-cancel teardown cycle
This blocks teardown until every admitted publisher reaches
`endFlightSqlResultPublication()`, but `FlightSqlConnectContext.kill(true)`
reaches this wait through `unregisterConnection()` before it calls
`cancelQuery()`. If the publisher is stuck in query work, the kill therefore
waits forever before sending the signal that could let it finish. The new
publication tests expose the same cycle deterministically by calling
`sealAndCloseFlightSqlDeferredExecutors()` on the admitted publisher thread and
only decrementing below that call. Cancel/signal first and wait from an
independent owner, or make terminal sealing nonblocking/cancel-aware.
--
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]
