aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David McLaughlin <da...@dmclaughlin.com>
Subject Re: Review Request 62763: Create Update Page with React
Date Fri, 06 Oct 2017 19:52:01 GMT


> On Oct. 6, 2017, 7:46 p.m., Santhosh Kumar Shanmugham wrote:
> > Looks good to me overall.
> > 
> > I see the pattern that we are passing in the entire `update` object to all the components,
when they only consume a small part of it. Any reason for keeping it this way.

This is mostly related to the plugin architecture. I'd rather be liberal so that if people
want to drop in customizations, they have access to as much data as possible in the components
they swap out. 

But in general I also prefer to not destructure the objects all over the client - it makes
it harder to change later if the API changes.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62763/#review187281
-----------------------------------------------------------


On Oct. 5, 2017, 10:44 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62763/
> -----------------------------------------------------------
> 
> (Updated Oct. 5, 2017, 10:44 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Implement update page for new UI. 
> 
> The most significant change here is the performance - for our jobs at Twitter with thousands
of instances, this shaves tens of seconds off the render time compared to the Angular version
(which I think is attributed to the tooltip code). 
> 
> I translated the "instance events" into something useful - now grouping them all events
for an instance together in a single item and showing history with the state machine component.

> 
> For the instance visualization, I also simplified the color scheme. There is now only:
success, error, warning, in progress and removed. So no more roll backs being represented
as green boxes with red borders (roll backs are now considered as the warning/attention color).

> 
> I didn't do much for configuration and kept the UX the same as the existing UI, I will
tackle showing diffs in the future. 
> 
> This is branched off of https://reviews.apache.org/r/62720
> 
> 
> Diffs
> -----
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/client/scheduler-client.js 1c381086934dafa22a62d18e035f599fb2260e15

>   ui/src/main/js/components/InstanceViz.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/Pagination.js 7bf2c04e2b879657c6ec43d261f2512fae79a08e 
>   ui/src/main/js/components/TaskConfig.js PRE-CREATION 
>   ui/src/main/js/components/Time.js PRE-CREATION 
>   ui/src/main/js/components/UpdateConfig.js PRE-CREATION 
>   ui/src/main/js/components/UpdateDetails.js PRE-CREATION 
>   ui/src/main/js/components/UpdateInstanceEvents.js PRE-CREATION 
>   ui/src/main/js/components/UpdateInstanceSummary.js PRE-CREATION 
>   ui/src/main/js/components/UpdateList.js PRE-CREATION 
>   ui/src/main/js/components/UpdateSettings.js PRE-CREATION 
>   ui/src/main/js/components/UpdateStateMachine.js PRE-CREATION 
>   ui/src/main/js/components/UpdateStatus.js PRE-CREATION 
>   ui/src/main/js/components/UpdateTime.js PRE-CREATION 
>   ui/src/main/js/components/UpdateTitle.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceViz-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/Pagination-test.js f2b72e93eabe9660f3ca54d3e151ce154fa81e0a

>   ui/src/main/js/components/__tests__/UpdateInstanceEvents-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/UpdateList-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/UpdateStatus-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Update.js PRE-CREATION 
>   ui/src/main/js/pages/Updates.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Update-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Updates-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/UpdateBuilders.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/js/utils/Update.js PRE-CREATION 
>   ui/src/main/js/utils/__tests__/Update-test.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-viz.scss PRE-CREATION 
>   ui/src/main/sass/components/_layout.scss 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_update-list.scss PRE-CREATION 
>   ui/src/main/sass/components/_update-page.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62763/diff/2/
> 
> 
> Testing
> -------
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Tested against live APIs. Note: I am pushing small nitpick bugs to the very end of this
work (where we will leverage internal beta-testing).
> 
> 
> File Attachments
> ----------------
> 
> Successful update
>   https://reviews.apache.org/media/uploaded/files/2017/10/05/457b8fe0-dfd4-4e30-bbd3-e5c76906fab2__Screen_Shot_2017-10-04_at_5.22.08_PM.png
> Update In-Progress
>   https://reviews.apache.org/media/uploaded/files/2017/10/05/d04afb4c-7110-4fe2-a0d9-c191b935a555__Screen_Shot_2017-10-04_at_5.25.40_PM.png
> Huge update
>   https://reviews.apache.org/media/uploaded/files/2017/10/05/5b2534e3-e5d6-48e8-9615-2b1f7bb13617__Screen_Shot_2017-10-04_at_5.27.15_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message