> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> >
> 
> Suman Karumuri wrote:
>     Spoke to Bill before making these changes. We decided to use getInfo API 
> and deprecate getAPIVersion. The getInfo will return with the clusterName and 
> APIVersion for now. In future it may include other attributes and hence the 
> flexible map.
>     
>     Thanks for the code comments. Will incorporate them once the API changes 
> are confirmed.
> 
> Kevin Sweeney wrote:
>     For this case the magic name still doesn't make sense to me as it's 
> defined in api.thrift. Why not just make it a field and maintain type-safety?
> 
> Bill Farner wrote:
>     I agree with Kevin, for all intents and purposes you can consider the 
> thrift struct a map.  Formalizing this with field names gives a more 
> consumable API.

Done.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 102
> > <https://reviews.apache.org/r/17562/diff/2/?file=456989#file456989line102>
> >
> >     This binding annotation belongs to the app, not the the http package. 
> > Please rename.

I am not sure what you mean here. Will ping you tomorrow for clarification.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  lines 906-908
> > <https://reviews.apache.org/r/17562/diff/2/?file=456989#file456989line906>
> >
> >     No need to create a new map every time - this can be an instance field 
> > set in the constructor.

Replaced this with GetInfoResult. Not moving it into constructor since 
constructing a response in the constructor seems weird and since the 
performance gains are negligible.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 
> > 19
> > <https://reviews.apache.org/r/17562/diff/2/?file=456992#file456992line19>
> >
> >     !==

Done


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 
> > 21
> > <https://reviews.apache.org/r/17562/diff/2/?file=456992#file456992line21>
> >
> >     !==

Thing we miss, when switching between languages. Done.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 31
> > <https://reviews.apache.org/r/17562/diff/2/?file=456993#file456993line31>
> >
> >     This comment is not informative.

Removed.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 32
> > <https://reviews.apache.org/r/17562/diff/2/?file=456993#file456993line32>
> >
> >     Rather than a "magic name" constant here you can just make clusterName 
> > a field in GetInfoResult to preserve type-safety and make generated code 
> > obvious.
> 
> Suman Karumuri wrote:
>     This is intentional.

Done.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, lines 34-36
> > <https://reviews.apache.org/r/17562/diff/2/?file=456993#file456993line34>
> >
> >     Revert.

Removed magic name. 


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 412
> > <https://reviews.apache.org/r/17562/diff/2/?file=456993#file456993line412>
> >
> >     Since the cluster name constant is defined here why not explicitly name 
> > it? In general I prefer the map<string, string> generalization only when 
> > the contents of the struct should somehow be "opaque" to the scheduler 
> > (e.g. defined by the client or defined by a plugin).
> 
> Suman Karumuri wrote:
>     Yes, it is intentionally kept generic.

Discussed in ticket.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, lines 454-457
> > <https://reviews.apache.org/r/17562/diff/2/?file=456993#file456993line454>
> >
> >     Not clearly documented but this is a "special" API that shouldn't be 
> > deprecated - old clients use it to determine they are out of date and 
> > short-circuit.
> 
> Suman Karumuri wrote:
>     We deprecated the API to ensure that we don't program against it anymore. 
> The plan is to delete it when all the old clients are gone. I plan to track 
> it using the API metrics. Once the usage of the API goes to zero will remove 
> it then.
>     
>     Also, I have filed AURORA-142 to move the aurora client over to the new 
> API.  AURORA-143 tracks, deleting the API task.

Discussed in ticket.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 459
> > <https://reviews.apache.org/r/17562/diff/2/?file=456993#file456993line459>
> >
> >     "metadata" - no hyphen.

Done.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 460
> > <https://reviews.apache.org/r/17562/diff/2/?file=456993#file456993line460>
> >
> >     How about getClusterInfo
> 
> Suman Karumuri wrote:
>     This is intentional.

Leaving this as getInfo to allow for future additions.


- Suman


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


On Jan. 30, 2014, 10:27 p.m., Suman Karumuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17562/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2014, 10:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-120
>     https://issues.apache.org/jira/browse/AURORA-120
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Added getInfo API that returns page title and thrift API version.
> 
> Added a TitleController to UI to set the page title. The controller queries 
> the getInfo API to get the clusterName which is now included in the page 
> title.
> 
> Deprecated getVersion API in favor of getInfo to obtain the thrift API 
> version. 
> 
> 
> Diffs
> -----
> 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  822b8b9e3348677ff2f45da2f76ae9a7a96a41b6 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 
> 2601da9ef64ef91cbf9db9b6f12a8d2cdbb0c297 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 0abd3e737e901f08d18c3ceb55ea2f94847cc2e1 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 
> a0385e95c8c1fc950f507511377dcebe35f258a7 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> 94569f9abfb56c50e67e09cc018484463f9de427 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  02b7a27e0a9ec5226d9d043de8bf4739fb151b09 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> 644d6e87f5ed44d2705ebbcc6619a727d52c03d8 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 62fc8045f6a5fda234df73452685bd04e3142aaf 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> e88d29732ee8652ede6c7ee13513122086385646 
> 
> Diff: https://reviews.apache.org/r/17562/diff/
> 
> 
> Testing
> -------
> 
> gradle clean build.
> gradle run.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Reply via email to