[host]: ping syncSignature per-host to prevent queue serialization#3948
Open
ZStack-Robot wants to merge 3 commits into
Open
[host]: ping syncSignature per-host to prevent queue serialization#3948ZStack-Robot wants to merge 3 commits into
ZStack-Robot wants to merge 3 commits into
Conversation
|
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough本PR修改了Ping主机任务的同步签名，从固定常量改为基于目标主机UUID的动态签名，并添加测试验证新的同步行为。 变更每主机Ping任务同步
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/core/thread/TestDispatchQueuePerHostSignature.java (1)
20-20: ⚡ Quick win测试类命名不符合仓库规范。
Line 20 的类名
TestDispatchQueuePerHostSignature未以Test或Case结尾，建议改为DispatchQueuePerHostSignatureTest，并同步文件名。
As per coding guidelines “测试类需要以 Test 或 Case 结尾。”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/src/test/java/org/zstack/test/core/thread/TestDispatchQueuePerHostSignature.java` at line 20, Rename the test class TestDispatchQueuePerHostSignature to follow the repository convention by changing the class name to DispatchQueuePerHostSignatureTest and update the containing Java file name accordingly; ensure all references to TestDispatchQueuePerHostSignature in imports, test suites, or build configs are updated to the new class name to avoid compilation failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@test/src/test/java/org/zstack/test/core/thread/TestDispatchQueuePerHostSignature.java`:
- Line 20: Rename the test class TestDispatchQueuePerHostSignature to follow the
repository convention by changing the class name to
DispatchQueuePerHostSignatureTest and update the containing Java file name
accordingly; ensure all references to TestDispatchQueuePerHostSignature in
imports, test suites, or build configs are updated to the new class name to
avoid compilation failures.
ℹ️ 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: 0aab5719-69dc-41f5-992e-122f5f52f1dc
📒 Files selected for processing (2)
compute/src/main/java/org/zstack/compute/host/HostBase.javatest/src/test/java/org/zstack/test/core/thread/TestDispatchQueuePerHostSignature.java
Collaborator
|
Comment from yaohua.wu: Review: MR !9839 — ZSTAC-80543P3 — Low
总结本次修复将 Verdict: APPROVED🤖 Robot Reviewer |
added 2 commits
May 19, 2026 01:03
…serialization getSyncSignature() changed from fixed "do-ping-host" to "do-ping-host-<hostUuid>", preventing all physical host pings from queuing on the same SyncTaskChain. Resolves: ZSTAC-83192, ZSTAC-79389
Verify after getSyncSignature() returns per-host key: - same signature serializes execution (same queue) - different signatures run in parallel (different host = different queue) Related: ZSTAC-83192
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80543-v2
branch
from
3db5a92 to
b996d6c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/src/test/java/org/zstack/test/core/thread/DispatchQueuePerHostSignatureTest.java (3)
25-57: ⚡ Quick win建议将魔法值提取为常量以提升可读性。
OrderedTask中存在多处魔法值：
- 第 45 行：
10秒的超时时间- 第 56 行：
getSyncLevel()返回的2没有说明其含义建议将这些值提取为有意义的常量，便于理解和维护。根据编码指南，应避免使用魔法值。
♻️ 建议的优化
public class DispatchQueuePerHostSignatureTest { private static final CLogger logger = Utils.getLogger(DispatchQueuePerHostSignatureTest.class); + private static final int DEFAULT_TIMEOUT_SECONDS = 10; + private static final int SYNC_LEVEL_DEFAULT = 2; ComponentLoader loader; ThreadFacade thdf; static class OrderedTask implements SyncTask<Integer> { // ... fields ... `@Override` public Integer call() throws Exception { executionOrder.add(id); started.countDown(); - letFinish.await(10, TimeUnit.SECONDS); + letFinish.await(DEFAULT_TIMEOUT_SECONDS, TimeUnit.SECONDS); return id; } `@Override` - public int getSyncLevel() { return 2; } + public int getSyncLevel() { return SYNC_LEVEL_DEFAULT; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/src/test/java/org/zstack/test/core/thread/DispatchQueuePerHostSignatureTest.java` around lines 25 - 57, Extract the magic numbers in OrderedTask into named constants: replace the hardcoded 10 in letFinish.await(10, TimeUnit.SECONDS) with a timeout constant (e.g., DEFAULT_WAIT_SECONDS or TASK_AWAIT_SECONDS) and replace the literal 2 returned by getSyncLevel() with a named SYNCHRONIZATION_LEVEL (or similar) so the meaning is clear; update the OrderedTask constructor/class to declare these private static final constants and use them in call() and getSyncLevel() to improve readability and maintainability.
66-91: ⚡ Quick win建议提取重复的超时常量。
测试方法中多次使用相同的超时值（如
10秒和500毫秒），建议提取为常量以提升可维护性。这样可以统一调整测试超时配置，也使代码意图更清晰。♻️ 建议的优化
public class DispatchQueuePerHostSignatureTest { private static final CLogger logger = Utils.getLogger(DispatchQueuePerHostSignatureTest.class); + private static final int DEFAULT_TIMEOUT_SECONDS = 10; + private static final int BLOCKING_CHECK_TIMEOUT_MS = 500; ComponentLoader loader; ThreadFacade thdf; `@Test` public void sameSignatureSerializes() throws Exception { List<Integer> executionOrder = Collections.synchronizedList(new ArrayList<>()); CountDownLatch started1 = new CountDownLatch(1); CountDownLatch letFinish1 = new CountDownLatch(1); Future<Integer> f1 = thdf.syncSubmit(new OrderedTask(1, "same-sig", executionOrder, started1, letFinish1)); - started1.await(10, TimeUnit.SECONDS); + started1.await(DEFAULT_TIMEOUT_SECONDS, TimeUnit.SECONDS); CountDownLatch started2 = new CountDownLatch(1); CountDownLatch letFinish2 = new CountDownLatch(1); Future<Integer> f2 = thdf.syncSubmit(new OrderedTask(2, "same-sig", executionOrder, started2, letFinish2)); // Task 2 should NOT start while Task 1 is still running (same signature = serial) - Assert.assertFalse("task 2 should be blocked by same signature", started2.await(500, TimeUnit.MILLISECONDS)); + Assert.assertFalse("task 2 should be blocked by same signature", started2.await(BLOCKING_CHECK_TIMEOUT_MS, TimeUnit.MILLISECONDS)); letFinish1.countDown(); - Assert.assertEquals(1, (int) f1.get(10, TimeUnit.SECONDS)); + Assert.assertEquals(1, (int) f1.get(DEFAULT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); - started2.await(10, TimeUnit.SECONDS); + started2.await(DEFAULT_TIMEOUT_SECONDS, TimeUnit.SECONDS); Assert.assertEquals("task 1 must execute before task 2", (Integer) 1, executionOrder.get(0)); Assert.assertEquals("task 2 must execute after task 1", (Integer) 2, executionOrder.get(1)); letFinish2.countDown(); - Assert.assertEquals(2, (int) f2.get(10, TimeUnit.SECONDS)); + Assert.assertEquals(2, (int) f2.get(DEFAULT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/src/test/java/org/zstack/test/core/thread/DispatchQueuePerHostSignatureTest.java` around lines 66 - 91, Extract repeated timeout literals into named constants and replace the inline numbers in sameSignatureSerializes: create constants such as DEFAULT_WAIT_SECONDS (used for started1.await(..., TimeUnit.SECONDS), started2.await(..., TimeUnit.SECONDS), f1.get(..., TimeUnit.SECONDS), f2.get(..., TimeUnit.SECONDS)) and SHORT_WAIT_MILLIS (used for started2.await(500, TimeUnit.MILLISECONDS)); update the calls in OrderedTask submissions and assertions to use these constants so timeouts are centralized and maintainable.
3-3: ⚡ Quick win使用 JUnit 4 的 Assert 类以保持一致性。
代码中使用了 JUnit 4 的注解（
@Before、@Test），但导入的是 JUnit 3 的junit.framework.Assert。建议统一使用org.junit.Assert以保持版本一致性。♻️ 建议的修复
-import junit.framework.Assert; +import org.junit.Assert; import org.junit.Before; import org.junit.Test;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/src/test/java/org/zstack/test/core/thread/DispatchQueuePerHostSignatureTest.java` at line 3, Replace the JUnit 3 import with the JUnit 4 Assert import: remove the existing import of junit.framework.Assert and import org.junit.Assert instead so the test class DispatchQueuePerHostSignatureTest (which uses `@Before/`@Test annotations) consistently uses JUnit 4; ensure any references to Assert methods remain valid (or switch to static imports from org.junit.Assert if preferred).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@test/src/test/java/org/zstack/test/core/thread/DispatchQueuePerHostSignatureTest.java`:
- Around line 25-57: Extract the magic numbers in OrderedTask into named
constants: replace the hardcoded 10 in letFinish.await(10, TimeUnit.SECONDS)
with a timeout constant (e.g., DEFAULT_WAIT_SECONDS or TASK_AWAIT_SECONDS) and
replace the literal 2 returned by getSyncLevel() with a named
SYNCHRONIZATION_LEVEL (or similar) so the meaning is clear; update the
OrderedTask constructor/class to declare these private static final constants
and use them in call() and getSyncLevel() to improve readability and
maintainability.
- Around line 66-91: Extract repeated timeout literals into named constants and
replace the inline numbers in sameSignatureSerializes: create constants such as
DEFAULT_WAIT_SECONDS (used for started1.await(..., TimeUnit.SECONDS),
started2.await(..., TimeUnit.SECONDS), f1.get(..., TimeUnit.SECONDS),
f2.get(..., TimeUnit.SECONDS)) and SHORT_WAIT_MILLIS (used for
started2.await(500, TimeUnit.MILLISECONDS)); update the calls in OrderedTask
submissions and assertions to use these constants so timeouts are centralized
and maintainable.
- Line 3: Replace the JUnit 3 import with the JUnit 4 Assert import: remove the
existing import of junit.framework.Assert and import org.junit.Assert instead so
the test class DispatchQueuePerHostSignatureTest (which uses `@Before/`@Test
annotations) consistently uses JUnit 4; ensure any references to Assert methods
remain valid (or switch to static imports from org.junit.Assert if preferred).
ℹ️ 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: a88df3a4-68ef-4c43-8283-e638a2a6c1a1
📒 Files selected for processing (2)
compute/src/main/java/org/zstack/compute/host/HostBase.javatest/src/test/java/org/zstack/test/core/thread/DispatchQueuePerHostSignatureTest.java
✅ Files skipped from review due to trivial changes (1)
- compute/src/main/java/org/zstack/compute/host/HostBase.java
Renamed from TestDispatchQueuePerHostSignature to follow project naming convention (Test suffix, not prefix). Resolves: ZSTAC-80543
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-80543-v2
branch
from
b996d6c to
f56f00d
Compare
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
2 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.
Resolves: ZSTAC-80543
sync from gitlab !9839