-
Notifications
You must be signed in to change notification settings - Fork 8
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
Upgrade Hive from 2 to 3 #50
Conversation
|
||
conf.setBoolVar(HiveConf.ConfVars.HIVESTATSAUTOGATHER, false); | ||
|
||
// Disable to get rid of clean up exception when stopping the Session. | ||
conf.setBoolVar(HiveConf.ConfVars.HIVE_SERVER2_LOGGING_OPERATION_ENABLED, false); | ||
|
||
// Used to prevent "Not authorized to make the get_current_notificationEventId call" errors |
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.
Nice one for adding the comments explaining why, future code archaeologists will thank you.
|
||
private int getWebUIPort() { | ||
// Try to find a free port, if impossible return the default port 0 which disables the WebUI altogether | ||
int DEFAULT_PORT = 0; |
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.
int DEFAULT_PORT = 0; | |
int defaultPort = 0; |
(since it's no longer static and thus not a "constant")
try (ServerSocket socket = new ServerSocket(0)) { | ||
return socket.getLocalPort(); | ||
} catch (IOException e) { | ||
return DEFAULT_PORT; |
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.
Maybe log the exception on "info" as this shouldn't really happen and indicates some kind of (recoverable) error?
conf.setVar(HiveConf.ConfVars.METASTORE_CONNECTION_USER_NAME, METASTORE_DB_USER); | ||
conf.setVar(HiveConf.ConfVars.METASTOREPWD, METASTORE_DB_PASSWORD); | ||
|
||
// conf.setVar(HiveConf.ConfVars.METASTORECONNECTURLKEY, connectionURL); |
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.
You can remove the commented out lines as they're dealt with below right?
return socket.getLocalPort(); | ||
} catch (IOException e) { | ||
log.info( | ||
"No free port available for the Web UI. Setting the port to " + defaultPort + ", which disables the WebUI."); |
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 we should also log e
here as there are a few different reasons the ServerSocket
constructor can throw an IOE.
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.
Makes sense. Added.
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.
Code looks good. I think we should add a section to the README which explains the Hive version support. You can probably just copy the header and first two bullet points from https://github.com/klarna/HiveRunner#hive-version-compatibility and then add this section inbetween "Maven Dependencies" and "JUnit5".
Something along these lines? |
No description provided.