morrySnow commented on code in PR #66673:
URL: https://github.com/apache/doris/pull/66673#discussion_r3765833717
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/Command.java:
##########
@@ -51,6 +51,32 @@ protected Command(PlanType type) {
public abstract void run(ConnectContext ctx, StmtExecutor executor) throws
Exception;
+ /**
+ * Execute this command through the shared command lifecycle.
+ */
+ public final void execute(ConnectContext ctx, StmtExecutor executor)
throws Exception {
+ Throwable failure = null;
Review Comment:
The `before -> run -> after` lifecycle currently adds dead code: `after()`
is an empty stub and the `Throwable failure` variable is only consumed by it,
so the whole try/catch/finally is functionally equivalent to `before(ctx);
run(ctx, executor);`. Per the repo coding standards (no speculative/defensive
code), I'd suggest either implementing a real hook now or simplifying to just:
```java
public final void execute(ConnectContext ctx, StmtExecutor executor) throws
Exception {
before(ctx);
run(ctx, executor);
}
```
If a hook is intentionally reserved for later, that intent should be stated
explicitly; right now the error-path tracking (`catch (Exception | Error e) {
failure = e; throw e; }`) only exists to feed a no-op.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterResourceCommand.java:
##########
@@ -33,7 +33,7 @@
/**
* Command for ALTER RESOURCE in Nereids.
*/
-public class AlterResourceCommand extends AlterCommand implements
NeedAuditEncryption {
+public class AlterResourceCommand extends AlterCommand implements
NeedAuditEncryption, CloudUnsupportedCommand {
Review Comment:
Behavior-change confirmation: marking `ALTER RESOURCE`/`ALTER STORAGE
POLICY` as `CloudUnsupportedCommand` hard-rejects them in cloud mode for
**all** users, including root, while `CREATE RESOURCE`, `DROP RESOURCE`,
`CREATE STORAGE POLICY` and `DROP STORAGE POLICY` remain unrestricted. Neither
the legacy `AlterResourceStmt` nor the current code had any cloud restriction
before, so in cloud mode `ALTER RESOURCE` previously executed successfully
(e.g., rotating S3 credentials by altering resource properties). This is a
user-visible break for such workflows and is asymmetric with the CREATE/DROP
counterparts. If resources/policies are considered manageable in cloud mode at
all, `CloudRootOnlyCommand` may be the safer choice over
`CloudUnsupportedCommand`; please confirm the hard rejection (even for root) is
intended.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/InternalSchemaInitializer.java:
##########
@@ -270,7 +270,7 @@ public static void modifyTblReplicaCount(Database database,
String tblName) {
ops.add(new
ModifyPartitionOp(Lists.newArrayList(tbl.getPartitionNames()), props, false));
AlterTableCommand alterTableCommand = new
AlterTableCommand(tableNameInfo, ops);
- alterTableCommand.run(ConnectContext.get(), null);
+ alterTableCommand.execute(ConnectContext.get(),
null);
Review Comment:
Related to the `run()` -> `execute()` conversion here: the other command
invocation in this same file, `createTable()` at line 516, still calls
`((CreateTableCommand) parsed).run(r.connectContext, stmtExecutor)` directly
and was missed by the migration. No functional impact today since
`CreateTableCommand` isn't cloud-restricted, but it's exactly the kind of
direct `run()` call the new lifecycle was introduced to eliminate — if
`CreateTableCommand` ever implements a restricted marker (e.g., internal stats
table creation on cloud), this path would silently bypass it. Suggest
converting it to `execute()` for consistency.
--
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]
