Re: Review Request 38326: Adding ssh options into "aurora task" commands.

[Bill Farner]

> On Sept. 11, 2015, 4:36 p.m., Bill Farner wrote:
> > How would you feel about an env var instead of command line arg?  This 
> > seems like something people might put in their bash profile.
> 
> Maxim Khutornenko wrote:
> I don't really like relying on env variables in places where direct input 
> is accepted. This is too brittle as users may not realize we are reading 
> system envs. It's also harder to reproduce as simple copy/paste will no 
> longer work across envrionments.
> 
> Bill Farner wrote:
> ```
> users may not realize we are reading system envs
> ```
> 
> That's what docs and help text are for, right?
> 
> The real downside is that if a user _needs_ an SSH option set all the 
> time, they have to include it all the time rather than once in their shell 
> profile.
> 
> Curious what others think.
> 
> Maxim Khutornenko wrote:
> We don't rely on any envrionment variables anywhere in our client cli, do 
> we? I remember there was a long discussion about relying on the implicit 
> profile settings when we were brainstorming client v2 design and the outcome 
> was: it's too confusing and error prone. Users don't read docs every time or 
> always remember how their bash profile looks.
> 
> Bill Farner wrote:
> We do for specifying how to view job diffs:
> ```
> $ grep -R os.environ src/main/python/apache/aurora/client
> src/main/python/apache/aurora/client/cli/jobs.py:diff_program = 
> os.environ.get("DIFF_VIEWER", "diff")
> ```
> 
> Implicit is bad when it's risky or has consequences, i don't think this 
> is either.
> 
> Maxim Khutornenko wrote:
> The DIFF_VIEWER is more of a one-time-set-end-forget option, while ssh 
> options are much more dynamic and on-demand.
> 
> The explicit command-line syntax is more flexible as it supports both 
> user and automated/unattended (e.g.: CI) environments. It also does not have 
> any side-effects. So, I don't really think convenience is the factor we 
> should seriously consider here.
> 
> Maxim Khutornenko wrote:
> Here is another point against bash profile route. The most likely reason 
> users would use this feature is to apply "-v" option. This is not the default 
> you'd like to keep anywhere in your profile. It also creates an inconvenient 
> support pattern when an operator/on-call would suggest users modifying their 
> bash profile to get more details about the failing command. Mutating 
> customer's environment for the sake of troubleshooting does not sound right 
> to me.
> 
> Zameer Manji wrote:
> I'm with Maxim for this change. I think debugging a user's environment 
> would be pretty cumbersome and increases the burden for support. I think if 
> there are any SSH flags that need to be set all the time for a user (please 
> share an example if there is one) it can be done in the users .ssh/config 
> which details how to connect to certain hosts anyways.

Using .ssh/config is definitely more appropriate.  Thanks for hashing this out.


- Bill


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


