-
Notifications
You must be signed in to change notification settings - Fork 80
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
[Feature] New Web UI #116
[Feature] New Web UI #116
Conversation
Wow .. super impressive. Given the large size of the PR we will have to discuss this as a team and figure in detail how we proceed. Couple of initial questions
There is a lot more to discuss in review and in general. Please be patient over the holidays .. maybe we can arrange a meeting in early January for a core group of project maintainers as next step. |
this is super awesome!! |
|
What do you mean launched independently? you mean that you can launch gateway server withouth web ui? |
This is a front-end and back-end separated architecture, where the pages can be deployed separately on servers like nginx. Just forward the front-end requests to '/webapp/', which would be relatively more secure, but I suppose not many people would choose to do so, you can just ignore it. Alternatively, you can choose to package the front-end static resources into 'gateway' and access them directly through the 'gateway'. This is the default and recommended approach. |
Please rebase, squash commits, and let us know when you are ready for us to review/test. So far this is looking very nice! |
I have completed squashing commits. I am ready for review and testing. Thank you for the feedback! |
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.
My comments are limited to the Java portions of this PR. Looks great, just a few items to address.
gateway-ha/src/main/java/io/trino/gateway/ha/config/ProxyBackendConfiguration.java
Outdated
Show resolved
Hide resolved
{ | ||
try { | ||
connectionManager.open(); | ||
String sql = "select * from query_history where 1=1"; |
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.
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.
Feel free to ignore #145. It's just a maintanance PR which should not bock other PRs.
gateway-ha/src/main/java/io/trino/gateway/ha/security/LbFormAuthManager.java
Outdated
Show resolved
Hide resolved
*/ | ||
public class GlobalPropertyRequest | ||
{ | ||
private String useSchema; |
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.
What is the useSchema
field used for?
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.
Please refer to the concept of "useScheme" in the following link: https://github.com/trinodb/trino-gateway/blob/main/docs/resource-groups-api.md.
LocalCacheClean: "Clear All Cache", | ||
}, | ||
Dashboard: { | ||
QPH: "QPH", |
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.
QPH = Query Per Hour? Then is it average too?
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.
It is not an average value because the configuration could be "queryHistoryHoursRetention:1", so there is no design to display average QPH.
gateway-ha/src/main/java/io/trino/gateway/ha/domain/TableData.java
Outdated
Show resolved
Hide resolved
Just a heads up that there should be a new column to display the health status of the cluster. See #80. |
Nice contribution! |
Any idea why I keep getting this error when I run this app locally? I'm using Update: This is the error trace:
|
Please fix |
gateway-ha/src/main/java/io/trino/gateway/ha/router/HaQueryHistoryManager.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/router/HaQueryHistoryManager.java
Outdated
Show resolved
Hide resolved
@@ -45,6 +45,7 @@ authentication: | |||
authorizationEndpoint: | |||
jwkEndpoint: | |||
redirectUrl: | |||
redirectWebUrl: |
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.
where/how is this property used?
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.
If the web UI is started independently, you can specify the redirect path.
By default, this configuration does not need to be specified.
children: React.ReactNode; | ||
} | ||
|
||
export class ErrorBoundary extends React.Component<IErrorBoundaryProps, IErrorBoundaryState> { |
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 change this to functional component to stay consistent with the rest of the components
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.
Because function components don't provide 'componentDidCatch', so here I am using class components directly without introducing any third-party libraries to implement it.
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.
For those who are testing, please note that
- UI requires AUTHORIZATION, so
docs/quickstart-config.yaml
doesnt' work properly unless presetUsers and authentication is set- Can this be optional?
I've got some suggestions for improvements in UI
-
In login screen, can you fix so that enter works to login? kinda bothersome to click on sign in button everytime
-
In history tab, how about adding
<all>
or sth placeholder forRoutedTo
andQueryId
which means all (just for the ui part like if none is selected, it means all in history)
-
add hyperlink at
Backends
to link cluster in dashboard for convenience?
-
add a button next to QPH/Avg. QPM/Avg. QPS to explain how each value is calculated?
+1 for making the authentication making optional. |
How about using the same approach as in the Trino UI .. username is required but can be random and you get full access. |
Fyi @ytwp ... in our dev sync earlier today we discussed the PR. We decided that we want to go ahead towards merge. A next step would be for you to rebase so its ready and then we can review. We will just get it to a good enough state for merge and plan on iterating on it as necessary together as well. The one thing we need to make sure is that we remove the requirement for authentication similar to how it is optional in Trino. Ping us on slack if you want to chat or let us know here. |
80a9ed4
to
55cea9e
Compare
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 👍
gateway-ha/src/main/java/io/trino/gateway/ha/module/HaGatewayProviderModule.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/util/PageUtil.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/util/PageUtil.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/util/PageUtil.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/util/PageUtil.java
Outdated
Show resolved
Hide resolved
Please update the commit title as "Modernize Web UI" or something. Also, please add the commit body to describe the changes. |
Developed a brand-new Web UI based on Semi UI, Vite, React, TypeScript, and E-charts framework to replace the old one.
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 ran a bunch of tests with this and looked over the whole codebase changes. This is a very significant improvement from the legacy UI.
It creates a base for further improvements that we can work on in follow up PRs where necessary, but the current state is already good for merge.
We will do further testing after merge, also with other features merged, and in the build up to the next release.
I will update the commit message as part of the merge. |
New Web UI
Screenshot