You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Stephan Erb <se...@apache.org> on 2017/11/08 22:32:39 UTC
Review Request 63685: RFC: Use new scheduler UI as landing page
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63685/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin and Joshua Cohen.
Repository: aurora
Description
-------
Is this something the community would be interested in?
Known issues in this minimal viable prototype:
* I did not manage to convince Jetty to perform the redirecting. Using the HTML feels hacky but works.
* An operator will have difficulties to navigate to /admin of a non-leading instance. In particular if there is no leader at all.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a
src/main/resources/scheduler/assets/index.html 533091395547a6b067dbf5f53e42a13a560971da
ui/src/main/js/components/Navigation.js 50881a9914cee2812807624cd28f24fa64207296
ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93
ui/src/main/js/pages/Admin.js PRE-CREATION
Diff: https://reviews.apache.org/r/63685/diff/1/
Testing
-------
File Attachments
----------------
Screen Shot 2017-11-08 at 23.18.06.png
https://reviews.apache.org/media/uploaded/files/2017/11/08/80ae2c5f-e431-4f86-8811-13bfe6a8627b__Screen_Shot_2017-11-08_at_23.18.06.png
Screen Shot 2017-11-08 at 23.18.20.png
https://reviews.apache.org/media/uploaded/files/2017/11/08/9fb0f2e0-49c9-427f-8b84-d3d1fc057a59__Screen_Shot_2017-11-08_at_23.18.20.png
Thanks,
Stephan Erb
Re: Review Request 63685: RFC: Use new scheduler UI as landing page
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63685/#review199332
-----------------------------------------------------------
I won't be able to work in this in the near future. Closing.
- Stephan Erb
On Nov. 8, 2017, 11:32 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63685/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 11:32 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Joshua Cohen.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Is this something the community would be interested in?
>
> Known issues in this minimal viable prototype:
>
> * I did not manage to convince Jetty to perform the redirecting. Using the HTML feels hacky but works.
> * An operator will have difficulties to navigate to /admin of a non-leading instance. In particular if there is no leader at all.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a
> src/main/resources/scheduler/assets/index.html 533091395547a6b067dbf5f53e42a13a560971da
> ui/src/main/js/components/Navigation.js 50881a9914cee2812807624cd28f24fa64207296
> ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93
> ui/src/main/js/pages/Admin.js PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63685/diff/1/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Screen Shot 2017-11-08 at 23.18.06.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/80ae2c5f-e431-4f86-8811-13bfe6a8627b__Screen_Shot_2017-11-08_at_23.18.06.png
> Screen Shot 2017-11-08 at 23.18.20.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/9fb0f2e0-49c9-427f-8b84-d3d1fc057a59__Screen_Shot_2017-11-08_at_23.18.20.png
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 63685: RFC: Use new scheduler UI as landing page
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63685/#review190503
-----------------------------------------------------------
Master (5dfe51c) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 8, 2017, 10:32 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63685/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 10:32 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Joshua Cohen.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Is this something the community would be interested in?
>
> Known issues in this minimal viable prototype:
>
> * I did not manage to convince Jetty to perform the redirecting. Using the HTML feels hacky but works.
> * An operator will have difficulties to navigate to /admin of a non-leading instance. In particular if there is no leader at all.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a
> src/main/resources/scheduler/assets/index.html 533091395547a6b067dbf5f53e42a13a560971da
> ui/src/main/js/components/Navigation.js 50881a9914cee2812807624cd28f24fa64207296
> ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93
> ui/src/main/js/pages/Admin.js PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63685/diff/1/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Screen Shot 2017-11-08 at 23.18.06.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/80ae2c5f-e431-4f86-8811-13bfe6a8627b__Screen_Shot_2017-11-08_at_23.18.06.png
> Screen Shot 2017-11-08 at 23.18.20.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/9fb0f2e0-49c9-427f-8b84-d3d1fc057a59__Screen_Shot_2017-11-08_at_23.18.20.png
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 63685: RFC: Use new scheduler UI as landing page
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63685/#review190538
-----------------------------------------------------------
I agree with the direction, and i agree that we should try for a redirect-free experience. Thought added below.
src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java
Line 334 (original), 334 (patched)
<https://reviews.apache.org/r/63685/#comment268005>
I may be misunderstanding the goal, but i would have expected this line to change to:
```
.put("/(?:index.html)?", "/assets/scheduler/index.html")
```
This should serve the scheduler UI when accessing `/` or `/index.html`.
- Bill Farner
On Nov. 8, 2017, 2:32 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63685/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 2:32 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Joshua Cohen.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Is this something the community would be interested in?
>
> Known issues in this minimal viable prototype:
>
> * I did not manage to convince Jetty to perform the redirecting. Using the HTML feels hacky but works.
> * An operator will have difficulties to navigate to /admin of a non-leading instance. In particular if there is no leader at all.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a
> src/main/resources/scheduler/assets/index.html 533091395547a6b067dbf5f53e42a13a560971da
> ui/src/main/js/components/Navigation.js 50881a9914cee2812807624cd28f24fa64207296
> ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93
> ui/src/main/js/pages/Admin.js PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63685/diff/1/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Screen Shot 2017-11-08 at 23.18.06.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/80ae2c5f-e431-4f86-8811-13bfe6a8627b__Screen_Shot_2017-11-08_at_23.18.06.png
> Screen Shot 2017-11-08 at 23.18.20.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/9fb0f2e0-49c9-427f-8b84-d3d1fc057a59__Screen_Shot_2017-11-08_at_23.18.20.png
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 63685: RFC: Use new scheduler UI as landing page
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63685/#review190533
-----------------------------------------------------------
In general I think the spirit of this change makes sense, but as you mention, the HTTP redirect is definitely not ideal. It should certainly be possible to serve up the actual scheduler UI by default without the need to redirect. Off the top of my head I'm not sure exactly what would need to be done though.
- Joshua Cohen
On Nov. 8, 2017, 10:32 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63685/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 10:32 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Joshua Cohen.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Is this something the community would be interested in?
>
> Known issues in this minimal viable prototype:
>
> * I did not manage to convince Jetty to perform the redirecting. Using the HTML feels hacky but works.
> * An operator will have difficulties to navigate to /admin of a non-leading instance. In particular if there is no leader at all.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a
> src/main/resources/scheduler/assets/index.html 533091395547a6b067dbf5f53e42a13a560971da
> ui/src/main/js/components/Navigation.js 50881a9914cee2812807624cd28f24fa64207296
> ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93
> ui/src/main/js/pages/Admin.js PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63685/diff/1/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Screen Shot 2017-11-08 at 23.18.06.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/80ae2c5f-e431-4f86-8811-13bfe6a8627b__Screen_Shot_2017-11-08_at_23.18.06.png
> Screen Shot 2017-11-08 at 23.18.20.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/9fb0f2e0-49c9-427f-8b84-d3d1fc057a59__Screen_Shot_2017-11-08_at_23.18.20.png
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 63685: RFC: Use new scheduler UI as landing page
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63685/#review190631
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java
Line 337 (original), 337 (patched)
<https://reviews.apache.org/r/63685/#comment268131>
We shouldn't have to define "UI" routes that are part of the React app in the Scheduler. We should explicitly route to /api and the remaining static assets left (Thrift stuff, jQuery for Thrift transport layer) and then route everything else to /assets/scheduler/index.html and let React Router decide what to do.
ui/src/main/js/components/Navigation.js
Lines 15 (patched)
<https://reviews.apache.org/r/63685/#comment268133>
I'm a little wary of putting this in the navigation for all users to see. Some of the endpoints are quite expensive and not part of public APIs.
ui/src/main/js/index.js
Lines 38 (patched)
<https://reviews.apache.org/r/63685/#comment268132>
The Admin does not need the API.
- David McLaughlin
On Nov. 8, 2017, 10:32 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63685/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2017, 10:32 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Joshua Cohen.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Is this something the community would be interested in?
>
> Known issues in this minimal viable prototype:
>
> * I did not manage to convince Jetty to perform the redirecting. Using the HTML feels hacky but works.
> * An operator will have difficulties to navigate to /admin of a non-leading instance. In particular if there is no leader at all.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a
> src/main/resources/scheduler/assets/index.html 533091395547a6b067dbf5f53e42a13a560971da
> ui/src/main/js/components/Navigation.js 50881a9914cee2812807624cd28f24fa64207296
> ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93
> ui/src/main/js/pages/Admin.js PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63685/diff/1/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Screen Shot 2017-11-08 at 23.18.06.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/80ae2c5f-e431-4f86-8811-13bfe6a8627b__Screen_Shot_2017-11-08_at_23.18.06.png
> Screen Shot 2017-11-08 at 23.18.20.png
> https://reviews.apache.org/media/uploaded/files/2017/11/08/9fb0f2e0-49c9-427f-8b84-d3d1fc057a59__Screen_Shot_2017-11-08_at_23.18.20.png
>
>
> Thanks,
>
> Stephan Erb
>
>