github-actions[bot] commented on code in PR #65670:
URL: https://github.com/apache/doris/pull/65670#discussion_r3588053993
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/ResourceMgr.java:
##########
@@ -145,16 +147,22 @@ public void replayDropResource(DropResourceOperationLog
operationLog) {
}
public void alterResource(String resourceName, Map<String, String>
properties) throws DdlException {
- if (!nameToResource.containsKey(resourceName)) {
+ Resource resource = nameToResource.get(resourceName);
+ if (resource == null) {
throw new DdlException("Resource(" + resourceName + ") dose not
exist.");
}
- Resource resource = nameToResource.get(resourceName);
- resource.modifyProperties(properties);
+ Resource logResource;
+ EditLog.EditLogItem logItem;
+ synchronized (resource.getAlterLock()) {
Review Comment:
This still lets a DROP interleave with an in-flight ALTER for the same
resource. `alterResource()` grabs the `Resource` from `nameToResource` before
taking `resource.getAlterLock()`, but `dropResource()` never takes that lock:
it can remove the map entry and enqueue `OP_DROP_RESOURCE` while the ALTER is
still in slow validation. When the ALTER resumes, it mutates the stale object
and submits `OP_ALTER_RESOURCE`; on replay, `replayDropResource()` removes the
name and the later `replayAlterResource()` unconditionally puts the detached
snapshot back, resurrecting a dropped resource or overwriting a recreated one.
Please serialize DROP with the same per-resource ordering lock, recheck that
the map still points to that object under the lock, enqueue the drop while
holding it, and await outside as the ALTER path does.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Resource.java:
##########
@@ -350,12 +357,27 @@ public Resource clone() {
return copied;
}
- private void notifyUpdate(Map<String, String> properties) {
-
references.entrySet().stream().collect(Collectors.groupingBy(Entry::getValue)).forEach((type,
refs) -> {
- if (type == ReferenceType.CATALOG) {
- // No longer support resource in Catalog.
+ final Resource getCopiedResourceSnapshot() {
+ String json;
+ readLock();
+ try {
+ synchronized (this) {
+ json = GsonUtils.GSON.toJson(this);
Review Comment:
This snapshot is not safe for Azure resources. `GsonUtils.GSON` uses
`HiddenAnnotationExclusionStrategy`, so only fields annotated with
`@SerializedName` are written, but `AzureResource.properties` is the live
property map and is not annotated. An `ALTER RESOURCE` on Azure will therefore
mutate the live map, call this helper, and submit an `OP_ALTER_RESOURCE`
snapshot whose copied `AzureResource` has an empty property map; replay then
replaces the valid resource with that empty snapshot. Please annotate
`AzureResource.properties` with `@SerializedName(value = "properties")` and add
a snapshot or Resource write/read test with non-empty Azure properties.
--
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]
