gavinchou commented on code in PR #65670:
URL: https://github.com/apache/doris/pull/65670#discussion_r3592659894
##########
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:
Addressed in 23b7dbcbd36. DROP now takes the same per-resource alterLock,
rechecks map identity, submits OP_DROP_RESOURCE before exact map removal, and
awaits outside the lock. ALTER also rechecks identity. Replay ignores ALTER for
an absent resource or a replacement with a different ID. A deterministic
ALTER/DROP test verifies submit order, lock-free await, normal replay, and
stale ALTER after same-name recreation.
##########
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:
Addressed in 23b7dbcbd36. ALTER snapshots explicitly clear references
because policy/catalog journals own those mutations. replayAlterResource()
copies references from the current live resource instead of replacing them from
the ALTER payload. The test verifies the payload has no blocking policy
reference and replay preserves the policy-owned reference.
##########
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:
The checkpoint part remains resolved on the current head.
Resource.toString()/write() do not acquire alterLock, and ResourceMgr.write()
now takes per-resource state snapshots without alterLock. The blocked-S3 test
serializes and reads a full ResourceMgr image while pingS3 is waiting. Remote
validation is outside the state read/write lock; alterLock only orders
same-resource lifecycle DDL.
##########
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:
Addressed in 23b7dbcbd36. AzureResource.properties now has
@SerializedName(value = "properties"). ResourcePersistTest mutates a non-empty
Azure region with validity checks disabled, performs the Gson write/read round
trip, and verifies the property survives.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/ResourceMgr.java:
##########
@@ -235,6 +243,16 @@ public void write(DataOutput out) throws IOException {
Text.writeString(out, json);
}
+ private Resource getAlterLogResource(Resource resource) {
+ try {
+ return resource.getCopiedResourceSnapshot();
+ } catch (Throwable t) {
Review Comment:
Addressed in 23b7dbcbd36. ResourceMgr.write() now serializes a detached
manager assembled from per-resource snapshots, coordinating both state fields
and references. Tests cover a full ResourceMgr write blocked by an HDFS state
writer and a full ResourceMgr write that completes while S3 validation is
blocked, so this does not reintroduce the remote-I/O lock dependency.
##########
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()) {
+ resource.modifyProperties(properties);
+ // Snapshot and enqueue under the same lock to preserve concurrent
ALTER order.
+ logResource = getAlterLogResource(resource);
Review Comment:
Revisited with the concrete batch-flush sequence; the earlier scope
assessment was too narrow. Addressed in 23b7dbcbd36: CREATE now snapshots and
submits OP_CREATE_RESOURCE while the resource is still absent from
nameToResource, then publishes the live object. The deterministic test blocks
CREATE submission, verifies the object is not visible, and then checks distinct
CREATE/ALTER payloads in journal order.
--
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]
