Use @PreDestroy to ensure KuduClientWrapper is closed

[leveyja]

Description

Netty's ResourceLeakDetector logged a warning
that the resource was not being released via KuduClientWrapper.

Is this change a fix, improvement, new feature, refactoring, or other?

Fix/Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Connector (Kudu)

How would you describe this change to a non-technical end user or system administrator?

Ensure closable resources are closed during graceful shutdown

Related issues, pull requests, and links

Documentation

(*) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(*) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[hashhar]

Thanks.

[hashhar]

I don't suppose there's an easy way to test this? (Maybe we might just end up testing an airlift feature instead of our code which doesn't add value).

[leveyja]

@kokosing - I'm wrong about the LifeCycleManager not being required :-(
Because KuduClientSession is the Guice-injected instance, and not the underlying KuduClientWrapper (which holds the resources which must be closed), we do need to explicitly call LifeCycleManager.addInstance(Obj obj) to have the lifecycle managed.

Update: Resolved by moving @PreDestroy to KuduClientSession and invoking client.close() from the new lifecycle managed method.

[hashhar]

Some context of "why" this works (Guice and Bootstrap magic) in commit message will likely help any future developer trying to copy what you did here. (The comments in #12149 (comment) have the explanation I think).

[leveyja]

Some context of "why" this works (Guice and Bootstrap magic) in commit message will likely help any future developer trying to copy what you did here. (The comments in #12149 (comment) have the explanation I think).

I've updated the commit message:

    Use @PreDestroy to ensure KuduClientSession releases resources on closure

    Netty's ResourceLeakDetector<HashedWheelTimer> logged a warning
    that the timer was not being released via KuduClientWrapper.

    KuduClientSession now has a @PreDestroy annotated method to release
    the KuduClientWrapper. This is called by Airlift's LifeCycleManager
    due to the presence of the @PreDestroy/@PostContruct javax
    annotations.

Not 100% sure if it captures it or if it could be shorted?

[hashhar]

Thanks for tracking this down. I did verify that the KuduAsyncClient and KuduClientWrapper are being closed via debugger.

[hashhar]

Lgtm % checkstyle

[grantatspothero]

Thanks for doing this @leveyja

[kokosing]

Thanks!