Re: Review Request 62720: Implement Instance pages in React

[Santhosh Kumar Shanmugham]

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


Ship it!




Ship It!

- Santhosh Kumar Shanmugham


On Oct. 5, 2017, 4:50 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 5, 2017, 4:50 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/3/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

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




ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js
Lines 54 (patched)


Which test cases do you think are missing? moment is the library that 
actually generates the message here and I don't want to duplicate their own 
unit test suite in this test. I've only added this coverage at all because the 
component has logic that selects the first and last events before passing them 
to moment. So that is what is covered here. I could also have mocked moment but 
figured this was preferable.


- David McLaughlin


On Oct. 5, 2017, 11:50 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 5, 2017, 11:50 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/3/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[Kai Huang]

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


Ship it!




LGTM, except missing some test cases for showing correct task duration.


ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js
Lines 54 (patched)


Add a few more test case for this suite


- Kai Huang


On Oct. 5, 2017, 11:50 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 5, 2017, 11:50 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/3/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[Aurora ReviewBot]

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


Ship it!




Master (0f1e684) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 5, 2017, 11:50 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 5, 2017, 11:50 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/3/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

> On Oct. 5, 2017, 12:28 a.m., Kai Huang wrote:
> > ui/src/main/js/components/StateMachine.js
> > Lines 37 (patched)
> > 
> >
> > Add a key property here to suppress the warnings in the browser console?

Done.


- David


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


On Oct. 5, 2017, 11:50 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 5, 2017, 11:50 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/3/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

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

(Updated Oct. 5, 2017, 11:50 p.m.)


Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.


Changes
---

Remove key warning.


Repository: aurora


Description
---

Instance page moved over to the new UI. 

One of the big issues I ran into here was dealing with Thrift types. Currently 
the Thrift compiler generates a file for use in JS. The compiler gives you two 
options for JavaScript - one is 'node' which plays well with our module system 
but assumes you'll be using the built-in socket implementation in node, and 
then one for 'jquery' which uses the global namespace but doesn't use vars (so 
we can't eval as it breaks strict mode). So the TL;DR is - to write tests 
against scheduler states, I've had to copy and paste the Thrift enums into a 
test setup file. 

The alternative is to simply ignore Thrift types and duplicate it all without 
relying on globals. Thus making sure the tests and application code are using 
the same enums. However, this approach won't help you find any UI regressions 
when you update Thrift - and even worse, you won't spot any API regressions 
when developing the UI. So I've just gone for this approach for now.


Diffs (updated)
-

  ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
  ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
  ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
  ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
  ui/src/main/js/components/Layout.js 4ca54e318786859e23dc0444d7d2750817428e81 
  ui/src/main/js/components/StateMachine.js PRE-CREATION 
  ui/src/main/js/components/TaskDetails.js PRE-CREATION 
  ui/src/main/js/components/TaskStatus.js PRE-CREATION 
  ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
  ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js PRE-CREATION 
  ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
  ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
  ui/src/main/js/pages/Instance.js PRE-CREATION 
  ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
  ui/src/main/js/test-utils/Builder.js PRE-CREATION 
  ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
  ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
  ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
  ui/src/main/js/utils/Task.js PRE-CREATION 
  ui/src/main/js/utils/Thrift.js PRE-CREATION 
  ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
  ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
  ui/src/main/sass/components/_job-list-page.scss 
d31344d8eabb41114f4ff4678ff841119dfdac82 
  ui/src/main/sass/components/_layout.scss 
1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
  ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
  ui/src/main/sass/components/_status.scss PRE-CREATION 
  ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 


Diff: https://reviews.apache.org/r/62720/diff/3/

Changes: https://reviews.apache.org/r/62720/diff/2-3/


Testing
---

./gradlew ui:lint
./gradlew ui:test

Testing in Vagrant.


File Attachments


Active Task with State Machine component
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
Expanded Finished Task
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
Tooltip
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png


Thanks,

David McLaughlin

Re: Review Request 62720: Implement Instance pages in React

[Joshua Cohen]

