Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 9:40 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Changes --- Dropping trailing commas. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On Aug. 1, 2014, 10:20 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1251 https://reviews.apache.org/r/24116/diff/5/?file=648843#file648843line1251 rather than throwing could we default this to an error response for ease of client debugging? an exception will be translated into an error: https://github.com/apache/incubator-aurora/blob/master/src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java#L90 - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49392 --- On Aug. 1, 2014, 9:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 9:40 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On Aug. 1, 2014, 3:20 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1251 https://reviews.apache.org/r/24116/diff/5/?file=648843#file648843line1251 rather than throwing could we default this to an error response for ease of client debugging? Bill Farner wrote: an exception will be translated into an error: https://github.com/apache/incubator-aurora/blob/master/src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java#L90 TMYK - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49392 --- On Aug. 1, 2014, 2:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 2:40 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49396 --- Ship it! Ship It! - Kevin Sweeney On Aug. 1, 2014, 2:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 2:40 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On Aug. 1, 2014, 3:29 p.m., Kevin Sweeney wrote: Ship It! (+1 once Bill's concerns are addressed) - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49396 --- On Aug. 1, 2014, 2:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 2:40 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1251 https://reviews.apache.org/r/24116/diff/5/?file=648843#file648843line1251 throw new UnsupportedOperationException(Not implemented); here and elsewhere Done. On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 540 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line540 once all reviewers like the shape of things, please come back to doc the struct fields. Doing it now and will revisit if there are more comments. On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 541 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line541 I wonder if we should use this opportunity to choose a name that does not imply batching. maxSimultaneousInstances? updateGroupSize? I'm hope to ideas. Anybody else have an opinion? We don't need to dwell on this. David McLaughlin wrote: +1 for a name that more accurately reflects what the property does. I am open to suggestions. Feels like updateGroupSize would be the least tied to implementation. On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 543 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line543 maxPerInstanceFailures? total to me suggests the global counter, which i think we should avoid This is the global counter. There must be something to track failed instances. How about maxFailedInstances? Also, changed the maxInstanceFailures to maxPerInstanceFailures to avoid confusion. On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 566 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line566 how would you feel about a closed range instead of a set? i think this improves API ergonomics, matches the use cases we know of, and doesn't break any use case. The closed range would not address patchy cases like 2-7, 10-12, 15-18. Also, it will implicitly assign TaskConfigs to instance holes. On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 596 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line596 space before open brace Fixed. On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 608 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line608 avoid introducing the new noun deploy oops, wrong concept crawled in :) On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 611 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line611 don't use a set, otherwise it begs the question why the others aren't all multi-value as well. This was requested by David. Dropping it until there is a need for it. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49387 --- On Aug. 1, 2014, 9:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 9:40 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 10:53 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Changes --- CR comments. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 541 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line541 I wonder if we should use this opportunity to choose a name that does not imply batching. maxSimultaneousInstances? updateGroupSize? I'm hope to ideas. Anybody else have an opinion? We don't need to dwell on this. David McLaughlin wrote: +1 for a name that more accurately reflects what the property does. Maxim Khutornenko wrote: I am open to suggestions. Feels like updateGroupSize would be the least tied to implementation. +1 to updateGroupSize On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 543 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line543 maxPerInstanceFailures? total to me suggests the global counter, which i think we should avoid Maxim Khutornenko wrote: This is the global counter. There must be something to track failed instances. How about maxFailedInstances? Also, changed the maxInstanceFailures to maxPerInstanceFailures to avoid confusion. I think we mean the same thing. maxFailedInstances and maxPerInstanceFailures should self-doc On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 566 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line566 how would you feel about a closed range instead of a set? i think this improves API ergonomics, matches the use cases we know of, and doesn't break any use case. Maxim Khutornenko wrote: The closed range would not address patchy cases like 2-7, 10-12, 15-18. Also, it will implicitly assign TaskConfigs to instance holes. ...but you have a list of them a level above, so you would have listrange instead of listset, both can express the same things AFAICT - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49387 --- On Aug. 1, 2014, 10:53 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 10:53 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On Aug. 1, 2014, 10:02 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 611 https://reviews.apache.org/r/24116/diff/5/?file=648844#file648844line611 don't use a set, otherwise it begs the question why the others aren't all multi-value as well. Maxim Khutornenko wrote: This was requested by David. Dropping it until there is a need for it. Sorry I misread this - thought it was a set versus list thing. This is definitely required to get all completed or active updates in a single RPC call. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49387 --- On Aug. 1, 2014, 10:53 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 10:53 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49407 --- Ship it! LGTM once setRange is used instead of setI32 - Bill Farner On Aug. 1, 2014, 10:53 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 10:53 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On Aug. 1, 2014, 11:46 p.m., Bill Farner wrote: LGTM once setRange is used instead of setI32 Done. Apparently, both begin and end are reserved thrift words. Had to stick to first and last for the Range. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49407 --- On Aug. 1, 2014, 10:53 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 10:53 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated Aug. 1, 2014, 11:58 p.m.) Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner. Changes --- CR comments. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On July 30, 2014, 9:36 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 692 https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line692 How would you feel about generalizing this to support more use cases, like getting all in-progress updates? You could accept more parameters for this: Response getUpdates(1: string role, 2: JobKey job, 3: UpdateStatus status) Not sure I like it. What should be the output if all 3 are provided? How about something like UpdateQuery instead (similar to TaskQuery)? - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49168 --- On July 30, 2014, 9:12 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 30, 2014, 9:12 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49254 --- I'd also suggest the following API methods: * Query updates based on state(s). I'm thinking of a show me all updates in progress type view that we'd want to support. * Query updates based on last event time. Here it's just a paginated view of all recent events, ordered by descending timestamp. - David McLaughlin On July 30, 2014, 9:12 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 30, 2014, 9:12 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On July 30, 2014, 9:36 p.m., David McLaughlin wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 505 https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line505 What about the shards option? Maxim Khutornenko wrote: I was not sure if we wanted to implement it in the first version. Definitely can throw it in. Yeah, I think it would be good to include it. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49171 --- On July 30, 2014, 9:12 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 30, 2014, 9:12 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 31, 2014, 6:27 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Changes --- Reworked update objects and addressed comments. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On July 31, 2014, 5:57 p.m., David McLaughlin wrote: I'd also suggest the following API methods: * Query updates based on state(s). I'm thinking of a show me all updates in progress type view that we'd want to support. * Query updates based on last event time. Here it's just a paginated view of all recent events, ordered by descending timestamp. The new getUpdates API will handle the state. I am not sure about the last event time yet. Let's circle back when we actually need one. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49254 --- On July 31, 2014, 6:27 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 31, 2014, 6:27 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 1262
https://reviews.apache.org/r/24116/diff/1/?file=646274#file646274line1262
Drop a Not implemented message in here and it will nicely propagate
out through the API.
Done.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 504
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line504
Please doc this, and fields within. Ditto elsewhere.
Done.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 513
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line513
Here's the enum i've sketched out in my branch, how do you feel about
these instead?
enum Status {
INIT,
ROLLING_FORWARD,
ROLLING_BACK,
ROLL_FORWARD_PAUSED,
ROLL_BACK_PAUSED,
ROLLED_FORWARD,
ROLLED_BACK,
ABORTED,
ERROR
}
Changed.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 525
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line525
Why does an update need to know the starting state? Shouldn't this be
gleaned from the Update immediately before it?
Dropped Update object completely.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 526
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line526
Shouldn't this be a TaskConfig?
Yup.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 528
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line528
startTimestampMs?
Dropped.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 529
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line529
endTimestampMs?
Dropped.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 582
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line582
This should contain update events, and explicitly not the Update. Can
you define a half-baked UpdateEvent struct? Just an instance ID and
timestamp should be fine for now, along with a TODO.
There is now UpdateEvent with a collection of UpdateInstanceEvents.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 688
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line688
Add a note about being not yet implemented here, for nice presentation
in the auto-generated help pages.
Done.
On July 30, 2014, 9:36 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 692
https://reviews.apache.org/r/24116/diff/1/?file=646275#file646275line692
How would you feel about generalizing this to support more use cases,
like getting all in-progress updates? You could accept more parameters for
this:
Response getUpdates(1: string role, 2: JobKey job, 3: UpdateStatus
status)
Maxim Khutornenko wrote:
Not sure I like it. What should be the output if all 3 are provided?
How about something like UpdateQuery instead (similar to TaskQuery)?
Taken the UpdateQuery-based approach.
- Maxim
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24116/#review49168
---
On July 31, 2014, 6:27 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24116/
---
(Updated July 31, 2014, 6:27 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-611
https://issues.apache.org/jira/browse/AURORA-611
Repository: aurora
Description
---
First stab at update APIs.
Diffs
-
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
12de5a3e9e3aae217b30c385e2d7ec7b43863ae2
src/main/thrift/org/apache/aurora/gen/api.thrift
54b8985971719247a5d42d8676075a51045bbb92
src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java
2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10
Diff: https://reviews.apache.org/r/24116/diff/
Testing
---
gradle build
Thanks,
Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On July 31, 2014, 6:31 p.m., David McLaughlin wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 575 https://reviews.apache.org/r/24116/diff/1-2/?file=646275#file646275line575 Can we make this a list? Sure, that's reasonable. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49263 --- On July 31, 2014, 6:27 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 31, 2014, 6:27 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 31, 2014, 6:37 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Changes --- CR comments. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49267 --- Ship it! LGTM as a start - although I'm sure I'll have more feedback once we start implementing the UI work. - David McLaughlin On July 31, 2014, 6:37 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 31, 2014, 6:37 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49271 --- src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/24116/#comment86203 Total nit, but shouldn't min come before max? src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/24116/#comment86204 FWIW, I think bill's states are clearer. src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/24116/#comment86205 Definitely support the idea of the more general queries. If we're going to the trouble of supporting an API like this, we should make the extra effort to make it flexible. - Mark Chu-Carroll On July 31, 2014, 2:37 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 31, 2014, 2:37 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49294 --- Was drafting some UI mock ups when I spotted some more issues. src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/24116/#comment86246 This needs everything we know about the update to be returned (e.g. UpdateConfiguration and UpdateSettings). We'll want to show all this info in the UI. src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/24116/#comment86247 Can we add pagination to this? - David McLaughlin On July 31, 2014, 6:37 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 31, 2014, 6:37 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On July 31, 2014, 11:13 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 546
https://reviews.apache.org/r/24116/diff/3/?file=647049#file647049line546
I find the latest revision of the structs a bit tough to comprehend.
How about this tweak:
struct JobUpdateRequest {
// fields currently in UpdateSettings and UpdateConfiguration
}
// the high-level view of what an update intended to do, and its
current status
struct JobUpdate {
1: string id
2: JobKey job
3: TaskConfig config
4: i32 instances
5: UpdateStatus status
6: i64 startTimestampMs
7: i64 endTimestampMs
}
enum InstanceUpdateAction {
ROLLED_FORWARD,
ROLLED_BACK,
ADDED,
REMOVED
}
// the individual actions taken as part of an update
struct InstanceUpdateEvent {
1: i32 instance
2: InstanceUpdateAction action
3: i64 timestampMs
}
These two structs would be fetched via two separate API calls.
Maxim Khutornenko wrote:
How does JobUpdate fit with the getUpdates() API? Do you propose we
return JobUpdate + mapi32, InstanceUpdateEvent with it?
I am not sure I like the fact that we are mixing immutable and mutable
data within the same return struct. This would mean we always return
TaskConfig even when it's not really needed.
I'd rather go with Event-only getUpdates() API and have something like
getUpdateDetails(string id) that would return JobUpdate with immutable-only
data. This approach is more like the current diff + a new API to return
JobUpdate only.
In the above arrangement, getUpdates() would return setJobUpdate, another API
would return the events associated with a specific update. (That's what i meant
by These two structs would be fetched via two separate API calls.)
- Bill
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24116/#review49301
---
On July 31, 2014, 6:37 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24116/
---
(Updated July 31, 2014, 6:37 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-611
https://issues.apache.org/jira/browse/AURORA-611
Repository: aurora
Description
---
First stab at update APIs.
Diffs
-
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
12de5a3e9e3aae217b30c385e2d7ec7b43863ae2
src/main/thrift/org/apache/aurora/gen/api.thrift
54b8985971719247a5d42d8676075a51045bbb92
src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java
2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10
Diff: https://reviews.apache.org/r/24116/diff/
Testing
---
gradle build
Thanks,
Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
On July 31, 2014, 11:13 p.m., Bill Farner wrote:
src/main/thrift/org/apache/aurora/gen/api.thrift, line 546
https://reviews.apache.org/r/24116/diff/3/?file=647049#file647049line546
I find the latest revision of the structs a bit tough to comprehend.
How about this tweak:
struct JobUpdateRequest {
// fields currently in UpdateSettings and UpdateConfiguration
}
// the high-level view of what an update intended to do, and its
current status
struct JobUpdate {
1: string id
2: JobKey job
3: TaskConfig config
4: i32 instances
5: UpdateStatus status
6: i64 startTimestampMs
7: i64 endTimestampMs
}
enum InstanceUpdateAction {
ROLLED_FORWARD,
ROLLED_BACK,
ADDED,
REMOVED
}
// the individual actions taken as part of an update
struct InstanceUpdateEvent {
1: i32 instance
2: InstanceUpdateAction action
3: i64 timestampMs
}
These two structs would be fetched via two separate API calls.
Maxim Khutornenko wrote:
How does JobUpdate fit with the getUpdates() API? Do you propose we
return JobUpdate + mapi32, InstanceUpdateEvent with it?
I am not sure I like the fact that we are mixing immutable and mutable
data within the same return struct. This would mean we always return
TaskConfig even when it's not really needed.
I'd rather go with Event-only getUpdates() API and have something like
getUpdateDetails(string id) that would return JobUpdate with immutable-only
data. This approach is more like the current diff + a new API to return
JobUpdate only.
Bill Farner wrote:
In the above arrangement, getUpdates() would return setJobUpdate,
another API would return the events associated with a specific update.
(That's what i meant by These two structs would be fetched via two separate
API calls.)
Maxim Khutornenko wrote:
In that case, the getUpdates(UpdateQuery) would return a bunch of quite
heavy JobUpdate objects without any need for their TaskConfigs. This would
make a query across cluster quite an expensive one.
Why not make getUpdates() return event-only data and another API fetch
heavy stuff when it's really needed? I think we are on the same page wrt 2
APIs, I just think the default (getUpdates) API should not include
TaskConfigs with it.
Instead of my original JobUpdate, how about this:
struct JobUpdate {
1: string id
2: JobKey job
3: UpdateStatus status
4: i64 endTimestampMs
}
InstanceUpdateAction, InstanceUpdateEvent,
s/JobUpdateRequest/JobUpdateConfiguration/ as above
Then 3 API methods:
- getJobUpdates(JobUpdateQuery query)
- getJobUpdateConfiguration(string id)
- getJobUpdateEvents(string id)
- Bill
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24116/#review49301
---
On July 31, 2014, 6:37 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24116/
---
(Updated July 31, 2014, 6:37 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-611
https://issues.apache.org/jira/browse/AURORA-611
Repository: aurora
Description
---
First stab at update APIs.
Diffs
-
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
12de5a3e9e3aae217b30c385e2d7ec7b43863ae2
src/main/thrift/org/apache/aurora/gen/api.thrift
54b8985971719247a5d42d8676075a51045bbb92
src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java
2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10
Diff: https://reviews.apache.org/r/24116/diff/
Testing
---
gradle build
Thanks,
Maxim Khutornenko
Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/#review49170 --- src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/24116/#comment86026 Why is the previous JobConfiguration needed to do an update? src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/24116/#comment86027 Does it need an updated timestamp for pauses, etc.? - David McLaughlin On July 30, 2014, 9:12 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24116/ --- (Updated July 30, 2014, 9:12 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-611 https://issues.apache.org/jira/browse/AURORA-611 Repository: aurora Description --- First stab at update APIs. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 12de5a3e9e3aae217b30c385e2d7ec7b43863ae2 src/main/thrift/org/apache/aurora/gen/api.thrift 54b8985971719247a5d42d8676075a51045bbb92 src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10 Diff: https://reviews.apache.org/r/24116/diff/ Testing --- gradle build Thanks, Maxim Khutornenko
Re: Review Request 24116: Defining stubs for the Update APIs.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24116/#review49168
---
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
https://reviews.apache.org/r/24116/#comment86011
Drop a Not implemented message in here and it will nicely propagate out
through the API.
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86017
Please doc this, and fields within. Ditto elsewhere.
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86016
Here's the enum i've sketched out in my branch, how do you feel about these
instead?
enum Status {
INIT,
ROLLING_FORWARD,
ROLLING_BACK,
ROLL_FORWARD_PAUSED,
ROLL_BACK_PAUSED,
ROLLED_FORWARD,
ROLLED_BACK,
ABORTED,
ERROR
}
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86018
Why does an update need to know the starting state? Shouldn't this be
gleaned from the Update immediately before it?
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86019
Shouldn't this be a TaskConfig?
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86020
startTimestampMs?
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86021
endTimestampMs?
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86028
This should contain update events, and explicitly not the Update. Can you
define a half-baked UpdateEvent struct? Just an instance ID and timestamp
should be fine for now, along with a TODO.
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86012
Add a note about being not yet implemented here, for nice presentation in
the auto-generated help pages.
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/24116/#comment86023
How would you feel about generalizing this to support more use cases, like
getting all in-progress updates? You could accept more parameters for this:
Response getUpdates(1: string role, 2: JobKey job, 3: UpdateStatus status)
- Bill Farner
On July 30, 2014, 9:12 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24116/
---
(Updated July 30, 2014, 9:12 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-611
https://issues.apache.org/jira/browse/AURORA-611
Repository: aurora
Description
---
First stab at update APIs.
Diffs
-
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
12de5a3e9e3aae217b30c385e2d7ec7b43863ae2
src/main/thrift/org/apache/aurora/gen/api.thrift
54b8985971719247a5d42d8676075a51045bbb92
src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java
2ea4a9ba0a1ea81fea5c4f5203457aa79ae67c10
Diff: https://reviews.apache.org/r/24116/diff/
Testing
---
gradle build
Thanks,
Maxim Khutornenko
