Skip to content

[Improve][Connector-V2] Enhance JDBC chunk splitter logging#10950

Open
zhangshenghang wants to merge 1 commit into
apache:devfrom
zhangshenghang:zsh/sync-chunk-splitter-log
Open

[Improve][Connector-V2] Enhance JDBC chunk splitter logging#10950
zhangshenghang wants to merge 1 commit into
apache:devfrom
zhangshenghang:zsh/sync-chunk-splitter-log

Conversation

@zhangshenghang
Copy link
Copy Markdown
Member

Purpose of this pull request

This PR syncs the logging enhancement from zhangshenghang#11 to Apache SeaTunnel.

It adds INFO logs for JDBC chunk splitting to make generated prepared SQL and split parameters easier to inspect when troubleshooting dynamic, string, and numeric splitting.

Does this PR introduce any user-facing change?

Yes. It adds additional INFO-level logs for JDBC chunk splitting. Connector behavior is unchanged.

How was this patch tested?

Ran:

./mvnw spotless:apply

Build verification was not run per sync request.

Check list

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 the update. I reviewed the full diff and traced where these log statements land on the real JDBC split-generation path.

What this PR fixes

  • User pain: debugging JDBC split generation can be hard when users do not know which SQL / bounds were actually prepared.
  • Fix approach: promote SQL and split-bound logs from debug-level / sparse output to more visible INFO-level logging.
  • One-line summary: better observability is useful, but the current logging strategy is too expensive and too imprecise for the main split path.

Runtime chain I checked

split generation
  -> ChunkSplitter.createPreparedStatement(...) [ChunkSplitter.java:139-147]
  -> FixedChunkSplitter / DynamicChunkSplitter bind per-split parameters
  -> normal snapshot / full-scan jobs can generate many splits on large tables

Findings

Issue 1: the PR moves per-split SQL logging onto the normal INFO path

  • Location: ChunkSplitter.java:139-147; FixedChunkSplitter.java:341, 411-418; DynamicChunkSplitter.java:903-931
  • Problem: these logs run on the normal split-generation path and can emit once per split for large tables.
  • Risk: large-table snapshot jobs can flood INFO logs with SQL and bound values, which hurts signal-to-noise and can become operationally expensive.
  • Better fix: keep this at DEBUG, or gate INFO logging behind an explicit troubleshooting option.
  • Severity: High

Issue 2: the new parameter logs are incomplete for multi-key splits and can be misleading

  • Location: DynamicChunkSplitter.java:903-931
  • Problem: the new INFO messages only print the first bound value pattern (for example splitEnd[0] / splitStart[0]) even though the actual statements can bind multiple split-key values.
  • Risk: operators may trust logs that do not actually describe the full bound set, which is worse than having no log at all when debugging correctness.
  • Better fix: either log the full parameter vector safely, or do not log partial parameter snapshots as if they were authoritative.
  • Severity: Medium

Merge conclusion

Conclusion: can merge after fixes

  1. Blocking items
  • Issue 1: avoid unconditional INFO-level per-split logging on the normal path.
  1. Suggested non-blocking follow-up
  • Issue 2: if the team keeps parameter logging, make it complete for multi-key splits.

Overall, I support improving diagnosability here, but the current logging level and shape are not ready to merge yet.

@nzw921rx
Copy link
Copy Markdown
Collaborator

+1

@DanielLeens
Copy link
Copy Markdown
Contributor

Thanks for taking a look, @nzw921rx. On the unchanged head 9de9ae5, Daniel's review result is still the same: improving diagnosability makes sense, but issue 1 from the previous review still needs to be addressed before merge.

There is no new code on the PR head, so I am not starting another full review in this round. Happy to re-review if the author pushes an update.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants