Fix thrift client connection for Kerberos Hive Client

[kevinjqliu]

Closes #1744

TSaslClientTransport cannot be reopen. This PR changes the behavior to recreate a TSaslClientTransport when its already closed.

Note, _HiveClient should be used with context manager, but can be used without.

[hussein-awala]

IMHO, it would be better to define the client as a cached_property, and just return it.
+1 for moving the init out of the __init__ func, and converting the client to a cached_property will help to do this, where the user will be able to use the client with and without a context manager.

[Fokko]

@kevinjqliu I think this one is also good for 0.9.1

[Fokko]

Gentle ping @kevinjqliu. Any thoughts on the cached_property suggested by @hussein-awala?

[mnzpk]

IMHO, it would be better to define the client as a cached_property, and just return it. +1 for moving the init out of the __init__ func, and converting the client to a cached_property will help to do this, where the user will be able to use the client with and without a context manager.

Hi @hussein-awala, just to make sure I have this right, would this mean moving most of the logic in _init_thrift_client to a cached property named client with __enter__ returning self.client and __exit__ closing the transport and deleting self.client?

Edit: I'm asking because there might be cases where the above could actually result in a user invoking methods on a client whose underlying transport has been closed. For example:

hive_client = _HiveClient(...)

p_client = hive_client.client

with hive_client as open_client:
    print(open_client.get_all_databases())

print(p_client.get_all_databases())  # Results in TTransportException: Transport not open

So its likely that this is not what you meant.

[kevinjqliu]

Thanks for chiming in @hussein-awala @mnzpk. Please take a look at the new implementation.

The context manager (__enter__ & __exit__) now manages the underlying _transport. The _transport will be closed on exit and re-initialized and opened on enter.

@mnzpk could you give this a try?

[kevinjqliu]

CI's currently failing for main branch, see https://github.com/apache/iceberg-python/pull/1899/files#r2040915222

make test-integration still works locally and ran successfully

[kevinjqliu]

cool CI passes now

@mnzpk please take a look when you have time :)

[Fokko]

Thanks for fixing this @kevinjqliu 🙌

[abhisheksinha-pty]

I was getting this exact error. Thanks for fixing this issue, but any idea how can I provide kerberos related details while creating catalog, like kerberos principal , kerberos_keytab, kerberos_service_name, kerberos_user ? I tried searching the documentation but could not find anything except this "hive.kerberos-authentication": "true".

[kevinjqliu]

@abhisheksinha-pty im not really sure how those params are passed into hive/kerberos. Here is where we create the hive client, i would assume you have to pass the params in there somehow. Do you know how its passed into a regular hive client?

[mnzpk]

Apologies for not reviewing this sooner but unfortunately, installing from main, I can still reproduce the reported issue. I've left a few comments with what I think would fix that. I've also added a few tests here that would allow testing this without having kerberos auth or a kerberized metastore instance set up. Not sure how useful you think those would be but let me know!

Thanks so much for all your work here.

[mnzpk]

It seems that we're still going to try re-opening the transport once it has been closed and since the exception raised in that case would be a TypeError, it would not be caught by the except below.

[mnzpk]

We're re-initializing the transport but since self._client is a cached_property, it'd still point to the old (and now closed) transport so I think we'd also want to delete self._client so that it gets re-created?