Bind UI related resources on a secondary port. #64
Conversation
Parquet only calls converts for which it found the values. The missing values are not reported. The BlockBuilder must be appended with nulls for the missing values based on fieldIndex of the currently read value by Parquet.
Ignore errors if the partitions have no corresponding valid hdfs path.
…Columns Append nulls for missing values in Parquet.
…d be reverted once prestodb#2863 is resolved.
…e-types Case insensitive types
This reverts commit 7365f1a. This blocked us from reading from mountable clusters.
…etastore Zookeeper based metastore
# Conflicts: # presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java
…user Run queries as user
…-user Pick username from unix system and disallow overriding it.
…seVersion Update release version to 0.157-tw-0.29
Upgrade to 0.161
…alidate invoke isReachable to test connection
| return enabledUIonSecondaryPort; | ||
| } | ||
|
|
||
| @Config("http-server.ui.secondary.port.enabled") |
There was a problem hiding this comment.
Can we deduce this property instead of reading it from config? If the ui.http.port is configured then this must be true, otherwise false.
There was a problem hiding this comment.
I think it would be better just have that property since the default of ui.http.port will be 0 which allows OS to assign a random port to the UI's HttpServer and we cannot deduce if the ui.http.port equals to 0 means it doesn't want to enable the UI on a secondary port.
There was a problem hiding this comment.
+1 to @dabaitu's comment. A user shouldn't have to set a port and also set a flag that they're setting a port.
There was a problem hiding this comment.
I would like to do but I don't think it would be easy. I have to go through all the way to understand how to distinguish the property from the config file and the property from default value because I don't think set a wired value for default just to deduce how the property come from is a good one. Giving this property is just same as Facebook guys did. See here https://github.com/airlift/airlift/blob/master/http-server/src/main/java/io/airlift/http/server/HttpServerConfig.java#L94
There was a problem hiding this comment.
It's not clear that the example you show is achieving the same thing. It seems it could be disabling whether the HTTP port is disabled or not. In our case, what's the expected behavior when the UI port if different from the http port but enabledUIonSecondaryPort is false?
There was a problem hiding this comment.
It would just not start the UI on the secondary port.
| new CoordinatorUIHttpServerModule(injector)); | ||
| Injector uIInjector = uIApp.doNotInitializeLogging().initialize(); | ||
|
|
||
| log.info("======== SECONDARY UI STARTED ========"); |
There was a problem hiding this comment.
When the server is started, the logging should show "======== SERVER STARTED ========". No need for "======== SECONDARY UI STARTED ========" IMO. I think it would be best to not introduce a new "secondary UI concept" and just have the concept of a UI port and a protocol (or something like that) port.
There was a problem hiding this comment.
So basically, we cannot change the current "http-server.http.port" to a new property name like "http-server.protocal.http.port" since it is defined in airlift's HttpServerModule. Based on this restriction I would prefer the secondary UI concept instead of the concept of protocol port plus UI port.
There was a problem hiding this comment.
But it's, of course, possible to disable the UI in main HttpServer Module if you prefer to do that.
There was a problem hiding this comment.
No need to change the existing port naming, but in the logging and configs, etc can we just refer to this as the the UI and the UI port, without the secondary part?
There was a problem hiding this comment.
Then do we need to disable the UI on the main HttpServer?
There was a problem hiding this comment.
I see what's happening now. You seem to be restarting a bunch of things twice on another port. Instead could break out the UI into its own configurable module so we could just include it in the list of Modules above and have it start as they do? Or does that not work because all the modules in the Bootstrap require a single http port?
There was a problem hiding this comment.
We cannot include the UI module in the main Bootstrap since the HttpServerModule will bind the HttpServerInfo(which injects the HttpServerConfig from the singleton, and the HttpServerConfig can only have one HTTP port) into the singleton and HttpServerProvider takes that instance. And all these steps happened in the airlift repo. What I did in the UI module is rebinding the initialized components in the main injector into the Bootstrap for UI. The hacky way here to make the secondary port works is fetching the initialized HttpServerConfig, setting the http port from the ServerConfig and building a new HttpServerInfo and a new HttpServerProvider.
There was a problem hiding this comment.
In short, all the modules in the Bootstrap require a single http port.
|
We should submit this review upstream to presto master for OSS comments before merging onto our fork. |
|
Submitted PR prestodb#7106 in OSS repo |
Bind UI related resources on a secondary port. It basically injects the instances from the main presto server into a secondary Http server on a secondary port. Besides the UI, it also binds the ServerInfoResource in order to make the monitor can fetch the status from the secondary port.