Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-09 Thread Maxim Khutornenko 


> On Oct. 9, 2015, 5:12 p.m., Kevin Sweeney wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 351
> > 
> >
> > Please rename this to deprecatedInstanceIds.
> 
> David McLaughlin wrote:
> -1 to this deprecation technique.
> 
> Maxim Khutornenko wrote:
> I thought we decided to not follow this approach and rely on 
> announcements and deprecation tickets instead?
> 
> Kevin Sweeney wrote:
> Can you elaborate on the rationale behind your -1?
> 
> Renaming this field does not alter binary compatibility in any way and 
> provides feedback to the API user that the field is slated for removal (in 
> the form of a compiler or unit test break, similar to the `@Deprecated` 
> annotation). A comment here is very likely to be missed. We have used this 
> technique in the past.

IMO, renaming fields does not necessarily improve our deprecation story but 
creates upgrade troubles twice more often: first on DEPRECATED renaming and 
second on field removal.

The @Deprecated annotation != field renaming as it does not break on upgrade 
(unless build env set to break on warnings, where it still can be suppressed 
without code changes).

We have stopped following DEPRECATED renaming awhile ago due to the above 
reasons.


- Maxim


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


On Oct. 9, 2015, 1:32 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 9, 2015, 1:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-09 Thread Kevin Sweeney 


> On Oct. 9, 2015, 10:12 a.m., Kevin Sweeney wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 351
> > 
> >
> > Please rename this to deprecatedInstanceIds.
> 
> David McLaughlin wrote:
> -1 to this deprecation technique.
> 
> Maxim Khutornenko wrote:
> I thought we decided to not follow this approach and rely on 
> announcements and deprecation tickets instead?

Can you elaborate on the rationale behind your -1?

Renaming this field does not alter binary compatibility in any way and provides 
feedback to the API user that the field is slated for removal (in the form of a 
compiler or unit test break, similar to the `@Deprecated` annotation). A 
comment here is very likely to be missed. We have used this technique in the 
past.


- Kevin


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


On Oct. 8, 2015, 6:32 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 8, 2015, 6:32 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-09 Thread David McLaughlin 

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

Ship it!


Ship It!

- David McLaughlin


On Oct. 9, 2015, 1:32 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 9, 2015, 1:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-09 Thread Maxim Khutornenko 


> On Oct. 9, 2015, 5:12 p.m., Kevin Sweeney wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 351
> > 
> >
> > Please rename this to deprecatedInstanceIds.
> 
> David McLaughlin wrote:
> -1 to this deprecation technique.

I thought we decided to not follow this approach and rely on announcements and 
deprecation tickets instead?


- Maxim


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


On Oct. 9, 2015, 1:32 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 9, 2015, 1:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-09 Thread David McLaughlin 


> On Oct. 9, 2015, 5:12 p.m., Kevin Sweeney wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 351
> > 
> >
> > Please rename this to deprecatedInstanceIds.

-1 to this deprecation technique.


- David


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


On Oct. 9, 2015, 1:32 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 9, 2015, 1:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-09 Thread Kevin Sweeney 

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



api/src/main/thrift/org/apache/aurora/gen/api.thrift (line 351)


Please rename this to deprecatedInstanceIds.


- Kevin Sweeney


On Oct. 8, 2015, 6:32 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 8, 2015, 6:32 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-09 Thread Joshua Cohen 

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

Ship it!


Ship It!

- Joshua Cohen


On Oct. 9, 2015, 1:32 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 9, 2015, 1:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-08 Thread Aurora ReviewBot 

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

Ship it!


Master (7408cb3) 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 Oct. 9, 2015, 1:32 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 9, 2015, 1:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-08 Thread Maxim Khutornenko 


> On Oct. 9, 2015, 12:01 a.m., David McLaughlin wrote:
> > This change is backwards incompatible. Shouldn't we add a new field that 
> > provides the new type and deprecate the old field as per the deprecation 
> > policy?
> 
> Maxim Khutornenko wrote:
> This API/struct were created for the UI. Given that there are no other 
> known dependencies and the fact that we are still on 0-major version, I'd 
> consider this as an acceptable tradeoff to minimize clutter. I wonder what 
> others think, should we enforce our versioning guidelines here?
> 
> Joshua Cohen wrote:
> I think we need to assume there are users out there that use the thrift 
> API in ways we don't know about and we need to version accordingly. As much 
> as I'd like to take the easy route and just make the fix I think to be 
> responsible maintainers of OSS we need to go through a deprecation cycle.

Added deprecation note.


- Maxim


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


On Oct. 9, 2015, 1:32 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 9, 2015, 1:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-08 Thread Maxim Khutornenko 

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

