Re: Review Request 31916: Add scheduler API support for audit messages when changing job updates.
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.
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.
--- 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.
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.
--- 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.
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.
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.
--- 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.
--- 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.
--- 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.
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.
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
