Skip to content

Split unrelated client summary and Pulsar E2E follow-ups from PR 10491#10922

Draft
davidzollo wants to merge 3 commits into
apache:devfrom
davidzollo:pr10491-unrelated-followups
Draft

Split unrelated client summary and Pulsar E2E follow-ups from PR 10491#10922
davidzollo wants to merge 3 commits into
apache:devfrom
davidzollo:pr10491-unrelated-followups

Conversation

@davidzollo
Copy link
Copy Markdown
Contributor

Summary

Validation

  • ./mvnw -nsu -Dmaven.gitcommitid.skip=true spotless:apply
  • ./mvnw -nsu -Dmaven.gitcommitid.skip=true -Dspotless.check.skip=true -f seatunnel-e2e/seatunnel-connector-v2-e2e/connector-pulsar-e2e/pom.xml -Dtest=CanalToPulsarIT -Dsurefire.failIfNoSpecifiedTests=false test (blocked by missing local Docker / /var/run/docker.sock)
  • ./mvnw -nsu -Dmaven.gitcommitid.skip=true -Dspotless.check.skip=true -f seatunnel-core/seatunnel-starter/pom.xml -DskipTests package (blocked by existing upstream DryRun compile issue in ClientCommandArgs, unrelated to this one-line change)

Notes

Copy link
Copy Markdown
Contributor

@DanielLeens DanielLeens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting these follow-up pieces out of #10491. I reviewed the full diff on the latest head (2337be1756b2354b5aa7dca4b0d11bcf1969d673), traced both paths locally, and I do not see a reopened source-level blocker here.

What this PR fixes

  • User pain: the CLI could print a negative Total Failed Count when sinkWriteCount > sourceReadCount, and the Pulsar Canal E2E still relied on a blind Thread.sleep(10s) before asserting downstream results.
  • Fix approach: ClientExecuteCommand now clamps that derived display metric to 0, and CanalToPulsarIT now waits for real data to appear in the Pulsar topic before continuing.
  • One-line summary: the first change removes a misleading negative summary value, and the second change replaces a timing guess with a real readiness check.

Runtime / execution chain I checked

CLI summary path
  -> ClientExecuteCommand.execute() waits for job completion
  -> engineClient.getJobMetricsSummary(jobId)
  -> finally block prints the summary table
  -> "Total Failed Count" now uses Math.max(0, sourceReadCount - sinkWriteCount)

Pulsar E2E path
  -> CanalToPulsarIT.startUp()
  -> waitForTopicCreated()
  -> inventoryDatabase.createAndInitialize()
  -> waitForCanalDataInTopic()
      -> PulsarClient.newReader(...).readNext(5s)
  -> only after a real message is present does the test continue to the PostgreSQL assertions

Key findings

  • The ClientExecuteCommand change is on the display path only (seatunnel-core/seatunnel-starter/src/main/java/org/apache/seatunnel/core/starter/seatunnel/command/ClientExecuteCommand.java:276-294). It does not change job control flow, checkpointing, or result semantics.
  • The E2E change is on the real asynchronous path that was flaky before (seatunnel-e2e/seatunnel-connector-v2-e2e/connector-pulsar-e2e/src/test/java/org/apache/seatunnel/e2e/connector/pulsar/CanalToPulsarIT.java:259-314). The test now waits for the actual Pulsar topic to contain data instead of assuming 10 seconds is always enough.
  • On the test-stability side, this looks good to me: the new wait uses an explicit condition (Reader.readNext(...)) plus Awaitility, and the Pulsar client / reader are properly closed with try-with-resources.

Merge conclusion

  • Conclusion: can merge from a source-level perspective.
  • Blocking items: none.
  • Non-blocking notes: the top-level Build check is still pending on my side while I am writing this, so I would still wait for it to finish green before merging.

Overall, this is a clean and focused split-out PR. The logic matches the real execution chain, and I did not find a new correctness or compatibility regression in the latest head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core SeaTunnel core module e2e reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants