Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-11 Thread Aurora ReviewBot 

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

Ship it!


Master (4ec563f) 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 March 11, 2015, 10:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 10:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  3d9ca4113514628e2ce5cdf9c715d7848bcdae47 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> 1f1e02eff254ac1b26e603da243c0dfc37049291 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-11 Thread Bill Farner 

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

(Updated March 11, 2015, 10:22 p.m.)


Review request for Aurora, David McLaughlin and Joshua Cohen.


Changes
---

Rebase.


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


Repository: aurora


Description
---

Add scheduler API support for audit messages when changing job updates.


Diffs (updated)
-

  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
badb8eec51d9034fdfee79061c777927b2ba1314 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
c0d48034ad6b6a91f1f58aca54544a5eddea4742 
  src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
  
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
3d9ca4113514628e2ce5cdf9c715d7848bcdae47 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 7f5e5c2091458192d9310a81314f3c2c42b11f49 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
eebe01b161fbebdc43e62df4836136a02c3d5fb7 
  src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
1f1e02eff254ac1b26e603da243c0dfc37049291 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-11 Thread Bill Farner 


> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, 
> > line 48
> > 
> >
> > Do we really want to fail an operation when a message gets too long? 
> > Since it's optional anyway, I'd expect truncation could be a more resilient 
> > way to handle this.
> 
> David McLaughlin wrote:
> I think it's better to just give a clear message telling them there is a 
> limit. Truncation could happen in the client if needed.
> 
> Maxim Khutornenko wrote:
> I was considering a case when some automated external service would 
> attempt to act on an update and append some arbitrary metadata with 
> pause/resume/abort. While not desirable, does not necessarily warrant a 
> failure. Stopping a misbehaving update should clearly be a higher priority 
> than enforcing a strict audit mode.
> 
> Bill Farner wrote:
> Maybe it does warrant a failure, though.  IMHO truncation would be a 
> policy decision that the scheduler is making on behalf of the client.  If the 
> most important part of the message is after the truncation, we've made a poor 
> choice.
> 
> Maxim Khutornenko wrote:
> IDK, this whole length enforcment seems quite arbitrary to me. We don't 
> restrict string size anywhere in thrift API or SQL schema. The only exception 
> is TaskIdGenerator where the ID length is critical for external reasons 
> (MESOS-691). Perhaps we should start doing it everywhere then?
> 
> Joshua Cohen wrote:
> If the requirement to restrict message length is not driven by the 
> underlying store, is there any reason to enforce it at all? Could we just let 
> clients truncate messages as appropriate to their use case?
> 
> Are we concerned with abuse and/or performance impact?
> 
> Bill Farner wrote:
> > this whole length enforcment seems quite arbitrary to me
> 
> I find this surprising.  To me it seems reasonable to be defensive and 
> protect a critical service from arbitary-length inputs.
> 
> > We don't restrict string size anywhere in thrift API or SQL schema
> 
> We do in some places, and we used to in more.  IMHO ideally we would do 
> this in every place we have arbitrary input.
> 
> > If the requirement to restrict message length is not driven by the 
> underlying store, is there any reason to enforce it at all?
> 
> IMHO if a user can crash our service, we have failed.  (There are 
> obviously tradeoffs here.)  In this case, i could cripple Aurora using this 
> input.

Circling back from offline discussion between myself, Maxim, and Joshua - we 
should aim to constrain these values at the edge of the system (thrift API) to 
provide useful error messages to users, and do so additionally in the database. 
 I will proceed with this change as-is and follow up with an input validation 
layer in https://issues.apache.org/jira/browse/AURORA-1188


- Bill


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


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-11 Thread Bill Farner 


> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, 
> > line 48
> > 
> >
> > Do we really want to fail an operation when a message gets too long? 
> > Since it's optional anyway, I'd expect truncation could be a more resilient 
> > way to handle this.
> 
> David McLaughlin wrote:
> I think it's better to just give a clear message telling them there is a 
> limit. Truncation could happen in the client if needed.
> 
> Maxim Khutornenko wrote:
> I was considering a case when some automated external service would 
> attempt to act on an update and append some arbitrary metadata with 
> pause/resume/abort. While not desirable, does not necessarily warrant a 
> failure. Stopping a misbehaving update should clearly be a higher priority 
> than enforcing a strict audit mode.
> 
> Bill Farner wrote:
> Maybe it does warrant a failure, though.  IMHO truncation would be a 
> policy decision that the scheduler is making on behalf of the client.  If the 
> most important part of the message is after the truncation, we've made a poor 
> choice.
> 
> Maxim Khutornenko wrote:
> IDK, this whole length enforcment seems quite arbitrary to me. We don't 
> restrict string size anywhere in thrift API or SQL schema. The only exception 
> is TaskIdGenerator where the ID length is critical for external reasons 
> (MESOS-691). Perhaps we should start doing it everywhere then?
> 
> Joshua Cohen wrote:
> If the requirement to restrict message length is not driven by the 
> underlying store, is there any reason to enforce it at all? Could we just let 
> clients truncate messages as appropriate to their use case?
> 
> Are we concerned with abuse and/or performance impact?

> this whole length enforcment seems quite arbitrary to me

I find this surprising.  To me it seems reasonable to be defensive and protect 
a critical service from arbitary-length inputs.

> We don't restrict string size anywhere in thrift API or SQL schema

We do in some places, and we used to in more.  IMHO ideally we would do this in 
every place we have arbitrary input.

> If the requirement to restrict message length is not driven by the underlying 
> store, is there any reason to enforce it at all?

IMHO if a user can crash our service, we have failed.  (There are obviously 
tradeoffs here.)  In this case, i could cripple Aurora using this input.


- Bill


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


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-11 Thread Joshua Cohen 


> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, 
> > line 48
> > 
> >
> > Do we really want to fail an operation when a message gets too long? 
> > Since it's optional anyway, I'd expect truncation could be a more resilient 
> > way to handle this.
> 
> David McLaughlin wrote:
> I think it's better to just give a clear message telling them there is a 
> limit. Truncation could happen in the client if needed.
> 
> Maxim Khutornenko wrote:
> I was considering a case when some automated external service would 
> attempt to act on an update and append some arbitrary metadata with 
> pause/resume/abort. While not desirable, does not necessarily warrant a 
> failure. Stopping a misbehaving update should clearly be a higher priority 
> than enforcing a strict audit mode.
> 
> Bill Farner wrote:
> Maybe it does warrant a failure, though.  IMHO truncation would be a 
> policy decision that the scheduler is making on behalf of the client.  If the 
> most important part of the message is after the truncation, we've made a poor 
> choice.
> 
> Maxim Khutornenko wrote:
> IDK, this whole length enforcment seems quite arbitrary to me. We don't 
> restrict string size anywhere in thrift API or SQL schema. The only exception 
> is TaskIdGenerator where the ID length is critical for external reasons 
> (MESOS-691). Perhaps we should start doing it everywhere then?

If the requirement to restrict message length is not driven by the underlying 
store, is there any reason to enforce it at all? Could we just let clients 
truncate messages as appropriate to their use case?

Are we concerned with abuse and/or performance impact?


- Joshua


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


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-11 Thread Joshua Cohen 

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

Ship it!


LGTM modulo the outstanding question re: length limit.

- Joshua Cohen


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-10 Thread Maxim Khutornenko 


> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, 
> > line 48
> > 
> >
> > Do we really want to fail an operation when a message gets too long? 
> > Since it's optional anyway, I'd expect truncation could be a more resilient 
> > way to handle this.
> 
> David McLaughlin wrote:
> I think it's better to just give a clear message telling them there is a 
> limit. Truncation could happen in the client if needed.
> 
> Maxim Khutornenko wrote:
> I was considering a case when some automated external service would 
> attempt to act on an update and append some arbitrary metadata with 
> pause/resume/abort. While not desirable, does not necessarily warrant a 
> failure. Stopping a misbehaving update should clearly be a higher priority 
> than enforcing a strict audit mode.
> 
> Bill Farner wrote:
> Maybe it does warrant a failure, though.  IMHO truncation would be a 
> policy decision that the scheduler is making on behalf of the client.  If the 
> most important part of the message is after the truncation, we've made a poor 
> choice.