(Updated Oct. 9, 2015, 1:32 a.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Deprecating instancesIds.


Repository: aurora


Description
---

Addressing a long standing TODO. Also some cleanup of unused util functions.


Diffs (updated)
-

  NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
a04e644453bfecde44ec7b51b53f42dc82e90c96 
  src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
5c1bdb4aeab285c46475a54bd13aeb780541fa08 
  src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
13b5c222a0d7b9dd347990e6c09aac09ee566315 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
fd07365faa5fe516680600ed774d35c564948b9b 
  src/main/resources/scheduler/assets/js/controllers.js 
85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
  src/main/resources/scheduler/assets/js/services.js 
b7699fe91d79f9a8141c8368da91443684b6994b 
  
src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java 
64cbd5b58d9254bb741e20e47165732e52569f70 

Diff: https://reviews.apache.org/r/39150/diff/


Testing
---

./gradlew -Pq build
Manual UI testing


Thanks,

Maxim Khutornenko

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-08 Thread Joshua Cohen 


> On Oct. 9, 2015, 12:01 a.m., David McLaughlin wrote:
> > This change is backwards incompatible. Shouldn't we add a new field that 
> > provides the new type and deprecate the old field as per the deprecation 
> > policy?
> 
> Maxim Khutornenko wrote:
> This API/struct were created for the UI. Given that there are no other 
> known dependencies and the fact that we are still on 0-major version, I'd 
> consider this as an acceptable tradeoff to minimize clutter. I wonder what 
> others think, should we enforce our versioning guidelines here?

I think we need to assume there are users out there that use the thrift API in 
ways we don't know about and we need to version accordingly. As much as I'd 
like to take the easy route and just make the fix I think to be responsible 
maintainers of OSS we need to go through a deprecation cycle.


- Joshua


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


On Oct. 8, 2015, 11:35 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 8, 2015, 11:35 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-08 Thread Maxim Khutornenko 


> On Oct. 9, 2015, 12:01 a.m., David McLaughlin wrote:
> > This change is backwards incompatible. Shouldn't we add a new field that 
> > provides the new type and deprecate the old field as per the deprecation 
> > policy?

This API/struct were created for the UI. Given that there are no other known 
dependencies and the fact that we are still on 0-major version, I'd consider 
this as an acceptable tradeoff to minimize clutter. I wonder what others think, 
should we enforce our versioning guidelines here?


- Maxim


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


On Oct. 8, 2015, 11:35 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 8, 2015, 11:35 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-08 Thread David McLaughlin 

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


This change is backwards incompatible. Shouldn't we add a new field that 
provides the new type and deprecate the old field as per the deprecation policy?

- David McLaughlin


On Oct. 8, 2015, 11:35 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 8, 2015, 11:35 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-08 Thread Aurora ReviewBot 

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

Ship it!


Master (7408cb3) 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 Oct. 8, 2015, 11:35 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39150/
> ---
> 
> (Updated Oct. 8, 2015, 11:35 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a long standing TODO. Also some cleanup of unused util functions.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a04e644453bfecde44ec7b51b53f42dc82e90c96 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5c1bdb4aeab285c46475a54bd13aeb780541fa08 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 13b5c222a0d7b9dd347990e6c09aac09ee566315 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  fd07365faa5fe516680600ed774d35c564948b9b 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
>   src/main/resources/scheduler/assets/js/services.js 
> b7699fe91d79f9a8141c8368da91443684b6994b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  64cbd5b58d9254bb741e20e47165732e52569f70 
> 
> Diff: https://reviews.apache.org/r/39150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual UI testing
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Review Request 39150: Converting to Range in ConfigGroup thrift.

2015-10-08 Thread Maxim Khutornenko 

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

Review request for Aurora, Joshua Cohen and Bill Farner.


Repository: aurora


Description
---

Addressing a long standing TODO. Also some cleanup of unused util functions.


Diffs
-

  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
a04e644453bfecde44ec7b51b53f42dc82e90c96 
  src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
5c1bdb4aeab285c46475a54bd13aeb780541fa08 
  src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
13b5c222a0d7b9dd347990e6c09aac09ee566315 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
fd07365faa5fe516680600ed774d35c564948b9b 
  src/main/resources/scheduler/assets/js/controllers.js 
85ae8ce9e51d1785e32ba3f2e4f31f35ec6f178d 
  src/main/resources/scheduler/assets/js/services.js 
b7699fe91d79f9a8141c8368da91443684b6994b 
  
src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java 
64cbd5b58d9254bb741e20e47165732e52569f70 

Diff: https://reviews.apache.org/r/39150/diff/


Testing
---

./gradlew -Pq build
Manual UI testing


Thanks,

Maxim Khutornenko