[FLINK-30371] Fix the problem of JdbcOutputFormat database connection leak

[KevDi]

I have redone the Fix from #5 because no more action is going on there.
I updated to the latest version of the main branch and also added the needed tests.
Hope this is fine so far.

[boring-cyborg]

Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)

[KevDi]

Any news on that?

[eskabetxe]

Hi @KevDi ,
Checks are not passing, please fix the commit message.
Thanks

[KevDi]

Hi @eskabetxe is there a format that should be used for the Commits?

[eskabetxe]

Should be on same format that you put on title of PT
[FLINK-XXXXX] Some text

And merge commits should be avoided..

Current you have 3 commit messages, the ideal is 1

The commit in the middle is ok, the other 2 no

[KevDi]

Ok i try to compress the commits to one single commit :)

[KevDi]

@eskabetxe had to experience a little bit but now it seems that only one commit is there =)

[eskabetxe]

@MartijnVisser could you approve the workflow please

[KevDi]

@eskabetxe @MartijnVisser i looked into this error but i was unable to resolve it. I removed the mentioned import with explicit imports but then i get an error telling me that some of the static imports are imported before others. If i used the Sort Imports´ on Intellij it removed the imports with the .*` version. Maybe i need some support with that.

[MartijnVisser]

@KevDi You'll need to rebase, the Flink project doesn't do merge commits.

[KevDi]

@MartijnVisser i have done the rebase.

[KevDi]

@MartijnVisser any updates on this?

[fapaul]

@EchoLee5 c @KevDi sorry for the long wait. Is anyone still willing to contribute this change? I can help with reviewing/merging this PR.

[lvyanquan]

Hi, @KevDi.
Would you like to rebase and fix the conflict?

[KevDi]

@fapaul i can try to take a look at it. Question would be if it is still enough to do the fix like i provided it here or if the solution from @tamirsagi here #5 (comment) would also work. I'm not sure about it because the error seems to come when the flush get an exception. It is then thrown upwords without a chance to close the connection.

[fapaul]

@fapaul i can try to take a look at it. Question would be if it is still enough to do the fix like i provided it here or if the solution from @tamirsagi here #5 (comment) would also work. I'm not sure about it because the error seems to come when the flush get an exception. It is then thrown upwords without a chance to close the connection.

I prefer the solution via the autoclosable block because your current fix changes the semantics slightly. In case the flushException was previously set by the background executor we now overwrite it with this PR.

In general, can you maybe share a full stack trace or how you are using the JdbcOutputFormat? I am mainly interested in understanding which JdbcSink, in your case, calls the JdbcOutputFormat to make sure we do not miss a case.

[KevDi]

@fapaul Ok i check if i found the old code. In general the question that i have is what happens if the flush call in close is throwing an exception. This will then been thrown upwards and from what i understand is that the connections are not closed at this point or am i wrong here?

[fapaul]

You are correct that in case an error is thrown during calling close we might not clean up the database connection. That mostly affects jobs that have a restart strategy or jobs running on a session cluster e.g. the JVM is never shutdown.

To fully confirm your scenario that, it would be helpful to understand which JdbcSink you are using since the JdbcOutputFormat is usually not used directly by users.

[KevDi]

@fapaul would it make sense to change both try-catch Blocks to try (connectionProvider) ? SO the one wrapping around the flush and the one wrapping around closeStatement call?

[fapaul]

SO the one wrapping around the flush

I don't think that is necessary. The flush is either retried if configured or the exception fails the job, and eventually, closeStatement is invoked.

With the current architecture of different sink implementations all depending on the JdbcOutputFormat, I'd avoid any chances that change the behavior even more.

[KevDi]

Ok but how should the connection leak beeing fixed then?

[fapaul]

Sorry my previous message wasn't well-formatted (fixed now). The try-catch block around the closeStatement is ok, but I'd not touch the flush method.