On Sept. 11, 2015, 4:31 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38326/
> ---
> 
> (Updated Sept. 11, 2015, 4:31 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Bugs: AURORA-1491
> https://issues.apache.org/jira/browse/AURORA-1491
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding ssh options into "aurora task" commands.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/command_runner.py 
> c7238e274e53138187c2fe6fe5f14b7ae5f43ba2 
>   src/main/python/apache/aurora/client/cli/options.py 
> 41b13d6f0e1ed355ea8b958b875c20f065349465 
>   src/main/python/apache/aurora/client/cli/task.py 
> d1f2568ac0afdd95c65523fde41f0dd16670a7a8 
>   src/test/python/apache/aurora/client/cli/test_task.py 
> 3ad0b70a7d918055ffee34f593d108a28de6e9f9 
> 
> Diff: https://reviews.apache.org/r/38326/diff/
> 
> 
> Testing
> ---
> 
> In vagrant:
> ```
> vagrant@aurora:~$ aurora task run -v --ssh-user=vagrant --ssh-options='-v -k' 
> --executor-sandbox devcluster/www-data/prod/hello 'ls'
> DEBUG] Command=(['task', 'run', '-v', '--ssh-user=vagrant', '--ssh-options=-v 
> -k', '--executor-sandbox', 'devcluster/www-data/prod/hello', 'ls'])
> DEBUG] Using auth module: 
>  0x7f05d2b88250>
> DEBUG] Running command: ['ssh', '-n', '-q', '-v', '-k', 
> 'vagrant@192.168.33.7', u'cd 
> /var/lib/mesos/slaves/*/frameworks/*/executors/thermos-1442013544190-www-data-prod-hello-0-fe7fa2f1-e21b-4cc3-9107-0777da35537e/runs/latest;ls']
> 192.168.33.7:  

Re: Review Request 38390: Adding oversubscription summary.

[Maxim Khutornenko]

> On Sept. 15, 2015, 1:20 a.m., Bill Farner wrote:
> > docs/configuration-reference.md, lines 344-355
> > 
> >
> > Tiers seems like a significant enough topic to warrant its own page 
> > with some more context and better flow.  As it stands, it's really hard to 
> > understand what exactly a tier is and why i want or need to define them. 
> > 
> > Also, you might consider dropping the term 'best-effort' and stick to 
> > 'revocable' to avoid overloading the naming.

| Tiers seems like a significant enough topic to warrant its own page with some 
more context and better flow.  As it stands, it's really hard to understand 
what exactly a tier is and why i want or need to define them. 

Agree. I am hesitant to promote the `tier` concept though until we fully 
conceptualize it in AURORA-1443. As it stands now, it only supports `revocable` 
value and has zero meaning outside the revocable offer work. 

| Also, you might consider dropping the term 'best-effort' and stick to 
'revocable' to avoid overloading the naming.

Sure, works for me.


> On Sept. 15, 2015, 1:20 a.m., Bill Farner wrote:
> > docs/configuration-reference.md, line 332
> > 
> >
> > This could use a more objective statement about what the flag _is_ 
> > rather than that it intends to be.

Done.


> On Sept. 15, 2015, 1:20 a.m., Bill Farner wrote:
> > docs/deploying-aurora-scheduler.md, line 201
> > 
> >
> > Would it make sense to remove this flag, and instead enable revocable 
> > resources when there's at least one configured tier that uses them?

I don't think we want to tie these together. We should be able to start/stop 
receiving revocable resources independent of the revocable tier presence. 
Modifying a tier config file to stop receiving mesos revocable offers is very 
confusing and error prone.


- Maxim


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


On Sept. 15, 2015, 12:54 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38390/
> ---
> 
> (Updated Sept. 15, 2015, 12:54 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1441
> https://issues.apache.org/jira/browse/AURORA-1441
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding oversubscription summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
> 
> Diff: https://reviews.apache.org/r/38390/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/blob/oversubscription_docs/docs/
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 38390: Adding oversubscription summary.

[Maxim Khutornenko]

> On Sept. 15, 2015, 8:59 p.m., Stephan Erb wrote:
> > docs/configuration-reference.md, line 346
> > 
> >
> > How about attaching a date or version classifier here? Makes it much 
> > easier for a reader to understand if this is a valid concern or if the docu 
> > is just out of date (unfortunately that is rather common in many projects).

I'd avoid sprinkling a version over our docs to avoid additional maintenance 
burden. I.e. a version pointer can get stale very quickly when our release 
plans change.


- Maxim


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


On Sept. 15, 2015, 12:54 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38390/
> ---
> 
> (Updated Sept. 15, 2015, 12:54 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1441
> https://issues.apache.org/jira/browse/AURORA-1441
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding oversubscription summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
> 
> Diff: https://reviews.apache.org/r/38390/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/blob/oversubscription_docs/docs/
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 38390: Adding oversubscription summary.

[Maxim Khutornenko]

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

(Updated Sept. 16, 2015, 12:38 a.m.)


Review request for Aurora and Bill Farner.


Changes
---

Bill's comments.


Bugs: AURORA-1441
https://issues.apache.org/jira/browse/AURORA-1441


Repository: aurora


Description
---

Adding oversubscription summary.


Diffs (updated)
-

  docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
  docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 

Diff: https://reviews.apache.org/r/38390/diff/


Testing
---

Private remote: 
https://github.com/maxim111333/incubator-aurora/blob/oversubscription_docs/docs/


Thanks,

Maxim Khutornenko

Re: Review Request 38385: Documenting dedicated job & quota relationship.

[Stephan Erb]

> On Sept. 16, 2015, 2:51 a.m., Bill Farner wrote:
> > docs/configuration-tutorial.md, line 583
> > 
> >
> > Woah, this is a new doc to me...seems significantly redundant with 
> > configuration-reference.md.  All the more point to trim this down and point 
> > off to prose about preemption/quota.  Mind filing a ticket to make these 
> > two docs less redundant?

That ticket does already exist: https://issues.apache.org/jira/browse/AURORA-829


- Stephan


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


On Sept. 16, 2015, 2:14 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38385/
> ---
> 
> (Updated Sept. 16, 2015, 2:14 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1462
> https://issues.apache.org/jira/browse/AURORA-1462
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Documenting dedicated job & quota relationship.
> 
> 
> Diffs
> -
> 
>   docs/client-commands.md f91e5df0c19104a961aa9955478bf2d37a2aa7b6 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/configuration-tutorial.md d6e7c352e16964e19a41340cb03a016620c17f7d 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
>   docs/resource-isolation.md 7e8d88d09093d85c07c84bd3d6476fc89ff21c3b 
> 
> Diff: https://reviews.apache.org/r/38385/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/tree/quota_dedicated_docs/docs
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 38326: Adding ssh options into "aurora task" commands.

[Zameer Manji]

> On Sept. 11, 2015, 4:36 p.m., Bill Farner wrote:
> > How would you feel about an env var instead of command line arg?  This 
> > seems like something people might put in their bash profile.
> 
> Maxim Khutornenko wrote:
> I don't really like relying on env variables in places where direct input 
> is accepted. This is too brittle as users may not realize we are reading 
> system envs. It's also harder to reproduce as simple copy/paste will no 
> longer work across envrionments.
> 
> Bill Farner wrote:
> ```
> users may not realize we are reading system envs
> ```
> 
> That's what docs and help text are for, right?
> 
> The real downside is that if a user _needs_ an SSH option set all the 
> time, they have to include it all the time rather than once in their shell 
> profile.
> 
> Curious what others think.
> 
> Maxim Khutornenko wrote:
> We don't rely on any envrionment variables anywhere in our client cli, do 
> we? I remember there was a long discussion about relying on the implicit 
> profile settings when we were brainstorming client v2 design and the outcome 
> was: it's too confusing and error prone. Users don't read docs every time or 
> always remember how their bash profile looks.
> 
> Bill Farner wrote:
> We do for specifying how to view job diffs:
> ```
> $ grep -R os.environ src/main/python/apache/aurora/client
> src/main/python/apache/aurora/client/cli/jobs.py:diff_program = 
> os.environ.get("DIFF_VIEWER", "diff")
> ```
> 
> Implicit is bad when it's risky or has consequences, i don't think this 
> is either.
> 
> Maxim Khutornenko wrote:
> The DIFF_VIEWER is more of a one-time-set-end-forget option, while ssh 
> options are much more dynamic and on-demand.
> 
> The explicit command-line syntax is more flexible as it supports both 
> user and automated/unattended (e.g.: CI) environments. It also does not have 
> any side-effects. So, I don't really think convenience is the factor we 
> should seriously consider here.
> 
> Maxim Khutornenko wrote:
> Here is another point against bash profile route. The most likely reason 
> users would use this feature is to apply "-v" option. This is not the default 
> you'd like to keep anywhere in your profile. It also creates an inconvenient 
> support pattern when an operator/on-call would suggest users modifying their 
> bash profile to get more details about the failing command. Mutating 
> customer's environment for the sake of troubleshooting does not sound right 
> to me.

I'm with Maxim for this change. I think debugging a user's environment would be 
pretty cumbersome and increases the burden for support. I think if there are 
any SSH flags that need to be set all the time for a user (please share an 
example if there is one) it can be done in the users .ssh/config which details 
how to connect to certain hosts anyways.


- Zameer


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


On Sept. 11, 2015, 4:31 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38326/
> ---
> 
> (Updated Sept. 11, 2015, 4:31 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Bugs: AURORA-1491
> https://issues.apache.org/jira/browse/AURORA-1491
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding ssh options into "aurora task" commands.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/command_runner.py 
> c7238e274e53138187c2fe6fe5f14b7ae5f43ba2 
>   src/main/python/apache/aurora/client/cli/options.py 
> 41b13d6f0e1ed355ea8b958b875c20f065349465 
>   src/main/python/apache/aurora/client/cli/task.py 
> d1f2568ac0afdd95c65523fde41f0dd16670a7a8 
>   src/test/python/apache/aurora/client/cli/test_task.py 
> 3ad0b70a7d918055ffee34f593d108a28de6e9f9 
> 
> Diff: https://reviews.apache.org/r/38326/diff/
> 
> 
> Testing
> ---
> 
> In vagrant:
> ```
> vagrant@aurora:~$ aurora task run -v --ssh-user=vagrant --ssh-options='-v -k' 
> --executor-sandbox devcluster/www-data/prod/hello 'ls'
> DEBUG] Command=(['task', 'run', '-v', '--ssh-user=vagrant', '--ssh-options=-v 
> -k', '--executor-sandbox', 'devcluster/www-data/prod/hello', 'ls'])
> DEBUG] Using auth module: 
>  0x7f05d2b88250>
> DEBUG] Running command: ['ssh', '-n', '-q', '-v', '-k', 
> 'vagrant@192.168.33.7', u'cd 
> /var/lib/mesos/slaves/*/frameworks/*/executors/thermos-1442013544190-www-data-prod-hello-0-fe7fa2f1-e21b-4cc3-9107-0777da35537e/runs/latest;ls']
> 192.168.33.7:  OpenSSH_6.6.1, OpenSSL 1.0.1f 6 Jan 2014
> 192.168.33.7:  debug1: Reading configuration data /etc/ssh/ssh_config
> 

Re: Review Request 38332: Convert all of our servlet implementations to jax-rs endpoints.

[Bill Farner]

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

(Updated Sept. 15, 2015, 7:35 p.m.)


Review request for Aurora and Joshua Cohen.


Repository: aurora


Description
---

There are a few positive outcomes here:

* our HTTP serving code is much more uniform
* endpoints have less boilerplate code
* endpoints are easier to test
* ServletModule setup is simpler and uniform


Diffs (updated)
-

  buildSrc/src/main/groovy/org/apache/aurora/build/CoverageReportCheck.groovy 
b996d4597492a76db0f2571db4e6c1ae9b4bcf33 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/AbortHandler.java
 e97bd825752bf04cca391e2119b5e33c66a1cab5 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/AssetHandler.java
 7a44f07e8514f32637d1832273cb4d9081a14031 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/ContentionPrinter.java
 1f8c453c8e582da44849a8e922d974a0e054c638 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/HealthHandler.java
 cc5ad4d5e15eaff78832c033e77b4ac0a532f6b3 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/LogConfig.java 
5520fb6a83d37884f6cace995f3cc3313c3980c4 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/QuitHandler.java
 4ce3c971b8e8ca696d7d5ea4e7d348237b836513 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/StringTemplateServlet.java
 60e0abb7f7bac4c84ece5e007a4b3980fbd9a585 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/TextResponseHandler.java
 23068eb4b8d9f1d848b4d007e14e7d32f3189ee1 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/ThreadStackPrinter.java
 5dd88041faafc563c87f51807476aa142e4942d5 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/TimeSeriesDataSource.java
 e87fe2c2a2d2ed609273298eb57085bcb155c3b2 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsHandler.java
 bf04525115fa4045e792f2c45116e8d10addb24f 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsJsonHandler.java
 e97ec6085c52a155ff9978c5121a5f98a8f93593 
  
commons/src/test/java/org/apache/aurora/common/net/http/handlers/AssetHandlerTest.java
 740c42fedf668233b80f878728620a10ecf9b86f 
  
commons/src/test/java/org/apache/aurora/common/net/http/handlers/VarsHandlerTest.java
 34f62fb9db2d723f8ef171db7c7b485d8416d24f 
  config/legacy_untested_classes.txt 07fd5f1066a4d926072d91b9de170b9035fb3ea4 
  src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
c503dcceb417d1f73c30dc63cf5352470d4c761d 

Diff: https://reviews.apache.org/r/38332/diff/


Testing
---

manually clicked through everything i could think of in ./gradlew run and in 
vagrant
end-to-end tests


Thanks,

Bill Farner

Re: Review Request 38332: Convert all of our servlet implementations to jax-rs endpoints.

[Aurora ReviewBot]

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


Master (44e4726) 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 Sept. 16, 2015, 2:35 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38332/
> ---
> 
> (Updated Sept. 16, 2015, 2:35 a.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> There are a few positive outcomes here:
> 
> * our HTTP serving code is much more uniform
> * endpoints have less boilerplate code
> * endpoints are easier to test
> * ServletModule setup is simpler and uniform
> 
> 
> Diffs
> -
> 
>   buildSrc/src/main/groovy/org/apache/aurora/build/CoverageReportCheck.groovy 
> b996d4597492a76db0f2571db4e6c1ae9b4bcf33 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/AbortHandler.java
>  e97bd825752bf04cca391e2119b5e33c66a1cab5 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/AssetHandler.java
>  7a44f07e8514f32637d1832273cb4d9081a14031 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/ContentionPrinter.java
>  1f8c453c8e582da44849a8e922d974a0e054c638 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/HealthHandler.java
>  cc5ad4d5e15eaff78832c033e77b4ac0a532f6b3 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/LogConfig.java
>  5520fb6a83d37884f6cace995f3cc3313c3980c4 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/QuitHandler.java
>  4ce3c971b8e8ca696d7d5ea4e7d348237b836513 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/StringTemplateServlet.java
>  60e0abb7f7bac4c84ece5e007a4b3980fbd9a585 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/TextResponseHandler.java
>  23068eb4b8d9f1d848b4d007e14e7d32f3189ee1 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/ThreadStackPrinter.java
>  5dd88041faafc563c87f51807476aa142e4942d5 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/TimeSeriesDataSource.java
>  e87fe2c2a2d2ed609273298eb57085bcb155c3b2 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsHandler.java
>  bf04525115fa4045e792f2c45116e8d10addb24f 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsJsonHandler.java
>  e97ec6085c52a155ff9978c5121a5f98a8f93593 
>   
> commons/src/test/java/org/apache/aurora/common/net/http/handlers/AssetHandlerTest.java
>  740c42fedf668233b80f878728620a10ecf9b86f 
>   
> commons/src/test/java/org/apache/aurora/common/net/http/handlers/VarsHandlerTest.java
>  34f62fb9db2d723f8ef171db7c7b485d8416d24f 
>   config/legacy_untested_classes.txt 07fd5f1066a4d926072d91b9de170b9035fb3ea4 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> c503dcceb417d1f73c30dc63cf5352470d4c761d 
> 
> Diff: https://reviews.apache.org/r/38332/diff/
> 
> 
> Testing
> ---
> 
> manually clicked through everything i could think of in ./gradlew run and in 
> vagrant
> end-to-end tests
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 38385: Documenting dedicated job & quota relationship.

[Aurora ReviewBot]

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

Ship it!


Master (44e4726) 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 Sept. 16, 2015, 12:14 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38385/
> ---
> 
> (Updated Sept. 16, 2015, 12:14 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1462
> https://issues.apache.org/jira/browse/AURORA-1462
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Documenting dedicated job & quota relationship.
> 
> 
> Diffs
> -
> 
>   docs/client-commands.md f91e5df0c19104a961aa9955478bf2d37a2aa7b6 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/configuration-tutorial.md d6e7c352e16964e19a41340cb03a016620c17f7d 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
>   docs/resource-isolation.md 7e8d88d09093d85c07c84bd3d6476fc89ff21c3b 
> 
> Diff: https://reviews.apache.org/r/38385/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/tree/quota_dedicated_docs/docs
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 38332: Convert all of our servlet implementations to jax-rs endpoints.

[Bill Farner]

> On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote:
> > commons/src/main/java/org/apache/aurora/common/net/http/handlers/AbortHandler.java,
> >  lines 72-73
> > 
> >
> > Will this ever appear to the client? Presumably abortListener shuts 
> > down the http server. quitquitquit appears to get around this by forking a 
> > new thread for the listener.

Yeah, i thought through thist oo...it's all but certain to not show up.  I've 
changed this to own up and return nothing.


> On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote:
> > commons/src/main/java/org/apache/aurora/common/net/http/handlers/ContentionPrinter.java,
> >  line 83
> > 
> >
> > `String.join`

Done.


> On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote:
> > commons/src/main/java/org/apache/aurora/common/net/http/handlers/QuitHandler.java,
> >  lines 72-73
> > 
> >
> > same question - will this actually run?

Probably, but not guaranteed.  In practice, teardown is is indefinite and much 
less predictable than the abort.


> On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote:
> > commons/src/test/java/org/apache/aurora/common/net/http/handlers/VarsHandlerTest.java,
> >  line 77
> > 
> >
> > String.join

Done.


> On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote:
> > commons/src/main/java/org/apache/aurora/common/net/http/handlers/TimeSeriesDataSource.java,
> >  line 101
> > 
> >
> > isn't the `@Produces` annotation enough here? afaik we don't need to 
> > explicitly serialize and deserialize json in jax-rs due to the json provider

I'd like to stay away from altering the implementation - this would pull a 
thread into unit tests, so i've left a TODO to defer.


> On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote:
> > commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsJsonHandler.java,
> >  line 72
> > 
> >
> > same question - do we stilll need to explicitly serialize to json?

Ditto - not necessary, but useful to limit the scope of the diff.  TODO added.


> On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java, 
> > lines 203-212
> > 
> >
> > This feels like a DRY violation to me - consider adding
> > 
> > `public static final String PATH = "/api"`
> > 
> > etc to each resource and referencing them in both the `@Path` 
> > annotation and here.

It is indeed a DRY violation, but it's not a new one - the existing path specs 
have this problem.  There are some nuances to differences between the `@Path` 
value and the filter value here, which leads me to prefer tackling an improved 
pattern separately.  Also, please see my reply to jcohen above - ultimately i'd 
like to find a way out of this filtering business and let jax-rs annotations 
Just Work with servlets, which would also address the DRY violation.


- Bill


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


On Sept. 13, 2015, 11:06 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38332/
> ---
> 
> (Updated Sept. 13, 2015, 11:06 a.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> There are a few positive outcomes here:
> 
> * our HTTP serving code is much more uniform
> * endpoints have less boilerplate code
> * endpoints are easier to test
> * ServletModule setup is simpler and uniform
> 
> 
> Diffs
> -
> 
>   buildSrc/src/main/groovy/org/apache/aurora/build/CoverageReportCheck.groovy 
> b996d4597492a76db0f2571db4e6c1ae9b4bcf33 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/AbortHandler.java
>  e97bd825752bf04cca391e2119b5e33c66a1cab5 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/AssetHandler.java
>  7a44f07e8514f32637d1832273cb4d9081a14031 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/ContentionPrinter.java
>  1f8c453c8e582da44849a8e922d974a0e054c638 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/HealthHandler.java
>  cc5ad4d5e15eaff78832c033e77b4ac0a532f6b3 
>   
> 

Re: Review Request 38390: Adding oversubscription summary.

[Bill Farner]

> On Sept. 14, 2015, 6:20 p.m., Bill Farner wrote:
> > docs/configuration-reference.md, lines 344-355
> > 
> >
> > Tiers seems like a significant enough topic to warrant its own page 
> > with some more context and better flow.  As it stands, it's really hard to 
> > understand what exactly a tier is and why i want or need to define them. 
> > 
> > Also, you might consider dropping the term 'best-effort' and stick to 
> > 'revocable' to avoid overloading the naming.
> 
> Maxim Khutornenko wrote:
> | Tiers seems like a significant enough topic to warrant its own page 
> with some more context and better flow.  As it stands, it's really hard to 
> understand what exactly a tier is and why i want or need to define them. 
> 
> Agree. I am hesitant to promote the `tier` concept though until we fully 
> conceptualize it in AURORA-1443. As it stands now, it only supports 
> `revocable` value and has zero meaning outside the revocable offer work. 
> 
> | Also, you might consider dropping the term 'best-effort' and stick to 
> 'revocable' to avoid overloading the naming.
> 
> Sure, works for me.

The intention is not to promote it (disclaimers are fine), but rather to paint 
the picture and not leave the reader hanging.  Doesn't have to be a lot of 
content, but some indication of how it fits in and the development status would 
go a long way.


> On Sept. 14, 2015, 6:20 p.m., Bill Farner wrote:
> > docs/deploying-aurora-scheduler.md, line 201
> > 
> >
> > Would it make sense to remove this flag, and instead enable revocable 
> > resources when there's at least one configured tier that uses them?
> 
> Maxim Khutornenko wrote:
> I don't think we want to tie these together. We should be able to 
> start/stop receiving revocable resources independent of the revocable tier 
> presence. Modifying a tier config file to stop receiving mesos revocable 
> offers is very confusing and error prone.

I buy this for the state today, but it seems like this should change if we ever 
make explicit tier configuration required.  The current situation is 
error-prone since the cluster operator needs to be sure to flip two knobs to 
really 'enable' the feature.


- Bill


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


On Sept. 15, 2015, 5:38 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38390/
> ---
> 
> (Updated Sept. 15, 2015, 5:38 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1441
> https://issues.apache.org/jira/browse/AURORA-1441
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding oversubscription summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
> 
> Diff: https://reviews.apache.org/r/38390/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/blob/oversubscription_docs/docs/
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 38280: Restore build properties within Scheduler vars endpoint and snapshots

[Bill Farner]

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


This patch is looking good to me, but i won't have time to give it a fair look 
as i head out of town.  I'd like to tap out so you don't have to wait 3 weeks 
to land it :-)

- Bill Farner


On Sept. 14, 2015, 11:46 a.m., Joe Smith wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38280/
> ---
> 
> (Updated Sept. 14, 2015, 11:46 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1473
> https://issues.apache.org/jira/browse/AURORA-1473
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Restore build properties within Scheduler vars endpoint and snapshots
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 670ba0850987308370e3c766048ad6ba246d9e29 
>   build-support/generate-build-properties PRE-CREATION 
>   build.gradle 9c78aff101793b25e4c1196c170eaf282f73a9bf 
>   
> commons/src/main/java/org/apache/aurora/common/stats/TimeSeriesRepositoryImpl.java
>  c314a0d51e9377c7fe4371a05d7a9375a47a7bf5 
>   commons/src/main/java/org/apache/aurora/common/util/BuildInfo.java 
> PRE-CREATION 
>   
> commons/src/main/java/org/apache/aurora/common/util/testing/FakeBuildInfo.java
>  PRE-CREATION 
>   
> commons/src/test/java/org/apache/aurora/common/stats/TimeSeriesRepositoryImplTest.java
>  89c134315dfdb4c55447032171ec3be8ec73823d 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  d76596c7422786e58b5a8aa79f324911cfd29b25 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 50838de8708d78fc0bd7ee672b7c7ba02dfcd505 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> cc9c066556385c073962903691c037b0c07cc94c 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  6e032a6cef86e5f408bfc0d853a52c7f898d0db8 
> 
> Diff: https://reviews.apache.org/r/38280/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Joe Smith
> 
>

Re: Review Request 38389: AURORA-1485: TaskConfig default container value breaks client library generation for GO

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On Sept. 15, 2015, 12:56 p.m., Jake Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38389/
> ---
> 
> (Updated Sept. 15, 2015, 12:56 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-1485
> https://issues.apache.org/jira/browse/AURORA-1485
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When generating the client api library for go the default containerizer used 
> as a hack for AURORA-1185 breaks and causes the client lib to not be 
> generated correctly. The default value for the optional field `container = { 
> "mesos": {} }` is not necessary as it is defaulted in java code on the api 
> side.
> 
> Tracking this down it looks like the default value was a last minute addition 
> based on some review comments and is not necessary or used anywhere.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 24a2ca8cc9a236228fdd8aa61c877319a803c9a6 
> 
> Diff: https://reviews.apache.org/r/38389/diff/
> 
> 
> Testing
> ---
> 
> Passes all java/py tests in ./build-support/jenkins/build.sh
> Passes all e2e tests in ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Jake Farrell
> 
>

Re: Review Request 38385: Documenting dedicated job & quota relationship.

[Bill Farner]

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


Overall content LGTM.  Mostly nits about organization, and a request for a new 
section to raise the preemption topic to a more prominent location.


docs/client-commands.md (line 335)


Please linkify `dedicated`, that would go a long way to point towards 
context.  A quick skim turnedt his up as a potential target: 
https://github.com/apache/aurora/blob/master/docs/deploying-aurora-scheduler.md#dedicated-attribute



docs/configuration-reference.md (line 327)


Not sure why we had the 'warning' in the first place, but i think it should 
be removed.  There isn't really any harm that warrants that tone.  Second - the 
note about preemption would be _great_ in a separate mini-section near 
'Resource Quota'.  With that, you can trim this down quite a bit:

> Indicated whether this is a production job that will be preferred for 
preemption [link].

Then the new preemption section can draw the connection to quota.



docs/configuration-tutorial.md (line 583)


Woah, this is a new doc to me...seems significantly redundant with 
configuration-reference.md.  All the more point to trim this down and point off 
to prose about preemption/quota.  Mind filing a ticket to make these two docs 
less redundant?



docs/deploying-aurora-scheduler.md (line 212)


I think it's fine to omit this and let the 'Resource Quota' section stand 
on its own.  Without a pointer to context, this is difficult to piece together. 
 Perhaps this:

> See the section about resource quotas [link] to learn how quotas apply to 
dedicated jobs.



docs/resource-isolation.md (line 150)


This doc is otherewise about machine-level resource isolation, so it seems 
like an odd match here.  It seems to align well with content in this page: 
docs/deploying-aurora-scheduler.md.


- Bill Farner


On Sept. 15, 2015, 5:14 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38385/
> ---
> 
> (Updated Sept. 15, 2015, 5:14 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1462
> https://issues.apache.org/jira/browse/AURORA-1462
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Documenting dedicated job & quota relationship.
> 
> 
> Diffs
> -
> 
>   docs/client-commands.md f91e5df0c19104a961aa9955478bf2d37a2aa7b6 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/configuration-tutorial.md d6e7c352e16964e19a41340cb03a016620c17f7d 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
>   docs/resource-isolation.md 7e8d88d09093d85c07c84bd3d6476fc89ff21c3b 
> 
> Diff: https://reviews.apache.org/r/38385/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/tree/quota_dedicated_docs/docs
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 38332: Convert all of our servlet implementations to jax-rs endpoints.

[Bill Farner]

> On Sept. 14, 2015, 9:19 a.m., Joshua Cohen wrote:
> > commons/src/main/java/org/apache/aurora/common/net/http/handlers/HealthHandler.java,
> >  line 78
> > 
> >
> > nit, not even related to your change, but this can just be `return 
> > healthChecker.get() ? IS_HEALTHY : IS_NOT_HEALTHY;` right?

There's a subtle difference - the Supplier could return null, which would be 
handled with this patch and result in an NPE in your example.  In practice, we 
don't do that, so i will make the proposed change.


> On Sept. 14, 2015, 9:19 a.m., Joshua Cohen wrote:
> > commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsJsonHandler.java,
> >  line 51
> > 
> >
> > Would probably make sense to combine this with VarsHandler and just 
> > have a separate method that handles `MediaType.APPLICATION_JSON`. Ok if you 
> > want to punt for a later change though.

Yeah, left a TODO - i'd like to save cleanup for later.


> On Sept. 14, 2015, 9:19 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java, 
> > lines 285-304
> > 
> >
> > Thoughts on making `JAX_RS_ENDPOINTS` above `Map` rather 
> > than a set? then we could map the values through `bind` and reduce this 
> > boilerplate a bit?

This has to be `Multimap` to work with your other suggestion 
of multiple endpoints from one class.

What i would _really_ like to do is figure out a way so we don't have to 
explicitly call all these out and filter them (servlets and jax-rs don't really 
play well together AFAICT).  No luck on cracking that nut, though, would love 
help.


- Bill


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


On Sept. 13, 2015, 11:06 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38332/
> ---
> 
> (Updated Sept. 13, 2015, 11:06 a.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> There are a few positive outcomes here:
> 
> * our HTTP serving code is much more uniform
> * endpoints have less boilerplate code
> * endpoints are easier to test
> * ServletModule setup is simpler and uniform
> 
> 
> Diffs
> -
> 
>   buildSrc/src/main/groovy/org/apache/aurora/build/CoverageReportCheck.groovy 
> b996d4597492a76db0f2571db4e6c1ae9b4bcf33 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/AbortHandler.java
>  e97bd825752bf04cca391e2119b5e33c66a1cab5 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/AssetHandler.java
>  7a44f07e8514f32637d1832273cb4d9081a14031 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/ContentionPrinter.java
>  1f8c453c8e582da44849a8e922d974a0e054c638 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/HealthHandler.java
>  cc5ad4d5e15eaff78832c033e77b4ac0a532f6b3 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/LogConfig.java
>  5520fb6a83d37884f6cace995f3cc3313c3980c4 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/QuitHandler.java
>  4ce3c971b8e8ca696d7d5ea4e7d348237b836513 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/StringTemplateServlet.java
>  60e0abb7f7bac4c84ece5e007a4b3980fbd9a585 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/TextResponseHandler.java
>  23068eb4b8d9f1d848b4d007e14e7d32f3189ee1 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/ThreadStackPrinter.java
>  5dd88041faafc563c87f51807476aa142e4942d5 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/TimeSeriesDataSource.java
>  e87fe2c2a2d2ed609273298eb57085bcb155c3b2 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsHandler.java
>  bf04525115fa4045e792f2c45116e8d10addb24f 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsJsonHandler.java
>  e97ec6085c52a155ff9978c5121a5f98a8f93593 
>   
> commons/src/test/java/org/apache/aurora/common/net/http/handlers/AssetHandlerTest.java
>  740c42fedf668233b80f878728620a10ecf9b86f 
>   
> commons/src/test/java/org/apache/aurora/common/net/http/handlers/VarsHandlerTest.java
>  34f62fb9db2d723f8ef171db7c7b485d8416d24f 
>   config/legacy_untested_classes.txt 07fd5f1066a4d926072d91b9de170b9035fb3ea4 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 

Re: Review Request 38385: Documenting dedicated job & quota relationship.

[Maxim Khutornenko]

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

(Updated Sept. 16, 2015, 12:14 a.m.)


Review request for Aurora and Bill Farner.


Changes
---

Stephan's comments.


Bugs: AURORA-1462
https://issues.apache.org/jira/browse/AURORA-1462


Repository: aurora


Description
---

Documenting dedicated job & quota relationship.


Diffs (updated)
-

  docs/client-commands.md f91e5df0c19104a961aa9955478bf2d37a2aa7b6 
  docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
  docs/configuration-tutorial.md d6e7c352e16964e19a41340cb03a016620c17f7d 
  docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
  docs/resource-isolation.md 7e8d88d09093d85c07c84bd3d6476fc89ff21c3b 

Diff: https://reviews.apache.org/r/38385/diff/


Testing
---

Private remote: 
https://github.com/maxim111333/incubator-aurora/tree/quota_dedicated_docs/docs


Thanks,

Maxim Khutornenko

Re: Review Request 38326: Adding ssh options into "aurora task" commands.

[Bill Farner]

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

Ship it!



src/main/python/apache/aurora/client/api/command_runner.py (line 93)


This patch uses mutable lists in several places, which i feel is a 
divergence from general prefernce for immutability.  I don't think it hinders 
readability at all to assign these lists once and not mutate them.


- Bill Farner


On Sept. 11, 2015, 4:31 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38326/
> ---
> 
> (Updated Sept. 11, 2015, 4:31 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Bugs: AURORA-1491
> https://issues.apache.org/jira/browse/AURORA-1491
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding ssh options into "aurora task" commands.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/command_runner.py 
> c7238e274e53138187c2fe6fe5f14b7ae5f43ba2 
>   src/main/python/apache/aurora/client/cli/options.py 
> 41b13d6f0e1ed355ea8b958b875c20f065349465 
>   src/main/python/apache/aurora/client/cli/task.py 
> d1f2568ac0afdd95c65523fde41f0dd16670a7a8 
>   src/test/python/apache/aurora/client/cli/test_task.py 
> 3ad0b70a7d918055ffee34f593d108a28de6e9f9 
> 
> Diff: https://reviews.apache.org/r/38326/diff/
> 
> 
> Testing
> ---
> 
> In vagrant:
> ```
> vagrant@aurora:~$ aurora task run -v --ssh-user=vagrant --ssh-options='-v -k' 
> --executor-sandbox devcluster/www-data/prod/hello 'ls'
> DEBUG] Command=(['task', 'run', '-v', '--ssh-user=vagrant', '--ssh-options=-v 
> -k', '--executor-sandbox', 'devcluster/www-data/prod/hello', 'ls'])
> DEBUG] Using auth module: 
>  0x7f05d2b88250>
> DEBUG] Running command: ['ssh', '-n', '-q', '-v', '-k', 
> 'vagrant@192.168.33.7', u'cd 
> /var/lib/mesos/slaves/*/frameworks/*/executors/thermos-1442013544190-www-data-prod-hello-0-fe7fa2f1-e21b-4cc3-9107-0777da35537e/runs/latest;ls']
> 192.168.33.7:  OpenSSH_6.6.1, OpenSSL 1.0.1f 6 Jan 2014
> 192.168.33.7:  debug1: Reading configuration data /etc/ssh/ssh_config
> 192.168.33.7:  debug1: /etc/ssh/ssh_config line 19: Applying options for *
> 192.168.33.7:  debug1: /etc/ssh/ssh_config line 57: Applying options for *
> 192.168.33.7:  debug1: Connecting to 192.168.33.7 [192.168.33.7] port 22.
> 192.168.33.7:  debug1: Connection established.
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_rsa type 1
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_rsa-cert type -1
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_dsa type -1
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_dsa-cert type -1
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_ecdsa type -1
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_ecdsa-cert type -1
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_ed25519 type -1
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_ed25519-cert type 
> -1
> 192.168.33.7:  debug1: Enabling compatibility mode for protocol 2.0
> 192.168.33.7:  debug1: Local version string SSH-2.0-OpenSSH_6.6.1p1 
> Ubuntu-2ubuntu2
> 192.168.33.7:  debug1: Remote protocol version 2.0, remote software version 
> OpenSSH_6.6.1p1 Ubuntu-2ubuntu2
> 192.168.33.7:  debug1: match: OpenSSH_6.6.1p1 Ubuntu-2ubuntu2 pat 
> OpenSSH_6.6.1* compat 0x0400
> 192.168.33.7:  debug1: SSH2_MSG_KEXINIT sent
> 192.168.33.7:  debug1: SSH2_MSG_KEXINIT received
> 192.168.33.7:  debug1: kex: server->client aes128-ctr 
> hmac-md5-...@openssh.com none
> 192.168.33.7:  debug1: kex: client->server aes128-ctr 
> hmac-md5-...@openssh.com none
> 192.168.33.7:  debug1: sending SSH2_MSG_KEX_ECDH_INIT
> 192.168.33.7:  debug1: expecting SSH2_MSG_KEX_ECDH_REPLY
> 192.168.33.7:  debug1: Server host key: ECDSA 
> 46:15:09:58:38:39:76:02:68:e1:c2:43:1f:70:bf:6e
> 192.168.33.7:  Warning: Permanently added '192.168.33.7' (ECDSA) to the list 
> of known hosts.
> 192.168.33.7:  debug1: ssh_ecdsa_verify: signature correct
> 192.168.33.7:  debug1: SSH2_MSG_NEWKEYS sent
> 192.168.33.7:  debug1: expecting SSH2_MSG_NEWKEYS
> 192.168.33.7:  debug1: SSH2_MSG_NEWKEYS received
> 192.168.33.7:  debug1: Roaming not allowed by server
> 192.168.33.7:  debug1: SSH2_MSG_SERVICE_REQUEST sent
> 192.168.33.7:  debug1: SSH2_MSG_SERVICE_ACCEPT received
> 192.168.33.7:  debug1: Authentications that can continue: publickey,password
> 192.168.33.7:  debug1: Next authentication method: publickey
> 192.168.33.7:  debug1: Offering RSA public key: /home/vagrant/.ssh/id_rsa
> 192.168.33.7:  debug1: Server accepts key: pkalg ssh-rsa blen 279
> 192.168.33.7:  debug1: key_parse_private2: 

Re: Review Request 38390: Adding oversubscription summary.

[Aurora ReviewBot]

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

Ship it!


Master (44e4726) 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 Sept. 16, 2015, 12:38 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38390/
> ---
> 
> (Updated Sept. 16, 2015, 12:38 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1441
> https://issues.apache.org/jira/browse/AURORA-1441
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding oversubscription summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
> 
> Diff: https://reviews.apache.org/r/38390/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/blob/oversubscription_docs/docs/
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 38326: Adding ssh options into "aurora task" commands.

[Maxim Khutornenko]

> On Sept. 11, 2015, 11:36 p.m., Bill Farner wrote:
> > How would you feel about an env var instead of command line arg?  This 
> > seems like something people might put in their bash profile.
> 
> Maxim Khutornenko wrote:
> I don't really like relying on env variables in places where direct input 
> is accepted. This is too brittle as users may not realize we are reading 
> system envs. It's also harder to reproduce as simple copy/paste will no 
> longer work across envrionments.
> 
> Bill Farner wrote:
> ```
> users may not realize we are reading system envs
> ```
> 
> That's what docs and help text are for, right?
> 
> The real downside is that if a user _needs_ an SSH option set all the 
> time, they have to include it all the time rather than once in their shell 
> profile.
> 
> Curious what others think.
> 
> Maxim Khutornenko wrote:
> We don't rely on any envrionment variables anywhere in our client cli, do 
> we? I remember there was a long discussion about relying on the implicit 
> profile settings when we were brainstorming client v2 design and the outcome 
> was: it's too confusing and error prone. Users don't read docs every time or 
> always remember how their bash profile looks.
> 
> Bill Farner wrote:
> We do for specifying how to view job diffs:
> ```
> $ grep -R os.environ src/main/python/apache/aurora/client
> src/main/python/apache/aurora/client/cli/jobs.py:diff_program = 
> os.environ.get("DIFF_VIEWER", "diff")
> ```
> 
> Implicit is bad when it's risky or has consequences, i don't think this 
> is either.
> 
> Maxim Khutornenko wrote:
> The DIFF_VIEWER is more of a one-time-set-end-forget option, while ssh 
> options are much more dynamic and on-demand.
> 
> The explicit command-line syntax is more flexible as it supports both 
> user and automated/unattended (e.g.: CI) environments. It also does not have 
> any side-effects. So, I don't really think convenience is the factor we 
> should seriously consider here.

Here is another point against bash profile route. The most likely reason users 
would use this feature is to apply "-v" option. This is not the default you'd 
like to keep anywhere in your profile. It also creates an inconvenient support 
pattern when an operator/on-call would suggest users modifying their bash 
profile to get more details about the failing command. Mutating customer's 
environment for the sake of troubleshooting does not sound right to me.


- Maxim


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


On Sept. 11, 2015, 11:31 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38326/
> ---
> 
> (Updated Sept. 11, 2015, 11:31 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Bugs: AURORA-1491
> https://issues.apache.org/jira/browse/AURORA-1491
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding ssh options into "aurora task" commands.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/command_runner.py 
> c7238e274e53138187c2fe6fe5f14b7ae5f43ba2 
>   src/main/python/apache/aurora/client/cli/options.py 
> 41b13d6f0e1ed355ea8b958b875c20f065349465 
>   src/main/python/apache/aurora/client/cli/task.py 
> d1f2568ac0afdd95c65523fde41f0dd16670a7a8 
>   src/test/python/apache/aurora/client/cli/test_task.py 
> 3ad0b70a7d918055ffee34f593d108a28de6e9f9 
> 
> Diff: https://reviews.apache.org/r/38326/diff/
> 
> 
> Testing
> ---
> 
> In vagrant:
> ```
> vagrant@aurora:~$ aurora task run -v --ssh-user=vagrant --ssh-options='-v -k' 
> --executor-sandbox devcluster/www-data/prod/hello 'ls'
> DEBUG] Command=(['task', 'run', '-v', '--ssh-user=vagrant', '--ssh-options=-v 
> -k', '--executor-sandbox', 'devcluster/www-data/prod/hello', 'ls'])
> DEBUG] Using auth module: 
>  0x7f05d2b88250>
> DEBUG] Running command: ['ssh', '-n', '-q', '-v', '-k', 
> 'vagrant@192.168.33.7', u'cd 
> /var/lib/mesos/slaves/*/frameworks/*/executors/thermos-1442013544190-www-data-prod-hello-0-fe7fa2f1-e21b-4cc3-9107-0777da35537e/runs/latest;ls']
> 192.168.33.7:  OpenSSH_6.6.1, OpenSSL 1.0.1f 6 Jan 2014
> 192.168.33.7:  debug1: Reading configuration data /etc/ssh/ssh_config
> 192.168.33.7:  debug1: /etc/ssh/ssh_config line 19: Applying options for *
> 192.168.33.7:  debug1: /etc/ssh/ssh_config line 57: Applying options for *
> 192.168.33.7:  debug1: Connecting to 192.168.33.7 [192.168.33.7] port 22.
> 192.168.33.7:  debug1: Connection established.
> 192.168.33.7:  debug1: identity file /home/vagrant/.ssh/id_rsa type 1
> 192.168.33.7:  debug1: identity file 

Re: Review Request 38389: AURORA-1485: TaskConfig default container value breaks client library generation for GO

[Aurora ReviewBot]

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

Ship it!


Master (44e4726) 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 Sept. 15, 2015, 12:41 a.m., Jake Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38389/
> ---
> 
> (Updated Sept. 15, 2015, 12:41 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-1485
> https://issues.apache.org/jira/browse/AURORA-1485
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When generating the client api library for go the default containerizer used 
> as a hack for AURORA-1185 breaks and causes the client lib to not be 
> generated correctly. The default value for the optional field `container = { 
> "mesos": {} }` is not necessary as it is defaulted in java code on the api 
> side.
> 
> Tracking this down it looks like the default value was a last minute addition 
> based on some review comments and is not necessary or used anywhere.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 24a2ca8cc9a236228fdd8aa61c877319a803c9a6 
>   src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzTestUtil.java 
> 13cb73d0e314c043aed9ab2aacea1908c069a297 
>   src/test/python/apache/aurora/client/cli/test_status.py 
> a75f15e99af97acb0af7e112e28c693a77f9eb24 
>   src/test/python/apache/aurora/client/cli/util.py 
> b03148b4edaa334620a72bee1937c8c16e19f23e 
>   src/test/python/apache/aurora/executor/test_thermos_executor.py 
> d24c61162e11bb4afef2902ebb031478fe3ed247 
> 
> Diff: https://reviews.apache.org/r/38389/diff/
> 
> 
> Testing
> ---
> 
> Passes all java/py tests in ./build-support/jenkins/build.sh
> Passes all e2e tests in ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Jake Farrell
> 
>

Re: Review Request 38390: Adding oversubscription summary.

[Aurora ReviewBot]

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

Ship it!


Master (44e4726) 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 Sept. 15, 2015, 12:54 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38390/
> ---
> 
> (Updated Sept. 15, 2015, 12:54 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1441
> https://issues.apache.org/jira/browse/AURORA-1441
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding oversubscription summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
> 
> Diff: https://reviews.apache.org/r/38390/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/blob/oversubscription_docs/docs/
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 38389: AURORA-1485: TaskConfig default container value breaks client library generation for GO

[Aurora ReviewBot]

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

Ship it!


Master (44e4726) 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 Sept. 15, 2015, 7:56 p.m., Jake Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38389/
> ---
> 
> (Updated Sept. 15, 2015, 7:56 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-1485
> https://issues.apache.org/jira/browse/AURORA-1485
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When generating the client api library for go the default containerizer used 
> as a hack for AURORA-1185 breaks and causes the client lib to not be 
> generated correctly. The default value for the optional field `container = { 
> "mesos": {} }` is not necessary as it is defaulted in java code on the api 
> side.
> 
> Tracking this down it looks like the default value was a last minute addition 
> based on some review comments and is not necessary or used anywhere.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 24a2ca8cc9a236228fdd8aa61c877319a803c9a6 
> 
> Diff: https://reviews.apache.org/r/38389/diff/
> 
> 
> Testing
> ---
> 
> Passes all java/py tests in ./build-support/jenkins/build.sh
> Passes all e2e tests in ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Jake Farrell
> 
>

Re: Review Request 38389: AURORA-1485: TaskConfig default container value breaks client library generation for GO

[Jake Farrell]

> On Sept. 15, 2015, 1:24 a.m., Bill Farner wrote:
> > The default value wasn't a hack, but moving it to the end of the 
> > `TaskConfig` message definition was.  I think the default is useful and 
> > should really remain.  IIRC we concluded that the `optional` was 
> > unnecessary, though.
> > 
> > I'm -1 to removing the default due to what appears like a thrift bug, but 
> > +1 to removing the confusing `optional` if that happens to fix things.

patch updated to remove optional and leave default value. Passes all test cases 
and e2e.


- Jake


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


On Sept. 15, 2015, 7:56 p.m., Jake Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38389/
> ---
> 
> (Updated Sept. 15, 2015, 7:56 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-1485
> https://issues.apache.org/jira/browse/AURORA-1485
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When generating the client api library for go the default containerizer used 
> as a hack for AURORA-1185 breaks and causes the client lib to not be 
> generated correctly. The default value for the optional field `container = { 
> "mesos": {} }` is not necessary as it is defaulted in java code on the api 
> side.
> 
> Tracking this down it looks like the default value was a last minute addition 
> based on some review comments and is not necessary or used anywhere.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 24a2ca8cc9a236228fdd8aa61c877319a803c9a6 
> 
> Diff: https://reviews.apache.org/r/38389/diff/
> 
> 
> Testing
> ---
> 
> Passes all java/py tests in ./build-support/jenkins/build.sh
> Passes all e2e tests in ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Jake Farrell
> 
>

Re: Review Request 38385: Documenting dedicated job & quota relationship.

[Stephan Erb]

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


Reading your changes, I have noticed that we are missing a canonical 
explanation of the quota feature. It is only explained via cross references 
from the production flag. A gentle high-level introduction (maybe here 
https://github.com/apache/aurora/blob/master/docs/resource-isolation.md) would 
be really beneficial for beginners.

(This remark is orthogonal to the issue addressed in this review request, so 
feel free to dismiss it here)

- Stephan Erb


On Sept. 15, 2015, 1:50 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38385/
> ---
> 
> (Updated Sept. 15, 2015, 1:50 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1462
> https://issues.apache.org/jira/browse/AURORA-1462
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Documenting dedicated job & quota relationship.
> 
> 
> Diffs
> -
> 
>   docs/client-commands.md f91e5df0c19104a961aa9955478bf2d37a2aa7b6 
>   docs/configuration-reference.md ad2701cadd38bb2fdbbe2acc477038986f8ec733 
>   docs/configuration-tutorial.md d6e7c352e16964e19a41340cb03a016620c17f7d 
>   docs/deploying-aurora-scheduler.md 8db0e615b6abe6865a889dbcfb24271655caaee6 
> 
> Diff: https://reviews.apache.org/r/38385/diff/
> 
> 
> Testing
> ---
> 
> Private remote: 
> https://github.com/maxim111333/incubator-aurora/tree/quota_dedicated_docs/docs
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>