> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney 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.


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

Yes, it is intentionally kept generic.


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 
> > 8
> > <https://reviews.apache.org/r/17562/diff/2/?file=456992#file456992line8>
> >
> >     Any reason this isn't an instance field?


> On Jan. 31, 2014, 12:56 a.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 5
> > <https://reviews.apache.org/r/17562/diff/2/?file=456991#file456991line5>
> >
> >     Is this necessary? Thought you could call chain these.

Yes. Call chaining makes for poorly readable and formatted code, so this is 
recommended. Further doing it this way, makes refactoring easier and cleaner.


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

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.


> 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

This is intentional.


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

This is intentional.


- 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