<fix>[core]: improve SimpleFlowChain#3548
Open
zstack-robot-1 wants to merge 1 commit intozsv_5.0.0from
Open
Conversation
* Added named constructors, factory methods, `then/error/done` overloads, and asynchronous backup collection to SimpleFlowChain; * Added the FlowBuilder stream builder and the `Flow.of` factory method to Flow, allowing for the configuration of `skip/run/rollback` behavior and the generation of anonymous Flow implementations. Related: ZSV-5936 Change-Id: I776b7374716365687279697063726f7a7167736f
核心概览为 变更清单
代码审查工作量评估🎯 3 (中等) | ⏱️ ~20 分钟 兔子的庆祝诗
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/core/workflow/Flow.java (1)
124-125: 给Flow.of(String)补上 Javadoc 和调用约束说明这是新的公开入口，但现在
handle()必填、默认rollback()语义以及skipIf/runIf的覆盖关系都只能靠读实现推断。把这些约束写到 Javadoc 上会更稳，接口方法上的显式public也可以顺手去掉。As per coding guidelines "接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/core/workflow/Flow.java` around lines 124 - 125, 为 Flow.of(String) 补充完整的 Javadoc，说明这是创建新 Flow 的公共入口并列出约束：调用者必须在返回的 FlowBuilder 上调用 handle()（handle() 为必填）；说明默认的 rollback() 语义（例如若未设置则为 no-op 或回滚责任归属）；明确 skipIf/runIf 的覆盖/优先级关系（例如后设置的覆盖先设置的或相反）；同时在接口方法上移除多余的 public 修饰符以符合约定；在说明中引用 Flow.of(String)、FlowBuilder、handle(), rollback(), skipIf(), runIf() 等标识以便定位。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/zstack/core/workflow/SimpleFlowChain.java`:
- Around line 261-274: The chain-level propagateExceptionTo() currently only
wires asyncBackups into the new convenience overloads
(error(Consumer<ErrorCode>) and done(Runnable)), leaving old handlers
(error(FlowErrorHandler), done(FlowDoneHandler), and any setFlow*Handler(...)
variants) unaware of asyncBackups; modify SimpleFlowChain so that when
propagateExceptionTo() mode is active you either (A) wrap legacy handlers
(FlowErrorHandler, FlowDoneHandler) with a thin adapter that receives the
chain's asyncBackups (reuse AsyncBackup firstAsyncBackup and otherAsyncBackups
logic) and delegates to the original handler, or (B) explicitly reject
registration of legacy handlers by throwing an IllegalStateException if
propagateExceptionTo() is enabled; update both error(FlowErrorHandler) and
done(FlowDoneHandler)/setFlow*Handler(...) entry points to apply the same
wrapping or rejection so chain-level propagateExceptionTo() behavior is
consistent across all handler overloads.
In `@header/src/main/java/org/zstack/header/core/workflow/Flow.java`:
- Around line 49-56: The issue: flows skipped via FlowBuilder.skipIf/runIf are
still being pushed into rollBackFlows by SimpleFlowChain.runFlow()/next(),
causing rollback(...) to run for flows that never executed. Fix by ensuring
skipped flows are not enqueued for rollback: in
SimpleFlowChain.runFlow()/next(), only push currentFlow into rollBackFlows when
its run() actually executes (or after successful run start), or add an
"executed" flag on the Flow instance and check it before adding to rollBackFlows
or before performing rollback; update the logic that handles the skip branch to
call next() without enqueuing the skipped Flow (or pass a flag to next() to
avoid push). Reference FlowBuilder.skipIf/runIf, SimpleFlowChain.runFlow(),
next(), rollBackFlows, and rollback(...) when making the change.
---
Nitpick comments:
In `@header/src/main/java/org/zstack/header/core/workflow/Flow.java`:
- Around line 124-125: 为 Flow.of(String) 补充完整的 Javadoc，说明这是创建新 Flow
的公共入口并列出约束：调用者必须在返回的 FlowBuilder 上调用 handle()（handle() 为必填）；说明默认的 rollback()
语义（例如若未设置则为 no-op 或回滚责任归属）；明确 skipIf/runIf
的覆盖/优先级关系（例如后设置的覆盖先设置的或相反）；同时在接口方法上移除多余的 public 修饰符以符合约定；在说明中引用
Flow.of(String)、FlowBuilder、handle(), rollback(), skipIf(), runIf() 等标识以便定位。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8417172f-ed13-47a4-b48d-dab25a9376ac
📒 Files selected for processing (2)
core/src/main/java/org/zstack/core/workflow/SimpleFlowChain.javaheader/src/main/java/org/zstack/header/core/workflow/Flow.java
Comment on lines
+261
to
+274
| @SuppressWarnings("rawtypes") | ||
| public SimpleFlowChain error(Consumer<ErrorCode> handler) { | ||
| AsyncBackup firstAsyncBackup = this.asyncBackups.isEmpty() ? null : this.asyncBackups.get(0); | ||
| AsyncBackup[] otherAsyncBackups = this.asyncBackups.isEmpty() ? new AsyncBackup[0] : | ||
| this.asyncBackups.subList(1, this.asyncBackups.size()).toArray(new AsyncBackup[0]); | ||
|
|
||
| DebugUtils.Assert(handler != null, "handler of errorHandler should not be null"); | ||
| return error(new FlowErrorHandler(firstAsyncBackup, otherAsyncBackups) { | ||
| @Override | ||
| public void handle(ErrorCode errCode, Map data) { | ||
| handler.accept(errCode); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
propagateExceptionTo() 目前只对新便捷重载生效，旧 handler 入口会静默失效
这里收集到的 asyncBackups 只被接到了 error(Consumer<ErrorCode>) 和 done(Runnable) 上；如果调用方继续使用现有的 error(FlowErrorHandler)、done(FlowDoneHandler) 或 setFlow*Handler(...)，链级别的 propagateExceptionTo() 配置不会生效，而且没有任何提示。建议要么统一在旧入口里做包装，要么在启用该配置后显式拒绝旧重载，避免链级配置静默无效。
Also applies to: 321-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/zstack/core/workflow/SimpleFlowChain.java` around
lines 261 - 274, The chain-level propagateExceptionTo() currently only wires
asyncBackups into the new convenience overloads (error(Consumer<ErrorCode>) and
done(Runnable)), leaving old handlers (error(FlowErrorHandler),
done(FlowDoneHandler), and any setFlow*Handler(...) variants) unaware of
asyncBackups; modify SimpleFlowChain so that when propagateExceptionTo() mode is
active you either (A) wrap legacy handlers (FlowErrorHandler, FlowDoneHandler)
with a thin adapter that receives the chain's asyncBackups (reuse AsyncBackup
firstAsyncBackup and otherAsyncBackups logic) and delegates to the original
handler, or (B) explicitly reject registration of legacy handlers by throwing an
IllegalStateException if propagateExceptionTo() is enabled; update both
error(FlowErrorHandler) and done(FlowDoneHandler)/setFlow*Handler(...) entry
points to apply the same wrapping or rejection so chain-level
propagateExceptionTo() behavior is consistent across all handler overloads.
Comment on lines
+49
to
+56
| public FlowBuilder skipIf(Predicate<Map> predicate) { | ||
| return withSkipPredicate(predicate); | ||
| } | ||
|
|
||
| public FlowBuilder runIf(Predicate<Map> predicate) { | ||
| DebugUtils.Assert(predicate != null, "predicate of FlowBuilder.runIf() should not be null"); | ||
| return withSkipPredicate(predicate.negate()); | ||
| } |
There was a problem hiding this comment.
skipIf/runIf 和 rollback(...) 的组合会回滚一个从未执行过的 flow
当前 SimpleFlowChain.runFlow() 的 skip 分支最终仍会走 next()，而 next() 会把 currentFlow 压进 rollBackFlows。这样一来，只要这里给可跳过的 flow 配了 rollback(...)，下游失败时就会对一个从未执行过 run() 的步骤执行回滚。建议在执行器层面避免把被跳过的 flow 入栈，否则这个新 API 很容易产生错误补偿。
Also applies to: 70-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@header/src/main/java/org/zstack/header/core/workflow/Flow.java` around lines
49 - 56, The issue: flows skipped via FlowBuilder.skipIf/runIf are still being
pushed into rollBackFlows by SimpleFlowChain.runFlow()/next(), causing
rollback(...) to run for flows that never executed. Fix by ensuring skipped
flows are not enqueued for rollback: in SimpleFlowChain.runFlow()/next(), only
push currentFlow into rollBackFlows when its run() actually executes (or after
successful run start), or add an "executed" flag on the Flow instance and check
it before adding to rollBackFlows or before performing rollback; update the
logic that handles the skip branch to call next() without enqueuing the skipped
Flow (or pass a flag to next() to avoid push). Reference
FlowBuilder.skipIf/runIf, SimpleFlowChain.runFlow(), next(), rollBackFlows, and
rollback(...) when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
then/error/doneoverloads,and asynchronous backup collection to SimpleFlowChain;
Flow.offactory methodto Flow, allowing for the configuration of
skip/run/rollbackbehaviorand the generation of anonymous Flow implementations.
Related: ZSV-5936
Change-Id: I776b7374716365687279697063726f7a7167736f
sync from gitlab !9402