-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HIVE-20682: Async query execution can potentially fail if shared sessionHive is closed by master thread. #452
Conversation
bc21f49
to
11d589d
Compare
this.get().close(); | ||
public synchronized void set(Hive hiveObj) { | ||
Hive currentHive = this.get(); | ||
if ((currentHive == null) || (currentHive != hiveObj)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this check ((currentHive == null) ) is not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep make sense
if (hiveDb != null && hiveDb != parentHive) { | ||
// If new hive object is created by the child thread, then we need to close it as it might | ||
// have created a hms connection. Call Hive.closeCurrent() that closes the HMS connection, causes | ||
// HMS connection leaks otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this check is required as we can not gurantee that parent hive object is session hive object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I create sessionHive, I set the allowClose flag to false. So, even if I try to invoke close on sessionHive via closeCurrent, it will be no-op and doesn't close it. So, this check is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure if we can safely assume that BackgroundWork is always called with session hive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, it is always sessionHive and for any custom implementation is there, I will add a comment in BackgroundWork for parentHive to use it correctly. Also, shall add an assert to verify it before setting it in async thread.
b8d08f1
to
af6e32b
Compare
No description provided.