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
Enable REST frontend protocol by default #4483
Conversation
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.
LGTM. Great to see REST protocol enabled by default. Other features relying to REST like kyuubi-admin
also benefit from this.
startBatchChecker() | ||
startInternal() | ||
} catch { | ||
case e: Exception => throw new KyuubiException(s"Cannot start $getName", e) | ||
} | ||
} | ||
super.start() | ||
info(s"Exposing REST endpoint at: ${server.getServerUri}") |
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.
Make output consistent
KyuubiTBinaryFrontendService: Service[KyuubiTBinaryFrontend] is started.
KyuubiTBinaryFrontendService: Starting and exposing JDBC connection at: jdbc:hive2://10.221.96.184:10009/
...
KyuubiRestFrontendService: Service[KyuubiRestFrontendService] is started.
KyuubiRestFrontendService: Exposing REST endpoint at: http://10.221.96.184:10099
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.
Possible to output the protocol as well?
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.
OK, currently it does not support SSL, so simply add http://
Thanks all, merged to master |
Why are the changes needed?
REST frontend protocol was introduced in Kyuubi 1.4.0 #1349, more and more users are using REST in recent days, and to simplify the planned Web UI developments, I think we should enable the REST by default for master(1.8.0-SNAPSHOT).
I do not remove the "experiment" because we don't have strong confident to ensure the API stability in this time, I would like to defer it until the community has confident to mark it as stable.
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request