HappenLee commented on code in PR #67673:
URL: https://github.com/apache/doris/pull/67673#discussion_r4059230977
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/AIResource.java:
##########
@@ -117,20 +144,25 @@ public void modifyProperties(Map<String, String>
properties) throws DdlException
LOG.debug("AI resource need check validity: {}", needCheck);
}
+ Map<String, String> changedProperties = new HashMap<>(this.properties);
Review Comment:
[P2] Keep the full resource read-modify-write under the write lock
The merged snapshot is built before `writeLock()`, but the commit now
replaces the entire `this.properties` map. Two concurrent ALTERs can therefore
both copy the same original map: A changes `ai.temperature`, B changes
`ai.max_token`, A installs its snapshot, and B then installs its snapshot and
silently restores the old temperature. Both statements can succeed and
increment the version. This is reachable through separate client sessions:
`AlterResourceCommand.doRun()` and `ResourceMgr.alterResource()` do not provide
an outer lock. A valid LOCAL resource is sufficient to demonstrate the
interleaving without involving validity-check failures.
Previously, each ALTER applied only its entries to the current map while
holding the write lock, so non-overlapping updates were retained. Please hold
the same write lock across reading the current properties, merging,
normalizing/validating, installing the snapshot, and incrementing the version,
with unlock in `finally`. The validation here is local field checking, so this
does not require holding the lock across remote requests. Add a controlled
concurrent-ALTER test asserting that both independent updates survive.
This finding is based on the current code path and interleaving analysis; I
have not run a cluster reproduction.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/AIResource.java:
##########
@@ -117,20 +144,25 @@ public void modifyProperties(Map<String, String>
properties) throws DdlException
LOG.debug("AI resource need check validity: {}", needCheck);
}
+ Map<String, String> changedProperties = new HashMap<>(this.properties);
+ for (Map.Entry<String, String> kv : properties.entrySet()) {
+ replaceIfEffectiveValue(changedProperties, kv.getKey(),
kv.getValue());
+ if (kv.getKey().equals(AIProperties.API_KEY)
+ || kv.getKey().equals(AIProperties.EMBED_API_KEY)
+ ||
kv.getKey().equals(AIProperties.MULTIMODAL_EMBED_API_KEY)) {
+ changedProperties.put(kv.getKey(), kv.getValue());
+ } else if (kv.getKey().equals(AIProperties.EFFORT)
+ && Strings.isNullOrEmpty(kv.getValue())) {
+ changedProperties.remove(kv.getKey());
+ }
+ }
if (needCheck) {
Review Comment:
[P2] Normalize dedicated providers even when ALTER validity checking is
skipped
`requiredAIProperties()` also performs provider normalization, so guarding
it with `needCheck` leaves valid lowercase providers unnormalized.
`isNeedCheck()` always returns false when the existing general provider is
LOCAL; explicitly setting `ai.validity_check=false` has the same effect.
For example, create a resource with a complete general LOCAL group, then add
a complete dedicated group through ALTER:
```sql
ALTER RESOURCE "local_chat" PROPERTIES (
"ai.embed.provider_type" = "openai",
"ai.embed.endpoint" = "http://127.0.0.1:8000/v1/embeddings";,
"ai.embed.model_name" = "text-embedding-3-small",
"ai.embed.api_key" = "placeholder"
);
SELECT EMBED('local_chat', 'hello');
```
The ALTER persists lowercase `openai`. Both FE and BE accept the complete
dedicated group, but `AIAdapterFactory` only registers uppercase `OPENAI`, so
`_init_from_resource()` gets a null adapter and reaches `DORIS_CHECK(adapter)`
before sending HTTP. This fails the query in Release builds and hits LOG(FATAL)
in builds without NDEBUG. The same issue applies to
`ai.embed.mm.provider_type`. Supplying the same lowercase group during CREATE
does normalize it correctly.
Please always perform local group-shape validation and provider
normalization, independently of any optional validity checking. Cover both a
general LOCAL resource and explicit `ai.validity_check=false` in ALTER tests,
including persisted/toThrift values and adapter selection. The current
normalization test only exercises `needCheck=true`.
The SQL above is a code-derived reproduction scenario with placeholder
connection settings; I have not executed it against a running cluster.
--
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]
