Re: Review Request 63418: Remove endpoint.thrift, ServiceInstance is never serialized to thrift

[Aurora ReviewBot]

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


Ship it!




Master (87eb891) 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. 30, 2017, 4:36 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63418/
> ---
> 
> (Updated Oct. 30, 2017, 4:36 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This enables removal of some unnecessary complexity in the build (commons no
> longer needs thrift) and the unused Codec abstraction (we always encode in
> JSON).
> 
> 
> Diffs
> -
> 
>   build.gradle 1c1d3811c861dc6ff16bbd9ce3daefb5833f52df 
>   commons/src/main/java/org/apache/aurora/common/io/Codec.java 
> 94d1e3654767d8e5c0280595d0944c2fcb68ef90 
>   commons/src/main/java/org/apache/aurora/common/thrift/Util.java 
> 4ef7f49b7953b56b592b2f8ac3e04df738615211 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/Encoding.java 
> 204f5c45b6810a82bdb74fb2d27fe2def6fd568a 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java 
> 9d316088ffd51b2dce816e6cc18d8e2a4a23cfd5 
>   
> commons/src/main/resources/org/apache/aurora/common/net/http/handlers/thrift.st
>  0586653e1b22a96b0ce71e2fc979d6d3a71d1677 
>   commons/src/main/thrift/org/apache/aurora/common/thrift/endpoint.thrift 
> 87da2e13db900a4b06f50d89c9559ecfe202508e 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/EncodingTest.java 
> 16c017104c4696a985eefa23e1611300a7357259 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java 
> 6cf335dd451ca05bb511d524ff5590b219505cae 
>   src/main/java/org/apache/aurora/scheduler/app/ServiceGroupMonitor.java 
> a1329fd5010fadbf6bceeb2ae0476a6261830492 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
>  77f90eead2812e24d0d56e289bba7944075ee3bc 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
>  eba56bea89961fd8e6507eaf4180f700e4b030c4 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorSingletonService.java
>  2847c41b15b64b6a91bae295a9e5a96680b0c816 
>   src/main/java/org/apache/aurora/scheduler/discovery/Encoding.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java 
> bc0e2a86c0a8a354d95350a3d2f63514ee26da70 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 3ed256b30a0492d65560344c3fc038d8d93f2ec0 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/BaseCuratorDiscoveryTest.java
>  02c81838c52245aad3c582e0dea62f4ca3693379 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
>  e6b57ee7d0af234a60794e10c63c2670f67e6ccd 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorSingletonServiceTest.java
>  946a78e185962b1459ce0dc32c55a036d04a7e19 
>   src/test/java/org/apache/aurora/scheduler/discovery/EncodingTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> f3ae5a566b555b753230629897847c1bb5abc3b1 
>   src/test/java/org/apache/aurora/scheduler/http/LeaderRedirectTest.java 
> a7cc046d97d7b817a5496a6a7190142375b7a8a1 
> 
> 
> Diff: https://reviews.apache.org/r/63418/diff/3/
> 
> 
> Testing
> ---
> 
> e2e tests pass
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 63436: Enabling ErrorBoundary in Scheduler UI

[Joshua Cohen]

> On Oct. 31, 2017, 2:08 a.m., David McLaughlin wrote:
> > ui/src/main/js/components/ErrorBoundary.js
> > Lines 28 (patched)
> > 
> >
> > Link to the Apache JIRA? And ask them to post the stacktrace below?

Could even create a link that will pre-populate the ticket: 
https://confluence.atlassian.com/jirakb/creating-issues-via-direct-html-links-159474.html.


- Joshua


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


On Oct. 31, 2017, 1:58 a.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63436/
> ---
> 
> (Updated Oct. 31, 2017, 1:58 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Kai Huang.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> # Enabling ErrorBoundary in Scheduler UI
> React 16 introduces a new concept of an “error boundary” that allows us to 
> limit the impact of an error and not unmount the whole component tree. I am 
> open to keeping or removing the stack trace.
> 
> from React docs:
> > As of React 16, errors that were not caught by any error boundary will 
> > result in unmounting of the whole React component tree.
> 
> 
> Diffs
> -
> 
>   ui/src/main/js/components/ErrorBoundary.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/ErrorBoundary-test.js PRE-CREATION 
>   ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93 
>   ui/src/main/sass/app.scss 8a10e1c5a501828c39ca7b9cf6b518a2d7a99d28 
>   ui/src/main/sass/components/_error-boundary.scss PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63436/diff/1/
> 
> 
> Testing
> ---
> 
> - Unit tests added. This however causes an error message to be logged on 
> stdout.
> 
> ```
> ? ./gradlew ui:test
> ...
> ASS src/main/js/pages/__tests__/Job-test.js
> PASS src/main/js/components/__tests__/InstanceViz-test.js
> PASS src/main/js/pages/__tests__/Update-test.js
> PASS src/main/js/components/__tests__/InstanceHistoryItem-test.js
> PASS src/main/js/components/__tests__/Pagination-test.js
> PASS src/main/js/components/__tests__/ErrorBoundary-test.js
>   ? Console
> 
> console.error node_modules/react-dom/cjs/react-dom.development.js:8305
>   The above error occurred in the  component:
>   in ComponentWithError
>   in ErrorBoundary (created by WrapperComponent)
>   in WrapperComponent
> 
>   React will try to recreate this component tree from scratch using the 
> error boundary you provided, ErrorBoundary.
> ...
> 
> BUILD SUCCESSFUL in 0s
> 2 actionable tasks: 2 up-to-date
> 
> # rmotamedi@tw-mbp-rmotamedi:~/oss/aurora on git:error-boundry ? [18:52:22]
> ? ./gradlew ui:lint
> 
> BUILD SUCCESSFUL in 6s
> 4 actionable tasks: 4 up-to-date
> ```
> 
> 
> - For more testing, I added the following component (that raises an error) 
> and installed it under `UpdateInstanceEvents`. I then caught the error at 
> various depths in the doom tree. I attached the screen-shots of how this will 
> render.
> 
> ## ComponentWithError
> 
> ```
> import React from 'react'
> 
> export default class ComponentWithError extends React.Component {
>   render() {
> throw new Error('Crashed!');
> return ;
>   }
> }
> ```
> 
> 
> File Attachments
> 
> 
> handling boundary on the main router
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/31/d95e9f85-3dad-4332-ac33-3cb9f8a4455c__Screen_Shot_2017-10-30_at_6.30.22_PM.png
> handling boundary on a component deep in the doom tree.
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/31/64d47905-b058-4ef5-9d19-fa3311a47f9d__Screen_Shot_2017-10-30_at_6.36.18_PM.png
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>

Re: Review Request 63436: Enabling ErrorBoundary in Scheduler UI

[Kai Huang]

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



+1 to fix the error message in test. Wonder if we could try this? 
https://github.com/facebook/react/issues/11098

- Kai Huang


On Oct. 31, 2017, 1:58 a.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63436/
> ---
> 
> (Updated Oct. 31, 2017, 1:58 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Kai Huang.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> # Enabling ErrorBoundary in Scheduler UI
> React 16 introduces a new concept of an “error boundary” that allows us to 
> limit the impact of an error and not unmount the whole component tree. I am 
> open to keeping or removing the stack trace.
> 
> from React docs:
> > As of React 16, errors that were not caught by any error boundary will 
> > result in unmounting of the whole React component tree.
> 
> 
> Diffs
> -
> 
>   ui/src/main/js/components/ErrorBoundary.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/ErrorBoundary-test.js PRE-CREATION 
>   ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93 
>   ui/src/main/sass/app.scss 8a10e1c5a501828c39ca7b9cf6b518a2d7a99d28 
>   ui/src/main/sass/components/_error-boundary.scss PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63436/diff/1/
> 
> 
> Testing
> ---
> 
> - Unit tests added. This however causes an error message to be logged on 
> stdout.
> 
> ```
> ? ./gradlew ui:test
> ...
> ASS src/main/js/pages/__tests__/Job-test.js
> PASS src/main/js/components/__tests__/InstanceViz-test.js
> PASS src/main/js/pages/__tests__/Update-test.js
> PASS src/main/js/components/__tests__/InstanceHistoryItem-test.js
> PASS src/main/js/components/__tests__/Pagination-test.js
> PASS src/main/js/components/__tests__/ErrorBoundary-test.js
>   ? Console
> 
> console.error node_modules/react-dom/cjs/react-dom.development.js:8305
>   The above error occurred in the  component:
>   in ComponentWithError
>   in ErrorBoundary (created by WrapperComponent)
>   in WrapperComponent
> 
>   React will try to recreate this component tree from scratch using the 
> error boundary you provided, ErrorBoundary.
> ...
> 
> BUILD SUCCESSFUL in 0s
> 2 actionable tasks: 2 up-to-date
> 
> # rmotamedi@tw-mbp-rmotamedi:~/oss/aurora on git:error-boundry ? [18:52:22]
> ? ./gradlew ui:lint
> 
> BUILD SUCCESSFUL in 6s
> 4 actionable tasks: 4 up-to-date
> ```
> 
> 
> - For more testing, I added the following component (that raises an error) 
> and installed it under `UpdateInstanceEvents`. I then caught the error at 
> various depths in the doom tree. I attached the screen-shots of how this will 
> render.
> 
> ## ComponentWithError
> 
> ```
> import React from 'react'
> 
> export default class ComponentWithError extends React.Component {
>   render() {
> throw new Error('Crashed!');
> return ;
>   }
> }
> ```
> 
> 
> File Attachments
> 
> 
> handling boundary on the main router
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/31/d95e9f85-3dad-4332-ac33-3cb9f8a4455c__Screen_Shot_2017-10-30_at_6.30.22_PM.png
> handling boundary on a component deep in the doom tree.
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/31/64d47905-b058-4ef5-9d19-fa3311a47f9d__Screen_Shot_2017-10-30_at_6.36.18_PM.png
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>

Re: Review Request 63436: Enabling ErrorBoundary in Scheduler UI

[Aurora ReviewBot]

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


Ship it!




Master (87eb891) 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. 31, 2017, 1:58 a.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63436/
> ---
> 
> (Updated Oct. 31, 2017, 1:58 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Kai Huang.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> # Enabling ErrorBoundary in Scheduler UI
> React 16 introduces a new concept of an “error boundary” that allows us to 
> limit the impact of an error and not unmount the whole component tree. I am 
> open to keeping or removing the stack trace.
> 
> from React docs:
> > As of React 16, errors that were not caught by any error boundary will 
> > result in unmounting of the whole React component tree.
> 
> 
> Diffs
> -
> 
>   ui/src/main/js/components/ErrorBoundary.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/ErrorBoundary-test.js PRE-CREATION 
>   ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93 
>   ui/src/main/sass/app.scss 8a10e1c5a501828c39ca7b9cf6b518a2d7a99d28 
>   ui/src/main/sass/components/_error-boundary.scss PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63436/diff/1/
> 
> 
> Testing
> ---
> 
> - Unit tests added. This however causes an error message to be logged on 
> stdout.
> 
> ```
> ? ./gradlew ui:test
> ...
> ASS src/main/js/pages/__tests__/Job-test.js
> PASS src/main/js/components/__tests__/InstanceViz-test.js
> PASS src/main/js/pages/__tests__/Update-test.js
> PASS src/main/js/components/__tests__/InstanceHistoryItem-test.js
> PASS src/main/js/components/__tests__/Pagination-test.js
> PASS src/main/js/components/__tests__/ErrorBoundary-test.js
>   ? Console
> 
> console.error node_modules/react-dom/cjs/react-dom.development.js:8305
>   The above error occurred in the  component:
>   in ComponentWithError
>   in ErrorBoundary (created by WrapperComponent)
>   in WrapperComponent
> 
>   React will try to recreate this component tree from scratch using the 
> error boundary you provided, ErrorBoundary.
> ...
> 
> BUILD SUCCESSFUL in 0s
> 2 actionable tasks: 2 up-to-date
> 
> # rmotamedi@tw-mbp-rmotamedi:~/oss/aurora on git:error-boundry ? [18:52:22]
> ? ./gradlew ui:lint
> 
> BUILD SUCCESSFUL in 6s
> 4 actionable tasks: 4 up-to-date
> ```
> 
> 
> - For more testing, I added the following component (that raises an error) 
> and installed it under `UpdateInstanceEvents`. I then caught the error at 
> various depths in the doom tree. I attached the screen-shots of how this will 
> render.
> 
> ## ComponentWithError
> 
> ```
> import React from 'react'
> 
> export default class ComponentWithError extends React.Component {
>   render() {
> throw new Error('Crashed!');
> return ;
>   }
> }
> ```
> 
> 
> File Attachments
> 
> 
> handling boundary on the main router
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/31/d95e9f85-3dad-4332-ac33-3cb9f8a4455c__Screen_Shot_2017-10-30_at_6.30.22_PM.png
> handling boundary on a component deep in the doom tree.
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/31/64d47905-b058-4ef5-9d19-fa3311a47f9d__Screen_Shot_2017-10-30_at_6.36.18_PM.png
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>

Re: Review Request 63436: Enabling ErrorBoundary in Scheduler UI

[David McLaughlin]

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



This looks good. Just to confirm - the behavior we'd see after this ships is 
the screenshot on the left, with the router? Even if a deep component has an 
error?

I'm not a huge fan of the error being logged in the test output. Is there any 
way to avoid that?


ui/src/main/js/components/ErrorBoundary.js
Lines 28 (patched)


Link to the Apache JIRA? And ask them to post the stacktrace below?


- David McLaughlin


On Oct. 31, 2017, 1:58 a.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63436/
> ---
> 
> (Updated Oct. 31, 2017, 1:58 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Kai Huang.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> # Enabling ErrorBoundary in Scheduler UI
> React 16 introduces a new concept of an “error boundary” that allows us to 
> limit the impact of an error and not unmount the whole component tree. I am 
> open to keeping or removing the stack trace.
> 
> from React docs:
> > As of React 16, errors that were not caught by any error boundary will 
> > result in unmounting of the whole React component tree.
> 
> 
> Diffs
> -
> 
>   ui/src/main/js/components/ErrorBoundary.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/ErrorBoundary-test.js PRE-CREATION 
>   ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93 
>   ui/src/main/sass/app.scss 8a10e1c5a501828c39ca7b9cf6b518a2d7a99d28 
>   ui/src/main/sass/components/_error-boundary.scss PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63436/diff/1/
> 
> 
> Testing
> ---
> 
> - Unit tests added. This however causes an error message to be logged on 
> stdout.
> 
> ```
> ? ./gradlew ui:test
> ...
> ASS src/main/js/pages/__tests__/Job-test.js
> PASS src/main/js/components/__tests__/InstanceViz-test.js
> PASS src/main/js/pages/__tests__/Update-test.js
> PASS src/main/js/components/__tests__/InstanceHistoryItem-test.js
> PASS src/main/js/components/__tests__/Pagination-test.js
> PASS src/main/js/components/__tests__/ErrorBoundary-test.js
>   ? Console
> 
> console.error node_modules/react-dom/cjs/react-dom.development.js:8305
>   The above error occurred in the  component:
>   in ComponentWithError
>   in ErrorBoundary (created by WrapperComponent)
>   in WrapperComponent
> 
>   React will try to recreate this component tree from scratch using the 
> error boundary you provided, ErrorBoundary.
> ...
> 
> BUILD SUCCESSFUL in 0s
> 2 actionable tasks: 2 up-to-date
> 
> # rmotamedi@tw-mbp-rmotamedi:~/oss/aurora on git:error-boundry ? [18:52:22]
> ? ./gradlew ui:lint
> 
> BUILD SUCCESSFUL in 6s
> 4 actionable tasks: 4 up-to-date
> ```
> 
> 
> - For more testing, I added the following component (that raises an error) 
> and installed it under `UpdateInstanceEvents`. I then caught the error at 
> various depths in the doom tree. I attached the screen-shots of how this will 
> render.
> 
> ## ComponentWithError
> 
> ```
> import React from 'react'
> 
> export default class ComponentWithError extends React.Component {
>   render() {
> throw new Error('Crashed!');
> return ;
>   }
> }
> ```
> 
> 
> File Attachments
> 
> 
> handling boundary on the main router
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/31/d95e9f85-3dad-4332-ac33-3cb9f8a4455c__Screen_Shot_2017-10-30_at_6.30.22_PM.png
> handling boundary on a component deep in the doom tree.
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/31/64d47905-b058-4ef5-9d19-fa3311a47f9d__Screen_Shot_2017-10-30_at_6.36.18_PM.png
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>

Review Request 63436: Enabling ErrorBoundary in Scheduler UI

[Reza Motamedi]

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

Review request for Aurora, David McLaughlin, Joshua Cohen, and Kai Huang.


Repository: aurora


Description
---

# Enabling ErrorBoundary in Scheduler UI
React 16 introduces a new concept of an “error boundary” that allows us to 
limit the impact of an error and not unmount the whole component tree. I am 
open to keeping or removing the stack trace.

from React docs:
> As of React 16, errors that were not caught by any error boundary will result 
> in unmounting of the whole React component tree.


Diffs
-

  ui/src/main/js/components/ErrorBoundary.js PRE-CREATION 
  ui/src/main/js/components/__tests__/ErrorBoundary-test.js PRE-CREATION 
  ui/src/main/js/index.js 9f94d4bd6f649d74bdd9c3aa99783e01cae25d93 
  ui/src/main/sass/app.scss 8a10e1c5a501828c39ca7b9cf6b518a2d7a99d28 
  ui/src/main/sass/components/_error-boundary.scss PRE-CREATION 


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


Testing
---

- Unit tests added. This however causes an error message to be logged on stdout.

```
? ./gradlew ui:test
...
ASS src/main/js/pages/__tests__/Job-test.js
PASS src/main/js/components/__tests__/InstanceViz-test.js
PASS src/main/js/pages/__tests__/Update-test.js
PASS src/main/js/components/__tests__/InstanceHistoryItem-test.js
PASS src/main/js/components/__tests__/Pagination-test.js
PASS src/main/js/components/__tests__/ErrorBoundary-test.js
  ? Console

console.error node_modules/react-dom/cjs/react-dom.development.js:8305
  The above error occurred in the  component:
  in ComponentWithError
  in ErrorBoundary (created by WrapperComponent)
  in WrapperComponent

  React will try to recreate this component tree from scratch using the 
error boundary you provided, ErrorBoundary.
...

BUILD SUCCESSFUL in 0s
2 actionable tasks: 2 up-to-date

# rmotamedi@tw-mbp-rmotamedi:~/oss/aurora on git:error-boundry ? [18:52:22]
? ./gradlew ui:lint

BUILD SUCCESSFUL in 6s
4 actionable tasks: 4 up-to-date
```


- For more testing, I added the following component (that raises an error) and 
installed it under `UpdateInstanceEvents`. I then caught the error at various 
depths in the doom tree. I attached the screen-shots of how this will render.

## ComponentWithError

```
import React from 'react'

export default class ComponentWithError extends React.Component {
  render() {
throw new Error('Crashed!');
return ;
  }
}
```


File Attachments


handling boundary on the main router
  
https://reviews.apache.org/media/uploaded/files/2017/10/31/d95e9f85-3dad-4332-ac33-3cb9f8a4455c__Screen_Shot_2017-10-30_at_6.30.22_PM.png
handling boundary on a component deep in the doom tree.
  
https://reviews.apache.org/media/uploaded/files/2017/10/31/64d47905-b058-4ef5-9d19-fa3311a47f9d__Screen_Shot_2017-10-30_at_6.36.18_PM.png


Thanks,

Reza Motamedi

Re: Review Request 63418: Remove endpoint.thrift, ServiceInstance is never serialized to thrift

[Aurora ReviewBot]

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



Master (87eb891) is red with this patch.
  ./build-support/jenkins/build.sh

:distZip
:assemble
:compileTestJavaNote: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processTestResources
:testClasses
:compileJmhJavaNote: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processJmhResources NO-SOURCE
:jmhClasses
:checkstyleJmh
:checkstyleMain
:checkstyleTest
:findbugsJmh
:findbugsMain
:findbugsTest
:licenseJmh UP-TO-DATE
:licenseMain UP-TO-DATE
:licenseTest UP-TO-DATE
:license UP-TO-DATE
:pmdJmh
:pmdMain
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java:29:
This final field could be made static
:pmdMain FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':pmdMain'.
> 1 PMD rule violations were found. See the report at: 
> file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/pmd/main.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.

* Get more help at https://help.gradle.org

BUILD FAILED in 4m 54s
40 actionable tasks: 31 executed, 9 up-to-date


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

- Aurora ReviewBot


On Oct. 30, 2017, 4:36 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63418/
> ---
> 
> (Updated Oct. 30, 2017, 4:36 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This enables removal of some unnecessary complexity in the build (commons no
> longer needs thrift) and the unused Codec abstraction (we always encode in
> JSON).
> 
> 
> Diffs
> -
> 
>   build.gradle 1c1d3811c861dc6ff16bbd9ce3daefb5833f52df 
>   commons/src/main/java/org/apache/aurora/common/io/Codec.java 
> 94d1e3654767d8e5c0280595d0944c2fcb68ef90 
>   commons/src/main/java/org/apache/aurora/common/thrift/Util.java 
> 4ef7f49b7953b56b592b2f8ac3e04df738615211 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/Encoding.java 
> 204f5c45b6810a82bdb74fb2d27fe2def6fd568a 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java 
> 9d316088ffd51b2dce816e6cc18d8e2a4a23cfd5 
>   
> commons/src/main/resources/org/apache/aurora/common/net/http/handlers/thrift.st
>  0586653e1b22a96b0ce71e2fc979d6d3a71d1677 
>   commons/src/main/thrift/org/apache/aurora/common/thrift/endpoint.thrift 
> 87da2e13db900a4b06f50d89c9559ecfe202508e 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/EncodingTest.java 
> 16c017104c4696a985eefa23e1611300a7357259 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java 
> 6cf335dd451ca05bb511d524ff5590b219505cae 
>   src/main/java/org/apache/aurora/scheduler/app/ServiceGroupMonitor.java 
> a1329fd5010fadbf6bceeb2ae0476a6261830492 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
>  77f90eead2812e24d0d56e289bba7944075ee3bc 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
>  eba56bea89961fd8e6507eaf4180f700e4b030c4 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorSingletonService.java
>  2847c41b15b64b6a91bae295a9e5a96680b0c816 
>   src/main/java/org/apache/aurora/scheduler/discovery/Encoding.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java 
> bc0e2a86c0a8a354d95350a3d2f63514ee26da70 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 3ed256b30a0492d65560344c3fc038d8d93f2ec0 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/BaseCuratorDiscoveryTest.java
>  02c81838c52245aad3c582e0dea62f4ca3693379 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
>  e6b57ee7d0af234a60794e10c63c2670f67e6ccd 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorSingletonServiceTest.java
>  946a78e185962b1459ce0dc32c55a036d04a7e19 
>   src/test/java/org/apache/aurora/scheduler/discovery/EncodingTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> f3ae5a566b555b753230629897847c1bb5abc3b1 
>   

Re: Review Request 63199: Refactor staticallyBannedOffers into a LRU cache

[Aurora ReviewBot]

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


Ship it!




Master (87eb891) 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. 30, 2017, 10:50 p.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63199/
> ---
> 
> (Updated Oct. 30, 2017, 10:50 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, 
> Stephan Erb, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Using the new `hold_offers_forever` option, it is possible for the 
> `staticallyBannedOffers` to grow very large in size as we never release 
> offers.
> 
> As an alternative to https://reviews.apache.org/r/63121/, I propose changing 
> `staticallyBannedOffers` into a LRU cache which expires entries after 
> `min_offer_hold_time` + `offer_hold_jitter_window` (referred to as 
> `maxOfferHoldTime`), while also taking an option for a maximum size for the 
> cache. I believe that this approach has a couple of benefits:
> 
> 1. The current behavior of `staticallyBannedOffers` is (kinda) preserved. 
> Entries will no longer be removed when the offer is used, but they will be 
> removed within `maxOfferHoldTime`. This means cluster operators will not have 
> to think about the new `offer_static_ban_cache_max_size` if they aren't 
> affected by the memory leak now.
> 2. Cluster operators that use Aurora as a single framework and hold offers 
> indefinitely can cap the size of the cache to avoid the memory leak.
> 3. Using an LRU cache greatly benefits quickly recurring crons and job 
> updates.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> e0ec793cad05674fb4b65246a6d153521b28b914 
>   
> src/main/java/org/apache/aurora/scheduler/config/validators/NotNegativeNumber.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 7011a4cc9eea827cdd54698aaed1a653774bce7f 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java 
> e060f2073dce4d2486d1ee2bfd873fe75167c6d0 
>   src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 
> e6b2c55e4f33f9a603157236766425edcaff10e7 
>   src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java 
> 244422c6b3ac6a2f7b4690cdc0f3440170b2567f 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> 3d38a5929a0255a980db30eeca0f059a2029f321 
> 
> 
> Diff: https://reviews.apache.org/r/63199/diff/6/
> 
> 
> Testing
> ---
> 
> Unit tests pass.
> Deployed on a scale test cluster and saw that a) `staticallyBannedOffers` 
> memory leak fixed with correct options and b) lowered assignment time for 
> quickly recurring crons and rescheduled jobs.
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>

Re: Review Request 63435: Remove inaccurate "Initializing sandbox" message

[Aurora ReviewBot]

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



Master (87eb891) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On Oct. 30, 2017, 11:43 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63435/
> ---
> 
> (Updated Oct. 30, 2017, 11:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The message is no longer completely accurate, now that we remain in 
> `STARTING` until health checks have passed.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> aeb3a4d4420c2a7a95ec9a866633bc0c0897ac56 
> 
> 
> Diff: https://reviews.apache.org/r/63435/diff/1/
> 
> 
> Testing
> ---
> 
> ./pants test.pytest src/test/python::
> 
> 
> File Attachments
> 
> 
> Screen Shot 2017-10-31 at 00.16.56.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/685d76fe-3db2-4761-80c1-f0af83642d0e__Screen_Shot_2017-10-31_at_00.16.56.png
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 63435: Remove inaccurate "Initializing sandbox" message

[David McLaughlin]

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


Ship it!




Also +1 to shipping as-is, given that inaccurate message is more harmful than 
no message at all.

- David McLaughlin


On Oct. 30, 2017, 11:43 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63435/
> ---
> 
> (Updated Oct. 30, 2017, 11:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The message is no longer completely accurate, now that we remain in 
> `STARTING` until health checks have passed.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> aeb3a4d4420c2a7a95ec9a866633bc0c0897ac56 
> 
> 
> Diff: https://reviews.apache.org/r/63435/diff/1/
> 
> 
> Testing
> ---
> 
> ./pants test.pytest src/test/python::
> 
> 
> File Attachments
> 
> 
> Screen Shot 2017-10-31 at 00.16.56.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/685d76fe-3db2-4761-80c1-f0af83642d0e__Screen_Shot_2017-10-31_at_00.16.56.png
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 63435: Remove inaccurate "Initializing sandbox" message

[Bill Farner]

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


Ship it!




LGTM since the current config lies, but added a wish list item.


src/main/python/apache/aurora/executor/aurora_executor.py
Line 98 (original), 98 (patched)


Bonus points for an `Awaiting task health checks` message when applicable.


- Bill Farner


On Oct. 30, 2017, 4:43 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63435/
> ---
> 
> (Updated Oct. 30, 2017, 4:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The message is no longer completely accurate, now that we remain in 
> `STARTING` until health checks have passed.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> aeb3a4d4420c2a7a95ec9a866633bc0c0897ac56 
> 
> 
> Diff: https://reviews.apache.org/r/63435/diff/1/
> 
> 
> Testing
> ---
> 
> ./pants test.pytest src/test/python::
> 
> 
> File Attachments
> 
> 
> Screen Shot 2017-10-31 at 00.16.56.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/685d76fe-3db2-4761-80c1-f0af83642d0e__Screen_Shot_2017-10-31_at_00.16.56.png
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Review Request 63435: Remove inaccurate "Initializing sandbox" message

[Stephan Erb]

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

Review request for Aurora, David McLaughlin and Bill Farner.


Repository: aurora


Description
---

The message is no longer completely accurate, now that we remain in 
`STARTING` until health checks have passed.


Diffs
-

  src/main/python/apache/aurora/executor/aurora_executor.py 
aeb3a4d4420c2a7a95ec9a866633bc0c0897ac56 


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


Testing
---

./pants test.pytest src/test/python::


File Attachments


Screen Shot 2017-10-31 at 00.16.56.png
  
https://reviews.apache.org/media/uploaded/files/2017/10/30/685d76fe-3db2-4761-80c1-f0af83642d0e__Screen_Shot_2017-10-31_at_00.16.56.png


Thanks,

Stephan Erb

Re: Review Request 63199: Refactor staticallyBannedOffers into a LRU cache

[Jordan Ly]

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

(Updated Oct. 30, 2017, 10:50 p.m.)


Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan 
Erb, and Bill Farner.


Changes
---

Create `CacheBuilder` in `OfferSettings` instead, pass to `OfferManager`


Repository: aurora


Description
---

Using the new `hold_offers_forever` option, it is possible for the 
`staticallyBannedOffers` to grow very large in size as we never release offers.

As an alternative to https://reviews.apache.org/r/63121/, I propose changing 
`staticallyBannedOffers` into a LRU cache which expires entries after 
`min_offer_hold_time` + `offer_hold_jitter_window` (referred to as 
`maxOfferHoldTime`), while also taking an option for a maximum size for the 
cache. I believe that this approach has a couple of benefits:

1. The current behavior of `staticallyBannedOffers` is (kinda) preserved. 
Entries will no longer be removed when the offer is used, but they will be 
removed within `maxOfferHoldTime`. This means cluster operators will not have 
to think about the new `offer_static_ban_cache_max_size` if they aren't 
affected by the memory leak now.
2. Cluster operators that use Aurora as a single framework and hold offers 
indefinitely can cap the size of the cache to avoid the memory leak.
3. Using an LRU cache greatly benefits quickly recurring crons and job updates.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
e0ec793cad05674fb4b65246a6d153521b28b914 
  
src/main/java/org/apache/aurora/scheduler/config/validators/NotNegativeNumber.java
 PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
7011a4cc9eea827cdd54698aaed1a653774bce7f 
  src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java 
e060f2073dce4d2486d1ee2bfd873fe75167c6d0 
  src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 
e6b2c55e4f33f9a603157236766425edcaff10e7 
  src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java 
244422c6b3ac6a2f7b4690cdc0f3440170b2567f 
  src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
3d38a5929a0255a980db30eeca0f059a2029f321 


Diff: https://reviews.apache.org/r/63199/diff/6/

Changes: https://reviews.apache.org/r/63199/diff/5-6/


Testing
---

Unit tests pass.
Deployed on a scale test cluster and saw that a) `staticallyBannedOffers` 
memory leak fixed with correct options and b) lowered assignment time for 
quickly recurring crons and rescheduled jobs.


Thanks,

Jordan Ly

Re: Review Request 63418: Remove endpoint.thrift, ServiceInstance is never serialized to thrift

[Stephan Erb]

> On Oct. 30, 2017, 7:09 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java
> > Lines 45 (patched)
> > 
> >
> > Should we just hardcode ALIVE here? Aurora is not using anything else.
> > 
> > We could also add comments to the enum that all other state are 
> > deprecated and not used.
> 
> Bill Farner wrote:
> Thanks for the nudge, this crossed my mind as well while making the 
> change.  I've gone ahead and buried the field to effectively hard-code it.

Thanks. LGTM


- Stephan


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


On Oct. 30, 2017, 5:36 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63418/
> ---
> 
> (Updated Oct. 30, 2017, 5:36 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This enables removal of some unnecessary complexity in the build (commons no
> longer needs thrift) and the unused Codec abstraction (we always encode in
> JSON).
> 
> 
> Diffs
> -
> 
>   build.gradle 1c1d3811c861dc6ff16bbd9ce3daefb5833f52df 
>   commons/src/main/java/org/apache/aurora/common/io/Codec.java 
> 94d1e3654767d8e5c0280595d0944c2fcb68ef90 
>   commons/src/main/java/org/apache/aurora/common/thrift/Util.java 
> 4ef7f49b7953b56b592b2f8ac3e04df738615211 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/Encoding.java 
> 204f5c45b6810a82bdb74fb2d27fe2def6fd568a 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java 
> 9d316088ffd51b2dce816e6cc18d8e2a4a23cfd5 
>   
> commons/src/main/resources/org/apache/aurora/common/net/http/handlers/thrift.st
>  0586653e1b22a96b0ce71e2fc979d6d3a71d1677 
>   commons/src/main/thrift/org/apache/aurora/common/thrift/endpoint.thrift 
> 87da2e13db900a4b06f50d89c9559ecfe202508e 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/EncodingTest.java 
> 16c017104c4696a985eefa23e1611300a7357259 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java 
> 6cf335dd451ca05bb511d524ff5590b219505cae 
>   src/main/java/org/apache/aurora/scheduler/app/ServiceGroupMonitor.java 
> a1329fd5010fadbf6bceeb2ae0476a6261830492 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
>  77f90eead2812e24d0d56e289bba7944075ee3bc 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
>  eba56bea89961fd8e6507eaf4180f700e4b030c4 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorSingletonService.java
>  2847c41b15b64b6a91bae295a9e5a96680b0c816 
>   src/main/java/org/apache/aurora/scheduler/discovery/Encoding.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java 
> bc0e2a86c0a8a354d95350a3d2f63514ee26da70 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 3ed256b30a0492d65560344c3fc038d8d93f2ec0 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/BaseCuratorDiscoveryTest.java
>  02c81838c52245aad3c582e0dea62f4ca3693379 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
>  e6b57ee7d0af234a60794e10c63c2670f67e6ccd 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorSingletonServiceTest.java
>  946a78e185962b1459ce0dc32c55a036d04a7e19 
>   src/test/java/org/apache/aurora/scheduler/discovery/EncodingTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> f3ae5a566b555b753230629897847c1bb5abc3b1 
>   src/test/java/org/apache/aurora/scheduler/http/LeaderRedirectTest.java 
> a7cc046d97d7b817a5496a6a7190142375b7a8a1 
> 
> 
> Diff: https://reviews.apache.org/r/63418/diff/2/
> 
> 
> Testing
> ---
> 
> e2e tests pass
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 63199: Refactor staticallyBannedOffers into a LRU cache

[Bill Farner]

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


Ship it!




LGTM, with the switch to `CacheBuilder` in `OfferSettings` as the finishing 
touch.  Nice patch!

- Bill Farner


On Oct. 30, 2017, 11:14 a.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63199/
> ---
> 
> (Updated Oct. 30, 2017, 11:14 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, 
> Stephan Erb, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Using the new `hold_offers_forever` option, it is possible for the 
> `staticallyBannedOffers` to grow very large in size as we never release 
> offers.
> 
> As an alternative to https://reviews.apache.org/r/63121/, I propose changing 
> `staticallyBannedOffers` into a LRU cache which expires entries after 
> `min_offer_hold_time` + `offer_hold_jitter_window` (referred to as 
> `maxOfferHoldTime`), while also taking an option for a maximum size for the 
> cache. I believe that this approach has a couple of benefits:
> 
> 1. The current behavior of `staticallyBannedOffers` is (kinda) preserved. 
> Entries will no longer be removed when the offer is used, but they will be 
> removed within `maxOfferHoldTime`. This means cluster operators will not have 
> to think about the new `offer_static_ban_cache_max_size` if they aren't 
> affected by the memory leak now.
> 2. Cluster operators that use Aurora as a single framework and hold offers 
> indefinitely can cap the size of the cache to avoid the memory leak.
> 3. Using an LRU cache greatly benefits quickly recurring crons and job 
> updates.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> e0ec793cad05674fb4b65246a6d153521b28b914 
>   
> src/main/java/org/apache/aurora/scheduler/config/validators/NotNegativeNumber.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 7011a4cc9eea827cdd54698aaed1a653774bce7f 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java 
> e060f2073dce4d2486d1ee2bfd873fe75167c6d0 
>   src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 
> e6b2c55e4f33f9a603157236766425edcaff10e7 
>   src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java 
> 244422c6b3ac6a2f7b4690cdc0f3440170b2567f 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> 3d38a5929a0255a980db30eeca0f059a2029f321 
> 
> 
> Diff: https://reviews.apache.org/r/63199/diff/5/
> 
> 
> Testing
> ---
> 
> Unit tests pass.
> Deployed on a scale test cluster and saw that a) `staticallyBannedOffers` 
> memory leak fixed with correct options and b) lowered assignment time for 
> quickly recurring crons and rescheduled jobs.
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>

Re: Review Request 63199: Refactor staticallyBannedOffers into a LRU cache

[Bill Farner]

> On Oct. 27, 2017, 3:03 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java
> > Lines 42-43 (patched)
> > 
> >
> > How about a `CacheBuilderSpec` to bundle these?
> 
> Jordan Ly wrote:
> I think that using a `CacheBuilderSpec` would cause some odd string 
> manipulation from our argument names to the ones `CacheBuilderSpec` uses.
> 
> I could just build the `CacheBuilder` in `OfferModule`/`OfferSettings` 
> and pass that into `OfferManager` instead. Which do you think would be 
> cleaner?

> I think that using a CacheBuilderSpec would cause some odd string 
> manipulation from our argument names to the ones CacheBuilderSpec uses

Please forgive me, i neglected to look closely enough at CacheBuilderSpec.  
Didn't realize it could only be created from a String.

> I could just build the CacheBuilder in OfferModule/OfferSettings and pass 
> that into OfferManager instead. Which do you think would be cleaner?

Let's go that route, that's what i should have suggested :-)


- Bill


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


On Oct. 30, 2017, 11:14 a.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63199/
> ---
> 
> (Updated Oct. 30, 2017, 11:14 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, 
> Stephan Erb, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Using the new `hold_offers_forever` option, it is possible for the 
> `staticallyBannedOffers` to grow very large in size as we never release 
> offers.
> 
> As an alternative to https://reviews.apache.org/r/63121/, I propose changing 
> `staticallyBannedOffers` into a LRU cache which expires entries after 
> `min_offer_hold_time` + `offer_hold_jitter_window` (referred to as 
> `maxOfferHoldTime`), while also taking an option for a maximum size for the 
> cache. I believe that this approach has a couple of benefits:
> 
> 1. The current behavior of `staticallyBannedOffers` is (kinda) preserved. 
> Entries will no longer be removed when the offer is used, but they will be 
> removed within `maxOfferHoldTime`. This means cluster operators will not have 
> to think about the new `offer_static_ban_cache_max_size` if they aren't 
> affected by the memory leak now.
> 2. Cluster operators that use Aurora as a single framework and hold offers 
> indefinitely can cap the size of the cache to avoid the memory leak.
> 3. Using an LRU cache greatly benefits quickly recurring crons and job 
> updates.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> e0ec793cad05674fb4b65246a6d153521b28b914 
>   
> src/main/java/org/apache/aurora/scheduler/config/validators/NotNegativeNumber.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 7011a4cc9eea827cdd54698aaed1a653774bce7f 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java 
> e060f2073dce4d2486d1ee2bfd873fe75167c6d0 
>   src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 
> e6b2c55e4f33f9a603157236766425edcaff10e7 
>   src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java 
> 244422c6b3ac6a2f7b4690cdc0f3440170b2567f 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> 3d38a5929a0255a980db30eeca0f059a2029f321 
> 
> 
> Diff: https://reviews.apache.org/r/63199/diff/5/
> 
> 
> Testing
> ---
> 
> Unit tests pass.
> Deployed on a scale test cluster and saw that a) `staticallyBannedOffers` 
> memory leak fixed with correct options and b) lowered assignment time for 
> quickly recurring crons and rescheduled jobs.
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>

Re: Review Request 63418: Remove endpoint.thrift, ServiceInstance is never serialized to thrift

[Bill Farner]

> On Oct. 30, 2017, 11:09 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java
> > Lines 45 (patched)
> > 
> >
> > Should we just hardcode ALIVE here? Aurora is not using anything else.
> > 
> > We could also add comments to the enum that all other state are 
> > deprecated and not used.

Thanks for the nudge, this crossed my mind as well while making the change.  
I've gone ahead and buried the field to effectively hard-code it.


- Bill


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


On Oct. 30, 2017, 9:36 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63418/
> ---
> 
> (Updated Oct. 30, 2017, 9:36 a.m.)
> 
> 
> Review request for Aurora, Jordan Ly and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This enables removal of some unnecessary complexity in the build (commons no
> longer needs thrift) and the unused Codec abstraction (we always encode in
> JSON).
> 
> 
> Diffs
> -
> 
>   build.gradle 1c1d3811c861dc6ff16bbd9ce3daefb5833f52df 
>   commons/src/main/java/org/apache/aurora/common/io/Codec.java 
> 94d1e3654767d8e5c0280595d0944c2fcb68ef90 
>   commons/src/main/java/org/apache/aurora/common/thrift/Util.java 
> 4ef7f49b7953b56b592b2f8ac3e04df738615211 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/Encoding.java 
> 204f5c45b6810a82bdb74fb2d27fe2def6fd568a 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java 
> 9d316088ffd51b2dce816e6cc18d8e2a4a23cfd5 
>   
> commons/src/main/resources/org/apache/aurora/common/net/http/handlers/thrift.st
>  0586653e1b22a96b0ce71e2fc979d6d3a71d1677 
>   commons/src/main/thrift/org/apache/aurora/common/thrift/endpoint.thrift 
> 87da2e13db900a4b06f50d89c9559ecfe202508e 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/EncodingTest.java 
> 16c017104c4696a985eefa23e1611300a7357259 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java 
> 6cf335dd451ca05bb511d524ff5590b219505cae 
>   src/main/java/org/apache/aurora/scheduler/app/ServiceGroupMonitor.java 
> a1329fd5010fadbf6bceeb2ae0476a6261830492 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
>  77f90eead2812e24d0d56e289bba7944075ee3bc 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
>  eba56bea89961fd8e6507eaf4180f700e4b030c4 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorSingletonService.java
>  2847c41b15b64b6a91bae295a9e5a96680b0c816 
>   src/main/java/org/apache/aurora/scheduler/discovery/Encoding.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java 
> bc0e2a86c0a8a354d95350a3d2f63514ee26da70 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 3ed256b30a0492d65560344c3fc038d8d93f2ec0 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/BaseCuratorDiscoveryTest.java
>  02c81838c52245aad3c582e0dea62f4ca3693379 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
>  e6b57ee7d0af234a60794e10c63c2670f67e6ccd 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorSingletonServiceTest.java
>  946a78e185962b1459ce0dc32c55a036d04a7e19 
>   src/test/java/org/apache/aurora/scheduler/discovery/EncodingTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> f3ae5a566b555b753230629897847c1bb5abc3b1 
>   src/test/java/org/apache/aurora/scheduler/http/LeaderRedirectTest.java 
> a7cc046d97d7b817a5496a6a7190142375b7a8a1 
> 
> 
> Diff: https://reviews.apache.org/r/63418/diff/1/
> 
> 
> Testing
> ---
> 
> e2e tests pass
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 63199: Refactor staticallyBannedOffers into a LRU cache

[Aurora ReviewBot]

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


Ship it!




Master (448e6d4) 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. 30, 2017, 6:14 p.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63199/
> ---
> 
> (Updated Oct. 30, 2017, 6:14 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, 
> Stephan Erb, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Using the new `hold_offers_forever` option, it is possible for the 
> `staticallyBannedOffers` to grow very large in size as we never release 
> offers.
> 
> As an alternative to https://reviews.apache.org/r/63121/, I propose changing 
> `staticallyBannedOffers` into a LRU cache which expires entries after 
> `min_offer_hold_time` + `offer_hold_jitter_window` (referred to as 
> `maxOfferHoldTime`), while also taking an option for a maximum size for the 
> cache. I believe that this approach has a couple of benefits:
> 
> 1. The current behavior of `staticallyBannedOffers` is (kinda) preserved. 
> Entries will no longer be removed when the offer is used, but they will be 
> removed within `maxOfferHoldTime`. This means cluster operators will not have 
> to think about the new `offer_static_ban_cache_max_size` if they aren't 
> affected by the memory leak now.
> 2. Cluster operators that use Aurora as a single framework and hold offers 
> indefinitely can cap the size of the cache to avoid the memory leak.
> 3. Using an LRU cache greatly benefits quickly recurring crons and job 
> updates.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> e0ec793cad05674fb4b65246a6d153521b28b914 
>   
> src/main/java/org/apache/aurora/scheduler/config/validators/NotNegativeNumber.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 7011a4cc9eea827cdd54698aaed1a653774bce7f 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java 
> e060f2073dce4d2486d1ee2bfd873fe75167c6d0 
>   src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 
> e6b2c55e4f33f9a603157236766425edcaff10e7 
>   src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java 
> 244422c6b3ac6a2f7b4690cdc0f3440170b2567f 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> 3d38a5929a0255a980db30eeca0f059a2029f321 
> 
> 
> Diff: https://reviews.apache.org/r/63199/diff/5/
> 
> 
> Testing
> ---
> 
> Unit tests pass.
> Deployed on a scale test cluster and saw that a) `staticallyBannedOffers` 
> memory leak fixed with correct options and b) lowered assignment time for 
> quickly recurring crons and rescheduled jobs.
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>

Re: Review Request 63418: Remove endpoint.thrift, ServiceInstance is never serialized to thrift

[Jordan Ly]

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


Ship it!




Ship It!

- Jordan Ly


On Oct. 30, 2017, 4:36 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63418/
> ---
> 
> (Updated Oct. 30, 2017, 4:36 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This enables removal of some unnecessary complexity in the build (commons no
> longer needs thrift) and the unused Codec abstraction (we always encode in
> JSON).
> 
> 
> Diffs
> -
> 
>   build.gradle 1c1d3811c861dc6ff16bbd9ce3daefb5833f52df 
>   commons/src/main/java/org/apache/aurora/common/io/Codec.java 
> 94d1e3654767d8e5c0280595d0944c2fcb68ef90 
>   commons/src/main/java/org/apache/aurora/common/thrift/Util.java 
> 4ef7f49b7953b56b592b2f8ac3e04df738615211 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/Encoding.java 
> 204f5c45b6810a82bdb74fb2d27fe2def6fd568a 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java 
> 9d316088ffd51b2dce816e6cc18d8e2a4a23cfd5 
>   
> commons/src/main/resources/org/apache/aurora/common/net/http/handlers/thrift.st
>  0586653e1b22a96b0ce71e2fc979d6d3a71d1677 
>   commons/src/main/thrift/org/apache/aurora/common/thrift/endpoint.thrift 
> 87da2e13db900a4b06f50d89c9559ecfe202508e 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/EncodingTest.java 
> 16c017104c4696a985eefa23e1611300a7357259 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java 
> 6cf335dd451ca05bb511d524ff5590b219505cae 
>   src/main/java/org/apache/aurora/scheduler/app/ServiceGroupMonitor.java 
> a1329fd5010fadbf6bceeb2ae0476a6261830492 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
>  77f90eead2812e24d0d56e289bba7944075ee3bc 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
>  eba56bea89961fd8e6507eaf4180f700e4b030c4 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorSingletonService.java
>  2847c41b15b64b6a91bae295a9e5a96680b0c816 
>   src/main/java/org/apache/aurora/scheduler/discovery/Encoding.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java 
> bc0e2a86c0a8a354d95350a3d2f63514ee26da70 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 3ed256b30a0492d65560344c3fc038d8d93f2ec0 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/BaseCuratorDiscoveryTest.java
>  02c81838c52245aad3c582e0dea62f4ca3693379 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
>  e6b57ee7d0af234a60794e10c63c2670f67e6ccd 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorSingletonServiceTest.java
>  946a78e185962b1459ce0dc32c55a036d04a7e19 
>   src/test/java/org/apache/aurora/scheduler/discovery/EncodingTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> f3ae5a566b555b753230629897847c1bb5abc3b1 
>   src/test/java/org/apache/aurora/scheduler/http/LeaderRedirectTest.java 
> a7cc046d97d7b817a5496a6a7190142375b7a8a1 
> 
> 
> Diff: https://reviews.apache.org/r/63418/diff/1/
> 
> 
> Testing
> ---
> 
> e2e tests pass
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 63406: Condense whitespace of navigation and breadcrumb, reduce impact of quota widget

[Stephan Erb]

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



The whitespace change looks good to me.

I have one general (orthogonal) question though:  Have you considered listing 
all tiers here, rather than 'Quota used' and 'Non-production'? At least the 
latter is always confusing and somewhat deprecated together with the production 
flag.

- Stephan Erb


On Oct. 30, 2017, 4:32 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63406/
> ---
> 
> (Updated Oct. 30, 2017, 4:32 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Condense whitespace (height reduction of ~50%) of navigation and breadcrumb 
> to better support smaller screens than mine. 
> 
> Reduce visual impact of quota widget on job list page. Tighten up items in 
> task list to fit more per page.
> 
> 
> Diffs
> -
> 
>   ui/src/main/js/pages/Jobs.js d3bef8c9e8d14fa5101b3103f102659cca7f4166 
>   ui/src/main/sass/components/_base.scss 
> fadd0d24539dcacc8bd2b7bedd3a5a03cffc78df 
>   ui/src/main/sass/components/_breadcrumb.scss 
> 67c0c98c2087ff5f926b746e570ecfac85e6bf5a 
>   ui/src/main/sass/components/_job-list-page.scss 
> 4783aa400d679eb302892a7bec7ae1382df9df8b 
>   ui/src/main/sass/components/_navigation.scss 
> 46cb54174e5e03b134bf3727c91209967af1028c 
>   ui/src/main/sass/components/_task-list.scss 
> 3c7221942353b592d36432bbe6a4c09ab7ce272f 
> 
> 
> Diff: https://reviews.apache.org/r/63406/diff/1/
> 
> 
> Testing
> ---
> 
> Cosmetic changes only. See screenshots.
> 
> 
> File Attachments
> 
> 
> whitespace-master.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/04261124-6e54-43cd-87fc-968f85869fda__whitespace-master.png
> whitespace-patch.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/6a929d59-c650-4844-bd1f-6eef80196111__whitespace-patch.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 63199: Refactor staticallyBannedOffers into a LRU cache

[Jordan Ly]

> On Oct. 27, 2017, 10:03 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java
> > Lines 321 (patched)
> > 
> >
> > `expireAfterWrite` doesn't result in LRU.  I think you mean 
> > `expireAfterAccess`.

LRU is maintained if the operator specifies a maximum size 
(https://github.com/google/guava/wiki/CachesExplained#size-based-eviction).

I use `expireAfterWrite` because offers will not be valid after 
`minOfferHoldTime`. I believe both strategies can exist simultaneously.


> On Oct. 27, 2017, 10:03 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java
> > Lines 42-43 (patched)
> > 
> >
> > How about a `CacheBuilderSpec` to bundle these?

I think that using a `CacheBuilderSpec` would cause some odd string 
manipulation from our argument names to the ones `CacheBuilderSpec` uses.

I could just build the `CacheBuilder` in `OfferModule`/`OfferSettings` and pass 
that into `OfferManager` instead. Which do you think would be cleaner?


> On Oct. 27, 2017, 10:03 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java
> > Lines 46-50 (patched)
> > 
> >
> > We've developed a 
> > [stutter](https://michaelwhatcott.com/go-code-that-stutters/) over time in 
> > this class.  Can you do some cleanup and s/offer// on parameters, fields, 
> > and methods?

Done.


> On Oct. 27, 2017, 10:03 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java
> > Lines 104 (patched)
> > 
> >
> > `0` is a valid size for guava's `CacheBuilder#maximumSize`:
> > > When size is zero, elements will be evicted immediately after being 
> > loaded into the cache. This can be useful in testing, or to disable caching 
> > temporarily without a code change.
> > 
> > Let's allow it, and introduce a `NotNegativeNumber` counterpart to 
> > `PositiveNumber`.

Added.


> On Oct. 27, 2017, 10:03 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java
> > Lines 109 (patched)
> > 
> >
> > s/Long/long/

Done.


> On Oct. 27, 2017, 10:03 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java
> > Line 178 (original), 188-189 (patched)
> > 
> >
> > Is this needed only because of the default `offerStaticBanCacheMaxSize 
> > = Long.MAX_VALUE`?  The double eviction strategy intuitively seems 
> > unnecessary.  I suggest a finite default for `offerStaticBanCacheMaxSize` 
> > (say, 1k) and no time expiry.

Spoke offline, but added a comment to explain the reasoning behind the dual 
eviction strategy.


> On Oct. 27, 2017, 10:03 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java
> > Lines 247-249 (original), 253-255 (patched)
> > 
> >
> > IMHO this test case is no longer interesting now that we have to punch 
> > through with `cleanupStaticBans()`.  The test now reads as "test that 
> > clearing a cache clears the cache".  I suggest removing the test case.

`cleanupWithStaticBans()` only ensures that the `size()` method returns the 
correct value by not counting evicted entries (i.e. the entry we just expired 
in the test will not be counted). I think this test is still useful as it 
ensures the eviction on expiration strategy works as intended. I added a 
comment to `cleanupWithStaticBans()` for clarity.


- Jordan


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


On Oct. 30, 2017, 6:14 p.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63199/
> ---
> 
> (Updated Oct. 30, 2017, 6:14 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, 
> Stephan Erb, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Using the new `hold_offers_forever` option, it is possible for the 
> `staticallyBannedOffers` to grow very large in size as we never release 
> offers.
> 
> As an alternative to https://reviews.apache.org/r/63121/, I propose changing 
> `staticallyBannedOffers` into a LRU cache which 

Re: Review Request 63199: Refactor staticallyBannedOffers into a LRU cache

[Jordan Ly]

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

(Updated Oct. 30, 2017, 6:14 p.m.)


Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan 
Erb, and Bill Farner.


Changes
---

Address feedback.


Repository: aurora


Description
---

Using the new `hold_offers_forever` option, it is possible for the 
`staticallyBannedOffers` to grow very large in size as we never release offers.

As an alternative to https://reviews.apache.org/r/63121/, I propose changing 
`staticallyBannedOffers` into a LRU cache which expires entries after 
`min_offer_hold_time` + `offer_hold_jitter_window` (referred to as 
`maxOfferHoldTime`), while also taking an option for a maximum size for the 
cache. I believe that this approach has a couple of benefits:

1. The current behavior of `staticallyBannedOffers` is (kinda) preserved. 
Entries will no longer be removed when the offer is used, but they will be 
removed within `maxOfferHoldTime`. This means cluster operators will not have 
to think about the new `offer_static_ban_cache_max_size` if they aren't 
affected by the memory leak now.
2. Cluster operators that use Aurora as a single framework and hold offers 
indefinitely can cap the size of the cache to avoid the memory leak.
3. Using an LRU cache greatly benefits quickly recurring crons and job updates.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
e0ec793cad05674fb4b65246a6d153521b28b914 
  
src/main/java/org/apache/aurora/scheduler/config/validators/NotNegativeNumber.java
 PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
7011a4cc9eea827cdd54698aaed1a653774bce7f 
  src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java 
e060f2073dce4d2486d1ee2bfd873fe75167c6d0 
  src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 
e6b2c55e4f33f9a603157236766425edcaff10e7 
  src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java 
244422c6b3ac6a2f7b4690cdc0f3440170b2567f 
  src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
3d38a5929a0255a980db30eeca0f059a2029f321 


Diff: https://reviews.apache.org/r/63199/diff/5/

Changes: https://reviews.apache.org/r/63199/diff/4-5/


Testing
---

Unit tests pass.
Deployed on a scale test cluster and saw that a) `staticallyBannedOffers` 
memory leak fixed with correct options and b) lowered assignment time for 
quickly recurring crons and rescheduled jobs.


Thanks,

Jordan Ly

Re: Review Request 63418: Remove endpoint.thrift, ServiceInstance is never serialized to thrift

[Stephan Erb]

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


Ship it!




LGTM.

One minor thing below that confused me when comming in contact with the Aurora 
code base for the first time.


src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java
Lines 45 (patched)


Should we just hardcode ALIVE here? Aurora is not using anything else.

We could also add comments to the enum that all other state are deprecated 
and not used.


- Stephan Erb


On Oct. 30, 2017, 5:36 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63418/
> ---
> 
> (Updated Oct. 30, 2017, 5:36 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This enables removal of some unnecessary complexity in the build (commons no
> longer needs thrift) and the unused Codec abstraction (we always encode in
> JSON).
> 
> 
> Diffs
> -
> 
>   build.gradle 1c1d3811c861dc6ff16bbd9ce3daefb5833f52df 
>   commons/src/main/java/org/apache/aurora/common/io/Codec.java 
> 94d1e3654767d8e5c0280595d0944c2fcb68ef90 
>   commons/src/main/java/org/apache/aurora/common/thrift/Util.java 
> 4ef7f49b7953b56b592b2f8ac3e04df738615211 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/Encoding.java 
> 204f5c45b6810a82bdb74fb2d27fe2def6fd568a 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java 
> 9d316088ffd51b2dce816e6cc18d8e2a4a23cfd5 
>   
> commons/src/main/resources/org/apache/aurora/common/net/http/handlers/thrift.st
>  0586653e1b22a96b0ce71e2fc979d6d3a71d1677 
>   commons/src/main/thrift/org/apache/aurora/common/thrift/endpoint.thrift 
> 87da2e13db900a4b06f50d89c9559ecfe202508e 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/EncodingTest.java 
> 16c017104c4696a985eefa23e1611300a7357259 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java 
> 6cf335dd451ca05bb511d524ff5590b219505cae 
>   src/main/java/org/apache/aurora/scheduler/app/ServiceGroupMonitor.java 
> a1329fd5010fadbf6bceeb2ae0476a6261830492 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
>  77f90eead2812e24d0d56e289bba7944075ee3bc 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
>  eba56bea89961fd8e6507eaf4180f700e4b030c4 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorSingletonService.java
>  2847c41b15b64b6a91bae295a9e5a96680b0c816 
>   src/main/java/org/apache/aurora/scheduler/discovery/Encoding.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java 
> bc0e2a86c0a8a354d95350a3d2f63514ee26da70 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 3ed256b30a0492d65560344c3fc038d8d93f2ec0 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/BaseCuratorDiscoveryTest.java
>  02c81838c52245aad3c582e0dea62f4ca3693379 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
>  e6b57ee7d0af234a60794e10c63c2670f67e6ccd 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorSingletonServiceTest.java
>  946a78e185962b1459ce0dc32c55a036d04a7e19 
>   src/test/java/org/apache/aurora/scheduler/discovery/EncodingTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> f3ae5a566b555b753230629897847c1bb5abc3b1 
>   src/test/java/org/apache/aurora/scheduler/http/LeaderRedirectTest.java 
> a7cc046d97d7b817a5496a6a7190142375b7a8a1 
> 
> 
> Diff: https://reviews.apache.org/r/63418/diff/1/
> 
> 
> Testing
> ---
> 
> e2e tests pass
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 63406: Condense whitespace of navigation and breadcrumb, reduce impact of quota widget

[Santhosh Kumar Shanmugham]

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


Ship it!




LGTM.

It does look a little weird with the space not being fully utilized. (Lack of 
symmetry and what not.) Not a blocker though.

- Santhosh Kumar Shanmugham


On Oct. 30, 2017, 8:32 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63406/
> ---
> 
> (Updated Oct. 30, 2017, 8:32 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Condense whitespace (height reduction of ~50%) of navigation and breadcrumb 
> to better support smaller screens than mine. 
> 
> Reduce visual impact of quota widget on job list page. Tighten up items in 
> task list to fit more per page.
> 
> 
> Diffs
> -
> 
>   ui/src/main/js/pages/Jobs.js d3bef8c9e8d14fa5101b3103f102659cca7f4166 
>   ui/src/main/sass/components/_base.scss 
> fadd0d24539dcacc8bd2b7bedd3a5a03cffc78df 
>   ui/src/main/sass/components/_breadcrumb.scss 
> 67c0c98c2087ff5f926b746e570ecfac85e6bf5a 
>   ui/src/main/sass/components/_job-list-page.scss 
> 4783aa400d679eb302892a7bec7ae1382df9df8b 
>   ui/src/main/sass/components/_navigation.scss 
> 46cb54174e5e03b134bf3727c91209967af1028c 
>   ui/src/main/sass/components/_task-list.scss 
> 3c7221942353b592d36432bbe6a4c09ab7ce272f 
> 
> 
> Diff: https://reviews.apache.org/r/63406/diff/1/
> 
> 
> Testing
> ---
> 
> Cosmetic changes only. See screenshots.
> 
> 
> File Attachments
> 
> 
> whitespace-master.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/04261124-6e54-43cd-87fc-968f85869fda__whitespace-master.png
> whitespace-patch.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/6a929d59-c650-4844-bd1f-6eef80196111__whitespace-patch.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 63418: Remove endpoint.thrift, ServiceInstance is never serialized to thrift

[Bill Farner]

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




src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java
Line 88 (original), 88 (patched)


I've removed some superfluous field validation here (and an accompanying 
test case).  These checks are redundant to what is performed in 
`Encoding#decode()`.


- Bill Farner


On Oct. 30, 2017, 9:36 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63418/
> ---
> 
> (Updated Oct. 30, 2017, 9:36 a.m.)
> 
> 
> Review request for Aurora, Jordan Ly and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This enables removal of some unnecessary complexity in the build (commons no
> longer needs thrift) and the unused Codec abstraction (we always encode in
> JSON).
> 
> 
> Diffs
> -
> 
>   build.gradle 1c1d3811c861dc6ff16bbd9ce3daefb5833f52df 
>   commons/src/main/java/org/apache/aurora/common/io/Codec.java 
> 94d1e3654767d8e5c0280595d0944c2fcb68ef90 
>   commons/src/main/java/org/apache/aurora/common/thrift/Util.java 
> 4ef7f49b7953b56b592b2f8ac3e04df738615211 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/Encoding.java 
> 204f5c45b6810a82bdb74fb2d27fe2def6fd568a 
>   commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java 
> 9d316088ffd51b2dce816e6cc18d8e2a4a23cfd5 
>   
> commons/src/main/resources/org/apache/aurora/common/net/http/handlers/thrift.st
>  0586653e1b22a96b0ce71e2fc979d6d3a71d1677 
>   commons/src/main/thrift/org/apache/aurora/common/thrift/endpoint.thrift 
> 87da2e13db900a4b06f50d89c9559ecfe202508e 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/EncodingTest.java 
> 16c017104c4696a985eefa23e1611300a7357259 
>   commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java 
> 6cf335dd451ca05bb511d524ff5590b219505cae 
>   src/main/java/org/apache/aurora/scheduler/app/ServiceGroupMonitor.java 
> a1329fd5010fadbf6bceeb2ae0476a6261830492 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
>  77f90eead2812e24d0d56e289bba7944075ee3bc 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
>  eba56bea89961fd8e6507eaf4180f700e4b030c4 
>   
> src/main/java/org/apache/aurora/scheduler/discovery/CuratorSingletonService.java
>  2847c41b15b64b6a91bae295a9e5a96680b0c816 
>   src/main/java/org/apache/aurora/scheduler/discovery/Encoding.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java 
> bc0e2a86c0a8a354d95350a3d2f63514ee26da70 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 3ed256b30a0492d65560344c3fc038d8d93f2ec0 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/BaseCuratorDiscoveryTest.java
>  02c81838c52245aad3c582e0dea62f4ca3693379 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
>  e6b57ee7d0af234a60794e10c63c2670f67e6ccd 
>   
> src/test/java/org/apache/aurora/scheduler/discovery/CuratorSingletonServiceTest.java
>  946a78e185962b1459ce0dc32c55a036d04a7e19 
>   src/test/java/org/apache/aurora/scheduler/discovery/EncodingTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> f3ae5a566b555b753230629897847c1bb5abc3b1 
>   src/test/java/org/apache/aurora/scheduler/http/LeaderRedirectTest.java 
> a7cc046d97d7b817a5496a6a7190142375b7a8a1 
> 
> 
> Diff: https://reviews.apache.org/r/63418/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 63418: Remove endpoint.thrift, ServiceInstance is never serialized to thrift

[Bill Farner]

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

Review request for Aurora, Jordan Ly and Stephan Erb.


Repository: aurora


Description
---

This enables removal of some unnecessary complexity in the build (commons no
longer needs thrift) and the unused Codec abstraction (we always encode in
JSON).


Diffs
-

  build.gradle 1c1d3811c861dc6ff16bbd9ce3daefb5833f52df 
  commons/src/main/java/org/apache/aurora/common/io/Codec.java 
94d1e3654767d8e5c0280595d0944c2fcb68ef90 
  commons/src/main/java/org/apache/aurora/common/thrift/Util.java 
4ef7f49b7953b56b592b2f8ac3e04df738615211 
  commons/src/main/java/org/apache/aurora/common/zookeeper/Encoding.java 
204f5c45b6810a82bdb74fb2d27fe2def6fd568a 
  commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java 
9d316088ffd51b2dce816e6cc18d8e2a4a23cfd5 
  
commons/src/main/resources/org/apache/aurora/common/net/http/handlers/thrift.st 
0586653e1b22a96b0ce71e2fc979d6d3a71d1677 
  commons/src/main/thrift/org/apache/aurora/common/thrift/endpoint.thrift 
87da2e13db900a4b06f50d89c9559ecfe202508e 
  commons/src/test/java/org/apache/aurora/common/zookeeper/EncodingTest.java 
16c017104c4696a985eefa23e1611300a7357259 
  commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java 
6cf335dd451ca05bb511d524ff5590b219505cae 
  src/main/java/org/apache/aurora/scheduler/app/ServiceGroupMonitor.java 
a1329fd5010fadbf6bceeb2ae0476a6261830492 
  
src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
 77f90eead2812e24d0d56e289bba7944075ee3bc 
  
src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
 eba56bea89961fd8e6507eaf4180f700e4b030c4 
  
src/main/java/org/apache/aurora/scheduler/discovery/CuratorSingletonService.java
 2847c41b15b64b6a91bae295a9e5a96680b0c816 
  src/main/java/org/apache/aurora/scheduler/discovery/Encoding.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/discovery/ServiceInstance.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java 
bc0e2a86c0a8a354d95350a3d2f63514ee26da70 
  src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
3ed256b30a0492d65560344c3fc038d8d93f2ec0 
  
src/test/java/org/apache/aurora/scheduler/discovery/BaseCuratorDiscoveryTest.java
 02c81838c52245aad3c582e0dea62f4ca3693379 
  
src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
 e6b57ee7d0af234a60794e10c63c2670f67e6ccd 
  
src/test/java/org/apache/aurora/scheduler/discovery/CuratorSingletonServiceTest.java
 946a78e185962b1459ce0dc32c55a036d04a7e19 
  src/test/java/org/apache/aurora/scheduler/discovery/EncodingTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
f3ae5a566b555b753230629897847c1bb5abc3b1 
  src/test/java/org/apache/aurora/scheduler/http/LeaderRedirectTest.java 
a7cc046d97d7b817a5496a6a7190142375b7a8a1 


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


Testing
---


Thanks,

Bill Farner

Re: Review Request 63406: Condense whitespace of navigation and breadcrumb, reduce impact of quota widget

[Aurora ReviewBot]

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


Ship it!




Master (3fe5d59) 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. 30, 2017, 3:32 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63406/
> ---
> 
> (Updated Oct. 30, 2017, 3:32 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Condense whitespace (height reduction of ~50%) of navigation and breadcrumb 
> to better support smaller screens than mine. 
> 
> Reduce visual impact of quota widget on job list page. Tighten up items in 
> task list to fit more per page.
> 
> 
> Diffs
> -
> 
>   ui/src/main/js/pages/Jobs.js d3bef8c9e8d14fa5101b3103f102659cca7f4166 
>   ui/src/main/sass/components/_base.scss 
> fadd0d24539dcacc8bd2b7bedd3a5a03cffc78df 
>   ui/src/main/sass/components/_breadcrumb.scss 
> 67c0c98c2087ff5f926b746e570ecfac85e6bf5a 
>   ui/src/main/sass/components/_job-list-page.scss 
> 4783aa400d679eb302892a7bec7ae1382df9df8b 
>   ui/src/main/sass/components/_navigation.scss 
> 46cb54174e5e03b134bf3727c91209967af1028c 
>   ui/src/main/sass/components/_task-list.scss 
> 3c7221942353b592d36432bbe6a4c09ab7ce272f 
> 
> 
> Diff: https://reviews.apache.org/r/63406/diff/1/
> 
> 
> Testing
> ---
> 
> Cosmetic changes only. See screenshots.
> 
> 
> File Attachments
> 
> 
> whitespace-master.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/04261124-6e54-43cd-87fc-968f85869fda__whitespace-master.png
> whitespace-patch.png
>   
> https://reviews.apache.org/media/uploaded/files/2017/10/30/6a929d59-c650-4844-bd1f-6eef80196111__whitespace-patch.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Review Request 63406: Condense whitespace of navigation and breadcrumb, reduce impact of quota widget

[David McLaughlin]

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

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


Repository: aurora


Description
---

Condense whitespace (height reduction of ~50%) of navigation and breadcrumb to 
better support smaller screens than mine. 

Reduce visual impact of quota widget on job list page. Tighten up items in task 
list to fit more per page.


Diffs
-

  ui/src/main/js/pages/Jobs.js d3bef8c9e8d14fa5101b3103f102659cca7f4166 
  ui/src/main/sass/components/_base.scss 
fadd0d24539dcacc8bd2b7bedd3a5a03cffc78df 
  ui/src/main/sass/components/_breadcrumb.scss 
67c0c98c2087ff5f926b746e570ecfac85e6bf5a 
  ui/src/main/sass/components/_job-list-page.scss 
4783aa400d679eb302892a7bec7ae1382df9df8b 
  ui/src/main/sass/components/_navigation.scss 
46cb54174e5e03b134bf3727c91209967af1028c 
  ui/src/main/sass/components/_task-list.scss 
3c7221942353b592d36432bbe6a4c09ab7ce272f 


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


Testing
---

Cosmetic changes only. See screenshots.


File Attachments


whitespace-master.png
  
https://reviews.apache.org/media/uploaded/files/2017/10/30/04261124-6e54-43cd-87fc-968f85869fda__whitespace-master.png
whitespace-patch.png
  
https://reviews.apache.org/media/uploaded/files/2017/10/30/6a929d59-c650-4844-bd1f-6eef80196111__whitespace-patch.png


Thanks,

David McLaughlin

Re: Review Request 63401: Add support for generating patch RCs from non-master branches

[Stephan Erb]

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


Ship it!




Ship It!

- Stephan Erb


On Oct. 30, 2017, 12:18 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63401/
> ---
> 
> (Updated Oct. 30, 2017, 12:18 a.m.)
> 
> 
> Review request for Aurora and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Turnsout 0.18.1 is our first patch release!  Our script needed some 
> adjustments to make such an RC possible.
> 
> 
> Diffs
> -
> 
>   build-support/release/release-candidate 
> 1a03d5e9c5a7920eea9c3362220612de9c5cc9c6 
> 
> 
> Diff: https://reviews.apache.org/r/63401/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>