github-actions[bot] commented on code in PR #65514:
URL: https://github.com/apache/doris/pull/65514#discussion_r3585866356
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Resource.java:
##########
@@ -258,13 +258,23 @@ public void dropResource() throws DdlException {
@Override
public String toString() {
- return GsonUtils.GSON.toJson(this);
+ readLock();
+ try {
+ return GsonUtils.GSON.toJson(this);
+ } finally {
+ readUnlock();
+ }
}
@Override
public void write(DataOutput out) throws IOException {
- String json = GsonUtils.GSON.toJson(this);
- Text.writeString(out, json);
+ readLock();
Review Comment:
Locking `Resource.write()` here only protects callers that serialize a
`Resource` directly. The live `/dump` path still goes through
`MetaService.dump()` -> `Env.dumpImage()` -> `saveResources()` ->
`ResourceMgr.write()`, and `ResourceMgr.write()` Gson-serializes the whole
manager (`nameToResource`) without invoking `Resource.write()` or taking each
resource's read lock. Since `ALTER RESOURCE` does not take the Env/db/table
locks used by `dumpImage()`, a live dump can still iterate
`HdfsResource.properties` while this PR's `modifyProperties()` is mutating the
same `HashMap` under only the resource write lock, so the original concurrent
serialization failure remains on that path. Please make `ResourceMgr.write()`
snapshot or serialize each resource under its read lock, and add a concurrent
`/dump` or `ResourceMgr.write()` vs HDFS alter test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/ResourceMgr.java:
##########
@@ -150,10 +151,22 @@ public void alterResource(String resourceName,
Map<String, String> properties) t
}
Resource resource = nameToResource.get(resourceName);
- resource.modifyProperties(properties);
+ EditLogItem editLogItem;
Review Comment:
This write lock now wraps the entire subclass `modifyProperties()` call for
every resource type. For S3 and Azure, `modifyProperties()` performs validity
checking before its old inner write-lock section, and that check calls
`pingS3`/`pingAzure`, which does remote put/head/list/multipart/delete
operations. With the new outer lock, an `ALTER RESOURCE` using the default
validity check can hold the live resource write lock across object-store I/O,
blocking same-resource readers that correctly take `readLock()` such as SHOW
RESOURCE/proc output, `Resource.write()`/`toString()`, and
`PushStoragePolicyTask`. Please build and validate a candidate property map
outside the live resource write lock, then take the lock only to recheck/apply
the mutation, clone/snapshot, and enqueue the edit log.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Resource.java:
##########
@@ -258,13 +258,23 @@ public void dropResource() throws DdlException {
@Override
public String toString() {
- return GsonUtils.GSON.toJson(this);
+ readLock();
+ try {
+ return GsonUtils.GSON.toJson(this);
+ } finally {
+ readUnlock();
+ }
}
@Override
public void write(DataOutput out) throws IOException {
- String json = GsonUtils.GSON.toJson(this);
- Text.writeString(out, json);
+ readLock();
Review Comment:
This read lock does not cover all mutable state that
`GsonUtils.GSON.toJson(this)` serializes. `Resource.references` is also an
annotated `HashMap`, but `addReference()` and `removeReference()` mutate it
under `synchronized` methods, not under this resource read/write lock. A
concurrent `CREATE/DROP STORAGE POLICY` can therefore update the reference map
while `ResourceMgr.alterResource()` is cloning this resource through
`Resource.write()`, so the edit-log snapshot can still hit a
`ConcurrentModificationException` or capture a racing reference set. Please put
`references` under the same lock discipline as the serialized resource state,
or snapshot it under the monitor used by the reference methods before
cloning/logging, and add a concurrent alter-vs-policy reference update test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/ResourceMgr.java:
##########
@@ -150,10 +151,22 @@ public void alterResource(String resourceName,
Map<String, String> properties) t
}
Resource resource = nameToResource.get(resourceName);
- resource.modifyProperties(properties);
+ EditLogItem editLogItem;
+ resource.writeLock();
+ try {
+ resource.modifyProperties(properties);
+ Resource resourceSnapshot = resource.clone();
+ if (resourceSnapshot == null) {
+ throw new IllegalStateException("Failed to snapshot resource "
+ resourceName);
+ }
Review Comment:
This call is still inside the resource write lock. In the normal
batch-edit-log path `submitEdit()` only enqueues, but when
`enable_batch_editlog=false` it falls back to `logEditDirectly()` and runs
`journal.write()` synchronously before returning the already-finished item. In
that supported configuration the following comment is not true: the ALTER can
still block on journal I/O while holding the live resource lock, and
same-resource readers wait behind it. Please either move the synchronous write
out of the resource lock with an ordering mechanism that still preserves ALTER
order, or make the comment and locking contract explicitly batch-mode-only.
--
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]