IDK, this whole length enforcment seems quite arbitrary to me. We don't 
restrict string size anywhere in thrift API or SQL schema. The only exception 
is TaskIdGenerator where the ID length is critical for external reasons 
(MESOS-691). Perhaps we should start doing it everywhere then?


- Maxim


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


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-10 Thread Bill Farner 


> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, 
> > line 48
> > 
> >
> > Do we really want to fail an operation when a message gets too long? 
> > Since it's optional anyway, I'd expect truncation could be a more resilient 
> > way to handle this.
> 
> David McLaughlin wrote:
> I think it's better to just give a clear message telling them there is a 
> limit. Truncation could happen in the client if needed.
> 
> Maxim Khutornenko wrote:
> I was considering a case when some automated external service would 
> attempt to act on an update and append some arbitrary metadata with 
> pause/resume/abort. While not desirable, does not necessarily warrant a 
> failure. Stopping a misbehaving update should clearly be a higher priority 
> than enforcing a strict audit mode.

Maybe it does warrant a failure, though.  IMHO truncation would be a policy 
decision that the scheduler is making on behalf of the client.  If the most 
important part of the message is after the truncation, we've made a poor choice.


- Bill


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


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-10 Thread Aurora ReviewBot 

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

Ship it!


Master (b53e023) 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 March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-10 Thread Maxim Khutornenko 


> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, 
> > line 48
> > 
> >
> > Do we really want to fail an operation when a message gets too long? 
> > Since it's optional anyway, I'd expect truncation could be a more resilient 
> > way to handle this.
> 
> David McLaughlin wrote:
> I think it's better to just give a clear message telling them there is a 
> limit. Truncation could happen in the client if needed.

I was considering a case when some automated external service would attempt to 
act on an update and append some arbitrary metadata with pause/resume/abort. 
While not desirable, does not necessarily warrant a failure. Stopping a 
misbehaving update should clearly be a higher priority than enforcing a strict 
audit mode.


- Maxim


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


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-10 Thread David McLaughlin 


> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, 
> > line 48
> > 
> >
> > Do we really want to fail an operation when a message gets too long? 
> > Since it's optional anyway, I'd expect truncation could be a more resilient 
> > way to handle this.

I think it's better to just give a clear message telling them there is a limit. 
Truncation could happen in the client if needed.


- David


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


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-10 Thread Maxim Khutornenko 

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



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java


Do we really want to fail an operation when a message gets too long? Since 
it's optional anyway, I'd expect truncation could be a more resilient way to 
handle this.


- Maxim Khutornenko


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.

2015-03-10 Thread David McLaughlin 

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

Ship it!


Ship It!

- David McLaughlin


On March 11, 2015, 12:04 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31916/
> ---
> 
> (Updated March 11, 2015, 12:04 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Joshua Cohen.
> 
> 
> Bugs: AURORA-1077
> https://issues.apache.org/jira/browse/AURORA-1077
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add scheduler API support for audit messages when changing job updates.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> badb8eec51d9034fdfee79061c777927b2ba1314 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c0d48034ad6b6a91f1f58aca54544a5eddea4742 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 78024a8c257f2d370a4b4d1ba79c6eac68d81ac2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  58824888a4839b71f4efa832a6d62ff6dd946e40 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  7f5e5c2091458192d9310a81314f3c2c42b11f49 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> eebe01b161fbebdc43e62df4836136a02c3d5fb7 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e119c494de8e81d7e2dd1f78337f08dcba3cd518 
> 
> Diff: https://reviews.apache.org/r/31916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>