Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Feb. 13, 2015, 12:35 a.m.) Review request for Aurora and Bill Farner. Changes --- Rebased. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs (updated) - docs/developing-aurora-client.md e002d908ce07020513fa06c7e70c41f9e2f55b1d docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review72291 --- docs/thrift-deprecation.md https://reviews.apache.org/r/29117/#comment118378 Sorry I missed this first review, but AFAIK this isn't true for dynamic languages (incl. our own scheduler UI)? - David McLaughlin On Feb. 13, 2015, 12:35 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Feb. 13, 2015, 12:35 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md e002d908ce07020513fa06c7e70c41f9e2f55b1d docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
On Feb. 13, 2015, 12:41 a.m., David McLaughlin wrote: docs/thrift-deprecation.md, line 20 https://reviews.apache.org/r/29117/diff/4/?file=862696#file862696line20 Sorry I missed this first review, but AFAIK this isn't true for dynamic languages (incl. our own scheduler UI)? Can you elaborate? The above statement assumes renaming the field throughout the codebase (including java, python and JS). Current +- 1 versions of client will still be able to communicate with the scheduler. UI ships with the scheduler and thus will not have the renaming problelm. What am I missing? - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review72291 --- On Feb. 13, 2015, 12:35 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Feb. 13, 2015, 12:35 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md e002d908ce07020513fa06c7e70c41f9e2f55b1d docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review72295 --- Ship it! Master (b62ec61) 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 Feb. 13, 2015, 12:35 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Feb. 13, 2015, 12:35 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md e002d908ce07020513fa06c7e70c41f9e2f55b1d docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review70652 --- Ship it! Ship It! - Bill Farner On Jan. 21, 2015, 10:59 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Jan. 21, 2015, 10:59 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review69071 --- This patch does not apply cleanly on master (116ee2d), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Jan. 21, 2015, 10:59 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Jan. 21, 2015, 10:59 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/thrift-deprecation.md, line 31 https://reviews.apache.org/r/29117/diff/2/?file=793344#file793344line31 There should be an item about logging and signaling in API responses when deprecated fields are used. Maxim Khutornenko wrote: We have not done this before and there is no standard way to accomplish it across our codebases. Should it rather be done at the feature level rather than the thrift schema level? I've definitely pushed for server-side logging in the scheduler, client-side logging, and i definitely think the API should include a `ResponseDetail` entry when a deprecated feature is used in a request. On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/thrift-deprecation.md, line 3 https://reviews.apache.org/r/29117/diff/2/?file=793344#file793344line3 First suggestion should be to go read this page: http://diwakergupta.github.io/thrift-missing-guide/ The page doesn't call out schema evolution, but fills in a bunch of other context. Maxim Khutornenko wrote: I had it initially but decided to leave it out to avoid possible confusion. Glad to add it back if you feel it will be useful. I see little harm - so i think it should be here. On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/thrift-deprecation.md, line 16 https://reviews.apache.org/r/29117/diff/2/?file=793344#file793344line16 I have mixed feelings about this. It's fine on the wire _for specific thrift encodings_. This would, for example, break /apibeta. Maxim Khutornenko wrote: Do you mean backwards compatibility with he old way of consuming /apibeta output? I don't think we are at the position to fully support /apibeta compatibilty and given its rather experimental status we most likely would not do it anyway. I dunno, i think it's something we should not take so lightly as to encourage casual renames. It's a practice we need to begin stepping up anyhow as we prepare to introduce a non-thrift API. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review67471 --- On Jan. 21, 2015, 10:59 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Jan. 21, 2015, 10:59 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/developing-aurora-client.md, line 124 https://reviews.apache.org/r/29117/diff/2/?file=793342#file793342line124 s/thrift/Thrift/ Uppercased everywhere. On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/thrift-deprecation.md, line 3 https://reviews.apache.org/r/29117/diff/2/?file=793344#file793344line3 First suggestion should be to go read this page: http://diwakergupta.github.io/thrift-missing-guide/ The page doesn't call out schema evolution, but fills in a bunch of other context. I had it initially but decided to leave it out to avoid possible confusion. Glad to add it back if you feel it will be useful. On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/thrift-deprecation.md, line 5 https://reviews.apache.org/r/29117/diff/2/?file=793344#file793344line5 capable of correctly handling done On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/thrift-deprecation.md, line 16 https://reviews.apache.org/r/29117/diff/2/?file=793344#file793344line16 I have mixed feelings about this. It's fine on the wire _for specific thrift encodings_. This would, for example, break /apibeta. Do you mean backwards compatibility with he old way of consuming /apibeta output? I don't think we are at the position to fully support /apibeta compatibilty and given its rather experimental status we most likely would not do it anyway. On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/thrift-deprecation.md, line 26 https://reviews.apache.org/r/29117/diff/2/?file=793344#file793344line26 Lost me at a field double. Rephrase? rephrased On Jan. 9, 2015, 6:44 p.m., Bill Farner wrote: docs/thrift-deprecation.md, line 31 https://reviews.apache.org/r/29117/diff/2/?file=793344#file793344line31 There should be an item about logging and signaling in API responses when deprecated fields are used. We have not done this before and there is no standard way to accomplish it across our codebases. Should it rather be done at the feature level rather than the thrift schema level? - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review67471 --- On Jan. 6, 2015, 11:30 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Jan. 6, 2015, 11:30 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Jan. 21, 2015, 10:59 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Bill's comments. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs (updated) - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review67471 --- docs/developing-aurora-client.md https://reviews.apache.org/r/29117/#comment111505 s/thrift/Thrift/ docs/developing-aurora-scheduler.md https://reviews.apache.org/r/29117/#comment111506 ditto docs/thrift-deprecation.md https://reviews.apache.org/r/29117/#comment111511 First suggestion should be to go read this page: http://diwakergupta.github.io/thrift-missing-guide/ The page doesn't call out schema evolution, but fills in a bunch of other context. docs/thrift-deprecation.md https://reviews.apache.org/r/29117/#comment111507 capable of correctly handling docs/thrift-deprecation.md https://reviews.apache.org/r/29117/#comment111516 I have mixed feelings about this. It's fine on the wire _for specific thrift encodings_. This would, for example, break /apibeta. docs/thrift-deprecation.md https://reviews.apache.org/r/29117/#comment111517 Lost me at a field double. Rephrase? docs/thrift-deprecation.md https://reviews.apache.org/r/29117/#comment111518 There should be an item about logging and signaling in API responses when deprecated fields are used. - Bill Farner On Jan. 6, 2015, 11:30 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Jan. 6, 2015, 11:30 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Jan. 6, 2015, 11:30 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- +wfarner Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- Review request for Aurora and Kevin Sweeney. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Dec. 16, 2014, 10:29 p.m.) Review request for Aurora and Kevin Sweeney. Changes --- Fixing thrift links. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs (updated) - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko
Re: Review Request 29117: Adding thrift API changes document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/#review65255 --- Ship it! Master (95ad0fa) 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 Dec. 16, 2014, 10:29 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29117/ --- (Updated Dec. 16, 2014, 10:29 p.m.) Review request for Aurora and Kevin Sweeney. Bugs: AURORA-973 https://issues.apache.org/jira/browse/AURORA-973 Repository: aurora Description --- This is a first stab at documenting thrift deprecation. Any suggestions/comments are welcome. Diffs - docs/developing-aurora-client.md b9912bce44d65ddd7f1e35f0ea9356a89d5fe767 docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f docs/thrift-deprecation.md PRE-CREATION Diff: https://reviews.apache.org/r/29117/diff/ Testing --- https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md Thanks, Maxim Khutornenko