Copilot commented on code in PR #8042:
URL: https://github.com/apache/incubator-seata/pull/8042#discussion_r3031206669
##########
test-suite/seata-benchmark-cli/src/main/java/org/apache/seata/benchmark/executor/SagaModeExecutor.java:
##########
@@ -307,10 +330,21 @@ public void init() throws Exception {
// Register services with configured rollback percentage
// Divide rollback percentage by 3 for each service so total
probability is approximately correct
int serviceRollbackPct = rollbackPercentage > 0 ? Math.max(1,
rollbackPercentage / 3) : 0;
-
- serviceInvoker.registerService("orderService", new
OrderSagaService(serviceRollbackPct, 5));
- serviceInvoker.registerService("inventoryService", new
InventorySagaService(serviceRollbackPct, 5));
- serviceInvoker.registerService("paymentService", new
PaymentSagaService(serviceRollbackPct, 5));
+ Random failureRandom =
+ sagaRandomSeed == null ? null : new
Random(sagaRandomSeed);
+
+ serviceInvoker.registerService(
+ "orderService",
+ new OrderSagaService(
+ serviceRollbackPct, 5,
STEP_ORDER.equals(sagaFailStep), failureRandom));
+ serviceInvoker.registerService(
+ "inventoryService",
+ new InventorySagaService(
+ serviceRollbackPct, 5,
STEP_INVENTORY.equals(sagaFailStep), failureRandom));
Review Comment:
`serviceRollbackPct` is always computed as `rollbackPercentage / 3`, but
when `--saga-fail-step` restricts injection to a single forward step, this
makes the *actual* failure probability roughly 1/3 of the configured
`--rollback-percentage` (only one service can fail, and it uses the reduced
percentage). If the intent is that `--rollback-percentage` remains the overall
forward failure ratio even with step targeting, adjust the per-service
percentage based on whether failure is restricted to one step (e.g., use the
full rollbackPercentage for the selected step).
##########
test-suite/seata-benchmark-cli/src/main/java/org/apache/seata/benchmark/executor/SagaModeExecutor.java:
##########
@@ -307,10 +330,21 @@ public void init() throws Exception {
// Register services with configured rollback percentage
// Divide rollback percentage by 3 for each service so total
probability is approximately correct
int serviceRollbackPct = rollbackPercentage > 0 ? Math.max(1,
rollbackPercentage / 3) : 0;
-
- serviceInvoker.registerService("orderService", new
OrderSagaService(serviceRollbackPct, 5));
- serviceInvoker.registerService("inventoryService", new
InventorySagaService(serviceRollbackPct, 5));
- serviceInvoker.registerService("paymentService", new
PaymentSagaService(serviceRollbackPct, 5));
+ Random failureRandom =
+ sagaRandomSeed == null ? null : new
Random(sagaRandomSeed);
+
+ serviceInvoker.registerService(
+ "orderService",
+ new OrderSagaService(
+ serviceRollbackPct, 5,
STEP_ORDER.equals(sagaFailStep), failureRandom));
+ serviceInvoker.registerService(
+ "inventoryService",
+ new InventorySagaService(
+ serviceRollbackPct, 5,
STEP_INVENTORY.equals(sagaFailStep), failureRandom));
+ serviceInvoker.registerService(
+ "paymentService",
+ new PaymentSagaService(
+ serviceRollbackPct, 5,
STEP_PAYMENT.equals(sagaFailStep), failureRandom));
Review Comment:
In real SAGA mode, failure injection is effectively disabled unless
`--saga-fail-step` is set, because `failInjectionEnabled` is computed via
`STEP_*.equals(sagaFailStep)` and `sagaFailStep` is null/empty by default. This
changes existing behavior (rollbackPercentage>0 previously injected failures
across steps) and conflicts with the documented semantics that
`--saga-fail-step` should *restrict* injection rather than enable it. Consider
treating a missing `sagaFailStep` as "all steps" (enable injection for all
services), and only restricting when an explicit step is provided.
##########
test-suite/seata-benchmark-cli/src/main/java/org/apache/seata/benchmark/model/BenchmarkMetrics.java:
##########
@@ -55,6 +59,29 @@ public void recordFailure(long latencyMs) {
addLatencySample(latencyMs);
}
+ public void recordTransaction(String status, long latencyMs) {
+ totalCount.incrementAndGet();
+
+ if (BenchmarkConstants.STATUS_COMMITTED.equals(status)) {
+ successCount.incrementAndGet();
+ committedCount.incrementAndGet();
+ } else if (BenchmarkConstants.STATUS_COMPENSATED.equals(status)) {
+ compensatedCount.incrementAndGet();
+ failedCount.incrementAndGet();
+ } else if
(BenchmarkConstants.STATUS_COMPENSATION_FAILED.equals(status)) {
+ compensationFailedCount.incrementAndGet();
+ failedCount.incrementAndGet();
+ } else if (BenchmarkConstants.STATUS_FAILED.equals(status)
+ || BenchmarkConstants.STATUS_ROLLBACKED.equals(status)) {
+ failedCount.incrementAndGet();
+ } else {
+ unknownCount.incrementAndGet();
+ failedCount.incrementAndGet();
+ }
Review Comment:
With `WorkloadGenerator` now using `recordTransaction(...)` for all
successful executions, `successCount` and `committedCount` are incremented
under the exact same condition (`STATUS_COMMITTED`), so `Success Count` will
always equal `Committed Count` in the final report/CSV. That makes the new
report output redundant/confusing, especially for SAGA where compensated is a
distinct end state. Consider either redefining `successCount` to represent
end-state success (committed + compensated) or removing the duplicated
"Success" lines from the report/CSV.
##########
test-suite/seata-benchmark-cli/README.md:
##########
@@ -232,17 +279,28 @@ When the benchmark completes, a final report is displayed:
===================================================
Seata Benchmark Final Report
===================================================
+Mode: SAGA
+Saga Shape: order
Total Transactions: 6,000
Success Count: 5,940
Failed Count: 60
Success Rate: 99.00%
+Committed Count: 4,860
+Compensated Count: 920
+Execution Failed Count:140
+Compensation Failed Count: 40
+Unknown Count: 0
+Committed Rate: 81.00%
+Compensated Rate: 15.33%
+End-State Success Rate: 96.33%
Average TPS: 100.2
Review Comment:
The README’s sample “Final Report” and CSV output metrics are internally
inconsistent (counts don’t sum to total, and rates don’t match the shown
counts). For example,
`Committed(4860)+Compensated(920)+ExecutionFailed(140)+CompensationFailed(40)=5960`
but `Total Transactions` is `6000`, and the displayed `Success Count/Rate`
don’t align with the committed/compensated breakdown. Please update the example
numbers so totals and rates are mathematically consistent, or explain what each
aggregate represents.
```suggestion
Success Count: 5,780
Failed Count: 220
Success Rate: 96.33%
Committed Count: 4,860
Compensated Count: 920
Execution Failed Count:180
Compensation Failed Count: 40
Unknown Count: 0
Committed Rate: 81.00%
Compensated Rate: 15.33%
End-State Success Rate: 96.33%
Average TPS: 100.0
```
##########
test-suite/seata-benchmark-cli/README.md:
##########
@@ -264,15 +322,26 @@ Output format:
```csv
Metric,Value
+Mode,SAGA
+Saga Shape,order
Total Transactions,6000
Success Count,5940
Failed Count,60
Success Rate (%),99.00
+Committed Count,4860
+Compensated Count,920
+Execution Failed Count,140
+Compensation Failed Count,40
+Unknown Count,0
+Committed Rate (%),81.00
+Compensated Rate (%),15.33
+End-State Success Rate (%),96.33
Average TPS,100.2
Elapsed Time (s),60
Latency P50 (ms),12
Latency P95 (ms),45
Latency P99 (ms),89
+Latency P99.9 (ms),120
Latency Max (ms),230
Export Timestamp,2025-12-01 10:30:45
Review Comment:
README CSV example uses the label `Export Timestamp`, but the implementation
writes `Export Time` (and uses the current time). Please align the
documentation with the actual CSV field name/output to avoid breaking
downstream parsing scripts.
```suggestion
Export Time,2025-12-01 10:30:45
```
##########
test-suite/seata-benchmark-cli/src/main/java/org/apache/seata/benchmark/model/BenchmarkMetrics.java:
##########
@@ -152,9 +223,16 @@ public void reset() {
totalCount.set(0);
successCount.set(0);
failedCount.set(0);
+ committedCount.set(0);
+ compensatedCount.set(0);
+ compensationFailedCount.set(0);
+ unknownCount.set(0);
+ totalSamples.set(0);
latencies.clear();
lastCountSnapshot = 0;
lastSnapshotTime = System.currentTimeMillis();
+ cachedStats = null;
+ lastStatsUpdateTime = 0;
}
Review Comment:
`BenchmarkMetrics.reset()` is called after warmup (see WorkloadGenerator),
but `startTime` is `final` and never reset. As a result, `getAverageTps()` and
`getElapsedTimeSeconds()` continue to include warmup time even after reset,
which makes the final report/CSV metrics inaccurate for the benchmark phase.
Consider making `startTime` mutable (e.g., AtomicLong/volatile long) and
resetting it inside `reset()`.
##########
test-suite/seata-benchmark-cli/src/main/java/org/apache/seata/benchmark/config/BenchmarkConfig.java:
##########
@@ -163,6 +192,31 @@ private void validateRange(int value, int min, int max,
String fieldName) {
}
}
+ private void validateSagaShape() {
+ if (sagaShape == null || sagaShape.trim().isEmpty()) {
+ return;
+ }
+
+ String normalized = sagaShape.trim().toLowerCase();
+ if (!"simple".equals(normalized) && !"order".equals(normalized)) {
+ throw new IllegalArgumentException("sagaShape must be one of:
simple, order");
+ }
+ sagaShape = normalized;
+ }
+
+ private void validateSagaFailStep() {
+ if (sagaFailStep == null || sagaFailStep.trim().isEmpty()) {
+ return;
+ }
+
+ String normalized = sagaFailStep.trim().toLowerCase();
+ if (!"inventory".equals(normalized) && !"payment".equals(normalized)
&& !"order".equals(normalized)) {
+ throw new IllegalArgumentException(
+ "sagaFailStep must be one of: inventory, payment, order");
+ }
+ sagaFailStep = normalized;
+ }
Review Comment:
`validateSagaShape()` / `validateSagaFailStep()` normalize user input via
`toLowerCase()` without specifying a locale. In Java this is locale-sensitive
and can produce unexpected results under certain default locales (e.g.,
Turkish). Use `toLowerCase(Locale.ROOT)` (and import `java.util.Locale`) for
deterministic CLI parsing.
##########
test-suite/seata-benchmark-cli/src/main/java/org/apache/seata/benchmark/executor/SagaModeExecutor.java:
##########
@@ -307,10 +330,21 @@ public void init() throws Exception {
// Register services with configured rollback percentage
// Divide rollback percentage by 3 for each service so total
probability is approximately correct
int serviceRollbackPct = rollbackPercentage > 0 ? Math.max(1,
rollbackPercentage / 3) : 0;
-
- serviceInvoker.registerService("orderService", new
OrderSagaService(serviceRollbackPct, 5));
- serviceInvoker.registerService("inventoryService", new
InventorySagaService(serviceRollbackPct, 5));
- serviceInvoker.registerService("paymentService", new
PaymentSagaService(serviceRollbackPct, 5));
+ Random failureRandom =
+ sagaRandomSeed == null ? null : new
Random(sagaRandomSeed);
+
+ serviceInvoker.registerService(
+ "orderService",
+ new OrderSagaService(
+ serviceRollbackPct, 5,
STEP_ORDER.equals(sagaFailStep), failureRandom));
+ serviceInvoker.registerService(
+ "inventoryService",
+ new InventorySagaService(
+ serviceRollbackPct, 5,
STEP_INVENTORY.equals(sagaFailStep), failureRandom));
+ serviceInvoker.registerService(
+ "paymentService",
+ new PaymentSagaService(
+ serviceRollbackPct, 5,
STEP_PAYMENT.equals(sagaFailStep), failureRandom));
Review Comment:
`failureRandom` is a single shared `Random` instance synchronized on every
`nextInt()` call across all services/threads. Under multi-threaded benchmarks
this can introduce lock contention and also makes the sequence of random values
(and thus failure outcomes) sensitive to thread scheduling, reducing
reproducibility across runs even with the same seed. Consider using a
per-thread or per-transaction deterministic generator (e.g., derive a seed from
`sagaRandomSeed` + thread index or businessKey) to avoid contention and improve
repeatability.
##########
test-suite/seata-benchmark-cli/src/main/java/org/apache/seata/benchmark/monitor/MetricsCollector.java:
##########
@@ -71,10 +86,22 @@ public String generateFinalReport() {
report.append("===================================================\n");
report.append(" Seata Benchmark Final Report\n");
report.append("===================================================\n");
+ report.append(String.format("Mode: %s\n",
config.getMode()));
+ if (config.getSagaShape() != null && !config.getSagaShape().isEmpty())
{
+ report.append(String.format("Saga Shape: %s\n",
config.getSagaShape()));
+ }
report.append(String.format("Total Transactions: %,d\n",
metrics.getTotalCount()));
report.append(String.format("Success Count: %,d\n",
metrics.getSuccessCount()));
report.append(String.format("Failed Count: %,d\n",
metrics.getFailedCount()));
report.append(String.format("Success Rate: %.2f%%\n",
metrics.getSuccessRate()));
+ report.append(String.format("Committed Count: %,d\n",
metrics.getCommittedCount()));
+ report.append(String.format("Compensated Count: %,d\n",
metrics.getCompensatedCount()));
+ report.append(String.format("Execution Failed Count:%,d\n",
metrics.getExecutionFailedCount()));
+ report.append(String.format("Compensation Failed Count: %,d\n",
metrics.getCompensationFailedCount()));
+ report.append(String.format("Unknown Count: %,d\n",
metrics.getUnknownCount()));
+ report.append(String.format("Committed Rate: %.2f%%\n",
metrics.getCommittedRate()));
+ report.append(String.format("Compensated Rate: %.2f%%\n",
metrics.getCompensatedRate()));
+ report.append(String.format("End-State Success Rate: %.2f%%\n",
metrics.getEndStateSuccessRate()));
Review Comment:
`Execution Failed Count` is formatted without a space after the colon
(`"Execution Failed Count:%,d"`), unlike the other aligned lines, which reduces
readability in the final report. Consider adding a space and/or consistent
padding so columns line up.
--
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]
