-
Notifications
You must be signed in to change notification settings - Fork 916
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
[KYUUBI #6615] Make Jetty sending server version in response configurable #6685
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6685 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 684 684
Lines 42237 42247 +10
Branches 5755 5755
======================================
- Misses 42237 42247 +10 ☔ View full report in Codecov by Sentry. |
@volatile protected var isStarted = false | ||
|
||
override def initialize(conf: KyuubiConf): Unit = { | ||
val port = conf.get(MetricsConf.METRICS_PROMETHEUS_PORT) | ||
val contextPath = conf.get(MetricsConf.METRICS_PROMETHEUS_PATH) | ||
httpServer = new Server(port) | ||
val sendServerVersion = conf.get(FRONTEND_JETTY_SEND_VERSION_ENABLED) |
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.
val sendServerVersion = conf.get(FRONTEND_JETTY_SEND_VERSION_ENABLED) | |
val jettyVersionEnabled = conf.get(FRONTEND_JETTY_SEND_VERSION_ENABLED) |
@@ -35,12 +36,21 @@ class PrometheusReporterService(registry: MetricRegistry) | |||
|
|||
// VisibleForTesting | |||
private[metrics] var httpServer: Server = _ | |||
private[metrics] var connector: ServerConnector = _ |
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.
private[metrics] var connector: ServerConnector = _ | |
private[metrics] var httpServerConnector: ServerConnector = _ |
To follow the naming conversion with existed httpServer
variable.
httpServer.stop() | ||
connector.stop() |
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.
Whether we should stop the server connector before the stopping the server?
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.
Hi @bowenliang123 ,
Thanks for the review.
I refered to the codes in JettyServer, in which the stop method closes the server first and then the server conenctor. I think it should work as well.
14880fe
to
0638a51
Compare
🔍 Description
Issue References 🔗
This pull request fixes #6615
Describe Your Solution 🔧
Add a config item that controls whether Jetty should send its version in response.
This is an additional patch which enables/disables sending Jetty version for prometheus reporter.
Sending Jetty version could be disabled by calling HttpConfiguration::setSendServerVersion(false)
Types of changes 🔖
Test Plan 🧪
Compiled and tested manually.
Behavior Without This Pull Request ⚰️
Behavior With This Pull Request 🎉
Related Unit Tests
Checklist 📝
Be nice. Be informative.