aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 24116: Defining stubs for the Update APIs.
Date Thu, 31 Jul 2014 23:50:59 GMT


> 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
+ map<i32, 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 set<JobUpdate>, 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.")

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.


- Maxim


-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message