<feature>[core]: support resnotify webhook infrastructure#3477
Closed
ZStack-Robot wants to merge 2 commits into5.5.12from
Closed
<feature>[core]: support resnotify webhook infrastructure#3477ZStack-Robot wants to merge 2 commits into5.5.12from
ZStack-Robot wants to merge 2 commits into5.5.12from
Conversation
Add CopyOnWriteArrayList for thread-safe entity lifecycle callbacks in DatabaseFacadeImpl. Add uninstallEntityLifeCycleCallback method. Add ResNotify DB schema with FK and indexes. Change-Id: Ia3667e35d74c06796946bb805aba4db5c87a3052
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (2)
Walkthrough新增资源通知相关数据库表（ResNotifySubscriptionVO、ResNotifyWebhookRefVO），并将实体生命周期回调从单一回调改为每事件可挂载多个回调，新增卸载回调的公共接口。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
🧹 Nitpick comments (2)
core/src/main/java/org/zstack/core/db/DatabaseFacadeImpl.java (2)
909-921:uninstall与install在 EntityInfo 为空时的行为不一致
installEntityLifeCycleCallback（第 900 行）在找不到EntityInfo时会触发断言失败，而uninstallEntityLifeCycleCallback则静默跳过。这种不一致可能会隐藏调用方传入错误类的 bug。♻️ 建议添加警告日志保持行为一致
`@Override` public void uninstallEntityLifeCycleCallback(Class clz, EntityEvent evt, EntityLifeCycleCallback cb) { if (clz != null) { EntityInfo info = entityInfoMap.get(clz); if (info != null) { info.uninstallLifeCycleCallback(evt, cb); + } else { + logger.warn(String.format("cannot find EntityInfo for the class[%s] when uninstalling lifecycle callback", clz)); } } else { for (EntityInfo info : entityInfoMap.values()) { info.uninstallLifeCycleCallback(evt, cb); } } }🤖 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/db/DatabaseFacadeImpl.java` around lines 909 - 921, uninstallEntityLifeCycleCallback currently silently skips when entityInfoMap.get(clz) returns null, but installEntityLifeCycleCallback asserts on missing EntityInfo; make behavior consistent by checking EntityInfo for null and either assert or log a warning before returning; update the method uninstallEntityLifeCycleCallback to mirror installEntityLifeCycleCallback's handling (use EntityInfo info = entityInfoMap.get(clz); if (info == null) { throw new AssertionError(...) } or at minimum processLogger.warn(...) with class name), then call info.uninstallLifeCycleCallback(evt, cb) when present (references: uninstallEntityLifeCycleCallback, installEntityLifeCycleCallback, EntityInfo, entityInfoMap, uninstallLifeCycleCallback).
457-469:contains+add存在轻微的 TOCTOU 竞态条件在并发场景下，两个线程可能同时通过
contains检查后都执行add，导致重复的回调被注册。由于使用了
CopyOnWriteArrayList，最坏情况仅为回调重复执行，影响较小。如需完全避免，可考虑使用addIfAbsent方法（CopyOnWriteArrayList原生支持）：♻️ 建议使用原子操作
void installLifeCycleCallback(EntityEvent evt, EntityLifeCycleCallback l) { List<EntityLifeCycleCallback> cbs = listeners.computeIfAbsent(evt, k -> new CopyOnWriteArrayList<>()); - if (!cbs.contains(l)) { - cbs.add(l); - } + ((CopyOnWriteArrayList<EntityLifeCycleCallback>) cbs).addIfAbsent(l); }🤖 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/db/DatabaseFacadeImpl.java` around lines 457 - 469, The installLifeCycleCallback method currently does a contains check then add on the CopyOnWriteArrayList stored in listeners, which can race and register duplicates; change the logic in installLifeCycleCallback to call the CopyOnWriteArrayList atomic method addIfAbsent (rather than contains + add) so duplicate callbacks for an EntityEvent/EntityLifeCycleCallback pair are avoided; keep uninstallLifeCycleCallback as-is but ensure it still operates on listeners.get(evt) and removes the callback if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/main/java/org/zstack/core/db/DatabaseFacadeImpl.java`:
- Around line 909-921: uninstallEntityLifeCycleCallback currently silently skips
when entityInfoMap.get(clz) returns null, but installEntityLifeCycleCallback
asserts on missing EntityInfo; make behavior consistent by checking EntityInfo
for null and either assert or log a warning before returning; update the method
uninstallEntityLifeCycleCallback to mirror installEntityLifeCycleCallback's
handling (use EntityInfo info = entityInfoMap.get(clz); if (info == null) {
throw new AssertionError(...) } or at minimum processLogger.warn(...) with class
name), then call info.uninstallLifeCycleCallback(evt, cb) when present
(references: uninstallEntityLifeCycleCallback, installEntityLifeCycleCallback,
EntityInfo, entityInfoMap, uninstallLifeCycleCallback).
- Around line 457-469: The installLifeCycleCallback method currently does a
contains check then add on the CopyOnWriteArrayList stored in listeners, which
can race and register duplicates; change the logic in installLifeCycleCallback
to call the CopyOnWriteArrayList atomic method addIfAbsent (rather than contains
+ add) so duplicate callbacks for an EntityEvent/EntityLifeCycleCallback pair
are avoided; keep uninstallLifeCycleCallback as-is but ensure it still operates
on listeners.get(evt) and removes the callback if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 5c235990-161f-4ec7-952f-9a81acff5a22
📒 Files selected for processing (2)
conf/db/upgrade/V5.5.12__schema.sqlcore/src/main/java/org/zstack/core/db/DatabaseFacadeImpl.java
Add uninstallEntityLifeCycleCallback to DatabaseFacade interface. Generate SDK action classes and ApiHelper methods for resnotify webhook subscription CRUD APIs. Change-Id: I9191952ca0a0959644bc2d90d539a818cf13d30a
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
3 participants
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.
ZSTAC-80472: Resource Notification Webhook - Core Infrastructure
Changes
core/dbconf/dbNotes
Related: ZSTAC-80472
sync from gitlab !9337