github-actions[bot] commented on code in PR #65670:
URL: https://github.com/apache/doris/pull/65670#discussion_r3587699466
##########
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 lock now covers the whole subclass `modifyProperties()` call, including
remote validation. For S3 and Azure resources, `modifyProperties()` can call
`pingS3()`/`pingAzure()` before taking the resource write lock or changing
local state, and those pings perform multiple object-store operations. At the
same time, checkpoint image serialization now reaches `Resource.toJson()`,
which waits on this same `alterLock`. So a checkpoint can block behind slow
remote validation even though the old resource state is still safe to snapshot
until the actual mutate/snapshot/enqueue phase starts. Please keep remote
validation outside the ordered critical section, then reacquire the resource
ordering lock to recheck any state that matters, mutate, snapshot, and enqueue
the edit log.
##########
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:
This still leaves the create journal entry using the live `Resource`.
`createResource()` publishes the object in `nameToResource` before
`logCreateResource(resource)` queues it, and in batch edit-log mode the queue
holds that `Writable` until the flusher later serializes it in
`JournalBatch.addJournal()`. A concurrent `ALTER RESOURCE` can therefore find
the just-created resource, mutate the same object, and enqueue its detached
ALTER snapshot after the create entry. If the FE crashes after the
OP_CREATE_RESOURCE entry is flushed but before the OP_ALTER_RESOURCE entry is
flushed, replay recreates the altered properties from the create record alone,
making an uncommitted alter durable. Please log a detached create snapshot as
well, ideally before the resource becomes visible or under the same
per-resource ordering lock.
--
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]
