Fix JDBC connection leak in HiveIncrementalPuller.saveDelta()

[mailtoboggavarapu-coder]

Describe the issue this Pull Request addresses

Describe the issue this Pull Request addresses

In HiveIncrementalPuller.saveDelta(), a JDBC Connection object obtained via getConnection() was never closed. The variable was declared inside the try block, making it inaccessible in the finally block where only the Statement was being closed.

This causes a JDBC connection leak every time saveDelta() is called.

Changes

• Hoisted Connection conn declaration to before the try block (initialized to null) so it is accessible in finally
• Changed Connection conn = getConnection() inside the try to conn = getConnection()
• Added a dedicated try/catch block in finally to close conn after closing stmt, consistent with the existing pattern for closing the Statement

Summary and Changelog

Impact

Risk Level

Documentation Update

Contributor's checklist

• Read through contributor's guide
• Enough context is provided in the sections above
• Adequate tests were added if applicable

[yihua]

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Accidentally approved

[yihua]

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for contributing! The intent here is correct and the leak is real, but there's a syntax error introduced in the diff that's breaking the build, and a subtle design concern with the singleton connection worth addressing before merge.

[yihua]

🤖 The diff removes the opening { from if (stmt != null) { but leaves the original closing } in place. That orphaned } now closes the try block prematurely, and the } catch immediately after it has nothing to attach to — this is a compile error, and is almost certainly why the Azure CI build is failing. The original braced form if (stmt != null) { stmt.close(); } should be kept as-is (or converted to try-with-resources).

[yihua]

🤖 Since getConnection() is a lazy singleton that stores the connection in this.connection, calling conn.close() here closes this.connection — but this.connection is never set back to null. The null-check in getConnection() won't detect a closed-but-non-null connection, so any subsequent call to getConnection() (e.g. if saveDelta() or getTableLocation() were invoked again on the same instance) would return the already-closed connection and immediately fail. Could you add this.connection = null; after conn.close() to guard against that?

[mailtoboggavarapu-coder]

Good catch! Added this.connection = null; after conn.close() so the lazy-singleton guard in getConnection() will correctly detect the closed state and re-open on the next call. Fixed in the latest commit.

[yihua]

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for addressing the feedback! Both prior concerns have been resolved — the syntax error is fixed and the stale connection reference is now nulled out. There's one small edge case remaining where this.connection may not get cleared if conn.close() itself throws; see the inline note.

[yihua]

🤖 If conn.close() throws a SQLException, execution jumps straight to the catch block and this.connection = null never runs — leaving the field pointing at a closed (or partially-closed) connection for any future caller. Moving the assignment to just before conn.close() (or wrapping those two lines in a tiny try/finally) would make the reset unconditional.

_{- Generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.}

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.97%. Comparing base (76b4fa4) to head (cb4b921).
⚠️ Report is 22 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (76b4fa4) and HEAD (cb4b921). Click for more details.

HEAD has 27 uploads less than BASE

Flag	BASE (76b4fa4)	HEAD (cb4b921)
spark-scala-tests	10	0
spark-java-tests	15	0
common-and-other-modules	1	0
utilities	1	0

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #18460       +/-   ##
=============================================
- Coverage     68.53%   53.97%   -14.57%     
+ Complexity    28037    12282    -15755     
=============================================
  Files          2444     1425     -1019     
  Lines        134752    70968    -63784     
  Branches      16292     8004     -8288     
=============================================
- Hits          92356    38306    -54050     
+ Misses        35112    29240     -5872     
+ Partials       7284     3422     -3862     

Flag	Coverage Δ
common-and-other-modules	?
hadoop-mr-java-client	44.87% <ø> (-0.06%)	⬇️
spark-client-hadoop-common	48.47% <ø> (+0.01%)	⬆️
spark-java-tests	?
spark-scala-tests	?
utilities	?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1802 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hudi-bot]

CI report:

• cb4b921 Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

• @hudi-bot run azure re-run the last Azure build

[mailtoboggavarapu-coder]

Thanks @yihua for the thorough review! All three issues have been addressed:

1. Syntax fix (if (stmt != null) braces): Restored the full braced form if (stmt != null) { stmt.close(); } so the closing } does not prematurely end the try block.

2. Null out stale connection reference: Added this.connection = null; after conn.close() so the lazy-singleton getConnection() detects the closed connection and re-opens it on the next call.

3. finally block for unconditional null-out (latest commit cb4b921): Moved this.connection = null; into a finally block so it always executes even if conn.close() throws a SQLException. The close section now reads:

try {
  if (conn != null) {
    conn.close();
  }
} catch (SQLException e) {
  LOG.error("Could not close the JDBC connection", e);
} finally {
  this.connection = null;
}

Could you please re-review and approve when you get a chance? Thanks!

[yihua]

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Nice fix — moving the null assignment into a finally block is the right approach and correctly resolves the prior concern. this.connection = null now runs unconditionally whether conn.close() succeeds, throws, or conn was null in the first place, so any subsequent getConnection() call will always obtain a fresh connection. All prior findings are addressed; no new issues introduced.

[mailtoboggavarapu-coder]

Thanks @yihua for the detailed follow-up review! To address the inline note about conn.close() throwing: commit cb4b921 wraps the close in a dedicated try/catch and moves this.connection = null into the finally block, so it runs unconditionally regardless of whether conn.close() succeeds or throws. The code now reads:

try {
    if (conn != null) {
        conn.close();
    }
} catch (SQLException e) {
    LOG.error("Could not close the JDBC connection", e);
} finally {
    this.connection = null;
}

As confirmed in your latest review, all prior findings are now addressed. Could a committer with write access please take a look and approve/merge when ready? The branch also needs to be synced with master (Update with Merge Commit) to clear the stale CI results. Thanks!