> On Oct. 4, 2017, 9:42 p.m., Stephan Erb wrote:
> > Regarding the question nodejs vs js thrift: nodejs supports both binary 
> > (good) and compact (better) thrift protocols. This should reduce scheduler 
> > and client side serialization overhead and lead to a slightly more snappy 
> > UI.
> 
> David McLaughlin wrote:
> But it doesn't support XMLHttpRequest, so it can't be used in a UI bundle.
> 
> David McLaughlin wrote:
> Or maybe I misunderstood the comparison (between nodejs and js thrift)? 
> 
> To try and give a clearer picture of the problems:
> 
> ThriftJS library: absolutely required to deserialize the Thrift over HTTP 
> API the Scheduler exposes (unless we pin the UI to /apibeta). 
> 
> The files generated by the Thrift compiler (e.g. api_types.js, 
> ReadOnlyScheduler.js) are also required in order to build query objects (like 
> TaskQuery, etc.).
> 
> So when you generate the files, you have two options: node, js or jquery. 
> 
> Node:
> * Supports CommonJS modules, meaning you don't have to rely on global 
> scope and can instead import all Thrift types into your ES6 code (allowing 
> you to reuse Thrift types in the app and your test code). 
> * Assumes server-side environment, so uses the nodejs socket library to 
> perform client requests, rather than XMLHttpRequest.
> 
> jQuery:
> * Not only does it not support CommonJS, but the generated output doesn't 
> even use var declarations for the variables. This caues it to break strict 
> mode and means you can't even use node.vm to slurp it into the global context 
> in test-setup.js. 
> * Uses jQuery to perform all network requests, which uses the browser 
> environment. 
> 
> JS:
> * As jQuery, but all web requests are synchronous. 
> 
> 
> I tried an approach where I generated both jquery and node thrift files 
> and then imported them only for tests. But the compiled code also assumes the 
> ThriftJS library is also in the same format.. so it broke the tests and/or UI 
> bundle. At which point I just attached the copy and pasted structs and enums 
> to global in my test setup :)
> 
> Stephan Erb wrote:
> My comment above was originating based on some foggy memory of this 
> github comment on Thrift binary support for javascript 
> https://github.com/apache/thrift/pull/345#issuecomment-244588418 :)
> 
> Thanks for the thorough explanation.

Could we use the node.js code with a browser shim for the socket stuff? 
Something like: https://github.com/substack/http-browserify (not sure if 
that'll work w/ webpack, but in general it should be doable, and maybe 
preferable?)


- Joshua


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


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/Insta

Re: Review Request 62720: Implement Instance pages in React

[Kai Huang]

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




ui/src/main/js/components/StateMachine.js
Lines 37 (patched)


Add a key property here to suppress the warnings in the browser console?


- Kai Huang


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[Stephan Erb]

> On Oct. 4, 2017, 11:42 p.m., Stephan Erb wrote:
> > Regarding the question nodejs vs js thrift: nodejs supports both binary 
> > (good) and compact (better) thrift protocols. This should reduce scheduler 
> > and client side serialization overhead and lead to a slightly more snappy 
> > UI.
> 
> David McLaughlin wrote:
> But it doesn't support XMLHttpRequest, so it can't be used in a UI bundle.
> 
> David McLaughlin wrote:
> Or maybe I misunderstood the comparison (between nodejs and js thrift)? 
> 
> To try and give a clearer picture of the problems:
> 
> ThriftJS library: absolutely required to deserialize the Thrift over HTTP 
> API the Scheduler exposes (unless we pin the UI to /apibeta). 
> 
> The files generated by the Thrift compiler (e.g. api_types.js, 
> ReadOnlyScheduler.js) are also required in order to build query objects (like 
> TaskQuery, etc.).
> 
> So when you generate the files, you have two options: node, js or jquery. 
> 
> Node:
> * Supports CommonJS modules, meaning you don't have to rely on global 
> scope and can instead import all Thrift types into your ES6 code (allowing 
> you to reuse Thrift types in the app and your test code). 
> * Assumes server-side environment, so uses the nodejs socket library to 
> perform client requests, rather than XMLHttpRequest.
> 
> jQuery:
> * Not only does it not support CommonJS, but the generated output doesn't 
> even use var declarations for the variables. This caues it to break strict 
> mode and means you can't even use node.vm to slurp it into the global context 
> in test-setup.js. 
> * Uses jQuery to perform all network requests, which uses the browser 
> environment. 
> 
> JS:
> * As jQuery, but all web requests are synchronous. 
> 
> 
> I tried an approach where I generated both jquery and node thrift files 
> and then imported them only for tests. But the compiled code also assumes the 
> ThriftJS library is also in the same format.. so it broke the tests and/or UI 
> bundle. At which point I just attached the copy and pasted structs and enums 
> to global in my test setup :)

My comment above was originating based on some foggy memory of this github 
comment on Thrift binary support for javascript 
https://github.com/apache/thrift/pull/345#issuecomment-244588418 :)

Thanks for the thorough explanation.


- Stephan


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


On Oct. 4, 2017, 7:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 7:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/ma

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

