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 Tue, 10 Oct 2017 16:59:59 GMT


> On Oct. 6, 2017, 7:46 p.m., Santhosh Kumar Shanmugham wrote:
> > ui/src/main/js/components/__tests__/InstanceViz-test.js
> > Lines 23 (patched)
> > <https://reviews.apache.org/r/62763/diff/1/?file=1846430#file1846430line23>
> >
> >     nit - assert `medium` and `big` do not appear?

Done.


> On Oct. 6, 2017, 7:46 p.m., Santhosh Kumar Shanmugham wrote:
> > ui/src/main/js/components/__tests__/Pagination-test.js
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/62763/diff/1/?file=1846431#file1846431line78>
> >
> >     We do not sort when `sortBy` is omitted. What do you mean by native sort?

The natural order of the collection (i.e. order by array index). Updated description.


> On Oct. 6, 2017, 7:46 p.m., Santhosh Kumar Shanmugham wrote:
> > ui/src/main/js/utils/__tests__/Update-test.js
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/62763/diff/1/?file=1846441#file1846441line47>
> >
> >     s/success/inprogress/

Fixed.


> On Oct. 6, 2017, 7:46 p.m., Santhosh Kumar Shanmugham wrote:
> > ui/src/main/js/client/scheduler-client.js
> > Line 2 (original), 2 (patched)
> > <https://reviews.apache.org/r/62763/diff/2/?file=1846834#file1846834line2>
> >
> >     Drop this?

Fixed.


> On Oct. 6, 2017, 7:46 p.m., Santhosh Kumar Shanmugham wrote:
> > ui/src/main/js/components/UpdateInstanceEvents.js
> > Lines 50 (patched)
> > <https://reviews.apache.org/r/62763/diff/2/?file=1846842#file1846842line50>
> >
> >     The `events` that are pushed into this component are already sorted. Do we require
an extra sort?

In the parent component, they are *reverse* sorted (to show a descending list of latest instances
updated) and then here they are sorted in ascending order (for the state machine).


- 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