You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2015/03/11 01:04:03 UTC
Review Request 31916: Add scheduler API support for audit messages
when changing job updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31916/
-----------------------------------------------------------
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.
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
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.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 48
> > <https://reviews.apache.org/r/31916/diff/1/?file=890852#file890852line48>
> >
> > 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.
Posted by Bill Farner <wf...@apache.org>.
> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 48
> > <https://reviews.apache.org/r/31916/diff/1/?file=890852#file890852line48>
> >
> > 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 48
> > <https://reviews.apache.org/r/31916/diff/1/?file=890852#file890852line48>
> >
> > 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.
Posted by Joshua Cohen <jc...@apache.org>.
> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 48
> > <https://reviews.apache.org/r/31916/diff/1/?file=890852#file890852line48>
> >
> > 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.
Posted by Bill Farner <wf...@apache.org>.
> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 48
> > <https://reviews.apache.org/r/31916/diff/1/?file=890852#file890852line48>
> >
> > 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.
Posted by Bill Farner <wf...@apache.org>.
> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 48
> > <https://reviews.apache.org/r/31916/diff/1/?file=890852#file890852line48>
> >
> > 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On March 11, 2015, 12:19 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 48
> > <https://reviews.apache.org/r/31916/diff/1/?file=890852#file890852line48>
> >
> > 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/31916/#comment123392>
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.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 31916: Add scheduler API support for audit
messages when changing job updates.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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