> On Oct. 4, 2017, 9:42 p.m., Stephan Erb wrote:
> > Regarding the question nodejs vs js thrift: nodejs supports both binary 
> > (good) and compact (better) thrift protocols. This should reduce scheduler 
> > and client side serialization overhead and lead to a slightly more snappy 
> > UI.
> 
> David McLaughlin wrote:
> But it doesn't support XMLHttpRequest, so it can't be used in a UI bundle.

Or maybe I misunderstood the comparison (between nodejs and js thrift)? 

To try and give a clearer picture of the problems:

ThriftJS library: absolutely required to deserialize the Thrift over HTTP API 
the Scheduler exposes (unless we pin the UI to /apibeta). 

The files generated by the Thrift compiler (e.g. api_types.js, 
ReadOnlyScheduler.js) are also required in order to build query objects (like 
TaskQuery, etc.).

So when you generate the files, you have two options: node, js or jquery. 

Node:
* Supports CommonJS modules, meaning you don't have to rely on global scope and 
can instead import all Thrift types into your ES6 code (allowing you to reuse 
Thrift types in the app and your test code). 
* Assumes server-side environment, so uses the nodejs socket library to perform 
client requests, rather than XMLHttpRequest.

jQuery:
* Not only does it not support CommonJS, but the generated output doesn't even 
use var declarations for the variables. This caues it to break strict mode and 
means you can't even use node.vm to slurp it into the global context in 
test-setup.js. 
* Uses jQuery to perform all network requests, which uses the browser 
environment. 

JS:
* As jQuery, but all web requests are synchronous. 


I tried an approach where I generated both jquery and node thrift files and 
then imported them only for tests. But the compiled code also assumes the 
ThriftJS library is also in the same format.. so it broke the tests and/or UI 
bundle. At which point I just attached the copy and pasted structs and enums to 
global in my test setup :)


- David


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


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/

Re: Review Request 62720: Implement Instance pages in React

[Kai Huang]

> On Oct. 4, 2017, 9:01 p.m., Kai Huang wrote:
> > ui/src/main/js/index.js
> > Line 21 (original), 22 (patched)
> > 
> >
> > I'm not sure why I cannot access the job page and the instance page.
> > Visiting this url 
> > http://192.168.33.7:8081/beta/scheduler/vagrant/test/hello/0 on my vagrant 
> > image gives me the following error:
> > 
> > ```
> > Home.js:15 Uncaught TypeError: Cannot read property 'getRoleSummary' of 
> > undefined
> > at HomePage.componentWillMount (Home.js:15)
> > at setComponentProps (preact.esm.js:617)
> > at renderComponent (preact.esm.js:711)
> > at setComponentProps (preact.esm.js:634)
> > at buildComponentFromVNode (preact.esm.js:814)
> > at idiff (preact.esm.js:355)
> > at innerDiffNode (preact.esm.js:474)
> > at idiff (preact.esm.js:397)
> > at diff (preact.esm.js:306)
> > at renderComponent (preact.esm.js:727)
> > ```
> 
> David McLaughlin wrote:
> I don't think you've applied the patch properly then? It means the api 
> isn't injected.

It turns out to be an issue of my local vagrant environment. Please ignore this 
comment.


- Kai


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


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> --

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

> On Oct. 4, 2017, 9:42 p.m., Stephan Erb wrote:
> > Regarding the question nodejs vs js thrift: nodejs supports both binary 
> > (good) and compact (better) thrift protocols. This should reduce scheduler 
> > and client side serialization overhead and lead to a slightly more snappy 
> > UI.

But it doesn't support XMLHttpRequest, so it can't be used in a UI bundle.


- David


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


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

> On Oct. 4, 2017, 9:37 p.m., Stephan Erb wrote:
> > ui/src/main/js/utils/Task.js
> > Lines 40-41 (patched)
> > 
> >
> > Is the array index safe here? If we need the guard for 
> > `task.taskEvents.length` in `getLastEventTime` shouldn't we also require it 
> > here?

This file is mostly a lift and shift from the old UI. I'll dig into the 
Scheduler to see if it's possible to have a task without events. If so, I'll 
protect against it everywhere.


- David


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


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[Stephan Erb]

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



Regarding the question nodejs vs js thrift: nodejs supports both binary (good) 
and compact (better) thrift protocols. This should reduce scheduler and client 
side serialization overhead and lead to a slightly more snappy UI.

- Stephan Erb


On Oct. 4, 2017, 7:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 7:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[Stephan Erb]

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



Looks great! :)

When playing around with the UI on the vagrant box I bumped into a few null 
pointers. You might want to give it a shot there.


ui/src/main/js/components/InstanceHistoryItem.js
Lines 29 (patched)


This could fail if we have a malformed tasks without events.



ui/src/main/js/components/StateMachine.js
Lines 11 (patched)


Oh, thank you! I always would have preferred UTC over the LOCAL time in the 
current UI.



ui/src/main/js/utils/Task.js
Lines 40-41 (patched)


Is the array index safe here? If we need the guard for 
`task.taskEvents.length` in `getLastEventTime` shouldn't we also require it 
here?


- Stephan Erb


On Oct. 4, 2017, 7:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 7:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

> On Oct. 4, 2017, 9:01 p.m., Kai Huang wrote:
> > ui/src/main/js/index.js
> > Line 21 (original), 22 (patched)
> > 
> >
> > I'm not sure why I cannot access the job page and the instance page.
> > Visiting this url 
> > http://192.168.33.7:8081/beta/scheduler/vagrant/test/hello/0 on my vagrant 
> > image gives me the following error:
> > 
> > ```
> > Home.js:15 Uncaught TypeError: Cannot read property 'getRoleSummary' of 
> > undefined
> > at HomePage.componentWillMount (Home.js:15)
> > at setComponentProps (preact.esm.js:617)
> > at renderComponent (preact.esm.js:711)
> > at setComponentProps (preact.esm.js:634)
> > at buildComponentFromVNode (preact.esm.js:814)
> > at idiff (preact.esm.js:355)
> > at innerDiffNode (preact.esm.js:474)
> > at idiff (preact.esm.js:397)
> > at diff (preact.esm.js:306)
> > at renderComponent (preact.esm.js:727)
> > ```

I don't think you've applied the patch properly then? It means the api isn't 
injected.


- David


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


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16

Re: Review Request 62720: Implement Instance pages in React

[Kai Huang]

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




ui/src/main/js/index.js
Line 21 (original), 22 (patched)


I'm not sure why I cannot access the job page and the instance page.
Visiting this url 
http://192.168.33.7:8081/beta/scheduler/vagrant/test/hello/0 on my vagrant 
image gives me the following error:

```
Home.js:15 Uncaught TypeError: Cannot read property 'getRoleSummary' of 
undefined
at HomePage.componentWillMount (Home.js:15)
at setComponentProps (preact.esm.js:617)
at renderComponent (preact.esm.js:711)
at setComponentProps (preact.esm.js:634)
at buildComponentFromVNode (preact.esm.js:814)
at idiff (preact.esm.js:355)
at innerDiffNode (preact.esm.js:474)
at idiff (preact.esm.js:397)
at diff (preact.esm.js:306)
at renderComponent (preact.esm.js:727)
```


- Kai Huang


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.p

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

> On Oct. 4, 2017, 5:50 p.m., Reza Motamedi wrote:
> > ui/.eslintrc
> > Lines 15 (patched)
> > 
> >
> > For my education, does this set `ACTIVE_STATES`, `Thrift`, ... up for 
> > you to later assign values to them? Like what comes in "test-setup.js"?

This particular entry is to tell the linter that these values are in the global 
scope.


- David


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


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[Reza Motamedi]

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




ui/.eslintrc
Lines 15 (patched)


For my education, does this set `ACTIVE_STATES`, `Thrift`, ... up for you 
to later assign values to them? Like what comes in "test-setup.js"?


- Reza Motamedi


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[Aurora ReviewBot]

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


Ship it!




Master (4e7cdc4) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 4, 2017, 5:43 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> ---
> 
> (Updated Oct. 4, 2017, 5:43 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. 
> Currently the Thrift compiler generates a file for use in JS. The compiler 
> gives you two options for JavaScript - one is 'node' which plays well with 
> our module system but assumes you'll be using the built-in socket 
> implementation in node, and then one for 'jquery' which uses the global 
> namespace but doesn't use vars (so we can't eval as it breaks strict mode). 
> So the TL;DR is - to write tests against scheduler states, I've had to copy 
> and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without 
> relying on globals. Thus making sure the tests and application code are using 
> the same enums. However, this approach won't help you find any UI regressions 
> when you update Thrift - and even worse, you won't spot any API regressions 
> when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 
> 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js 
> PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss 
> d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 
> 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> 
> 
> Active Task with State Machine component
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 62720: Implement Instance pages in React

[David McLaughlin]

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

(Updated Oct. 4, 2017, 5:43 a.m.)


Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.


Repository: aurora


Description
---

Instance page moved over to the new UI. 

One of the big issues I ran into here was dealing with Thrift types. Currently 
the Thrift compiler generates a file for use in JS. The compiler gives you two 
options for JavaScript - one is 'node' which plays well with our module system 
but assumes you'll be using the built-in socket implementation in node, and 
then one for 'jquery' which uses the global namespace but doesn't use vars (so 
we can't eval as it breaks strict mode). So the TL;DR is - to write tests 
against scheduler states, I've had to copy and paste the Thrift enums into a 
test setup file. 

The alternative is to simply ignore Thrift types and duplicate it all without 
relying on globals. Thus making sure the tests and application code are using 
the same enums. However, this approach won't help you find any UI regressions 
when you update Thrift - and even worse, you won't spot any API regressions 
when developing the UI. So I've just gone for this approach for now.


Diffs (updated)
-

  ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
  ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
  ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
  ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
  ui/src/main/js/components/Layout.js 4ca54e318786859e23dc0444d7d2750817428e81 
  ui/src/main/js/components/StateMachine.js PRE-CREATION 
  ui/src/main/js/components/TaskDetails.js PRE-CREATION 
  ui/src/main/js/components/TaskStatus.js PRE-CREATION 
  ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
  ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js PRE-CREATION 
  ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
  ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
  ui/src/main/js/pages/Instance.js PRE-CREATION 
  ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
  ui/src/main/js/test-utils/Builder.js PRE-CREATION 
  ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
  ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
  ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
  ui/src/main/js/utils/Task.js PRE-CREATION 
  ui/src/main/js/utils/Thrift.js PRE-CREATION 
  ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
  ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
  ui/src/main/sass/components/_job-list-page.scss 
d31344d8eabb41114f4ff4678ff841119dfdac82 
  ui/src/main/sass/components/_layout.scss 
1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
  ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
  ui/src/main/sass/components/_status.scss PRE-CREATION 
  ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 


Diff: https://reviews.apache.org/r/62720/diff/2/

Changes: https://reviews.apache.org/r/62720/diff/1-2/


Testing
---

./gradlew ui:lint
./gradlew ui:test

Testing in Vagrant.


File Attachments


Active Task with State Machine component
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
Expanded Finished Task
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
Tooltip
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png


Thanks,

David McLaughlin

Review Request 62720: Implement Instance pages in React

[David McLaughlin]

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

Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.


Repository: aurora


Description
---

Instance page moved over to the new UI. 

One of the big issues I ran into here was dealing with Thrift types. Currently 
the Thrift compiler generates a file for use in JS. The compiler gives you two 
options for JavaScript - one is 'node' which plays well with our module system 
but assumes you'll be using the built-in socket implementation in node, and 
then one for 'jquery' which uses the global namespace but doesn't use vars (so 
we can't eval as it breaks strict mode). So the TL;DR is - to write tests 
against scheduler states, I've had to copy and paste the Thrift enums into a 
test setup file. 

The alternative is to simply ignore Thrift types and duplicate it all without 
relying on globals. Thus making sure the tests and application code are using 
the same enums. However, this approach won't help you find any UI regressions 
when you update Thrift - and even worse, you won't spot any API regressions 
when developing the UI. So I've just gone for this approach for now.


Diffs
-

  ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
  ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
  ui/src/main/js/client/scheduler-client.js 
1c381086934dafa22a62d18e035f599fb2260e15 
  ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
  ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
  ui/src/main/js/components/Layout.js 4ca54e318786859e23dc0444d7d2750817428e81 
  ui/src/main/js/components/StateMachine.js PRE-CREATION 
  ui/src/main/js/components/TaskDetails.js PRE-CREATION 
  ui/src/main/js/components/TaskStatus.js PRE-CREATION 
  ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
  ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js PRE-CREATION 
  ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
  ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
  ui/src/main/js/pages/Instance.js PRE-CREATION 
  ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
  ui/src/main/js/test-utils/Builder.js PRE-CREATION 
  ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
  ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
  ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
  ui/src/main/js/utils/Task.js PRE-CREATION 
  ui/src/main/js/utils/Thrift.js PRE-CREATION 
  ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
  ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
  ui/src/main/sass/components/_job-list-page.scss 
d31344d8eabb41114f4ff4678ff841119dfdac82 
  ui/src/main/sass/components/_layout.scss 
1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
  ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
  ui/src/main/sass/components/_status.scss PRE-CREATION 
  ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 


Diff: https://reviews.apache.org/r/62720/diff/1/


Testing
---

./gradlew ui:lint
./gradlew ui:test

Testing in Vagrant.


File Attachments


Active Task with State Machine component
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
Expanded Finished Task
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
Tooltip
  
https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png


Thanks,

David McLaughlin