You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2013/12/17 03:34:32 UTC
Review Request 16311: Fixing updater diff.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/
-----------------------------------------------------------
Review request for Aurora, Bill Farner and Brian Wickman.
Repository: aurora
Description
-------
Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to json and sorting json dict keys.
Diffs
-----
src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
Diff: https://reviews.apache.org/r/16311/diff/
Testing
-------
./pants src/test/python/twitter/aurora/client/api:updater
Thanks,
Maxim Khutornenko
Re: Review Request 16311: Fixing updater diff.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Jan. 2, 2014, 9:11 p.m., Brian Wickman wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, lines 170-191
> > <https://reviews.apache.org/r/16311/diff/3/?file=413507#file413507line170>
> >
> > this seems a bit elaborate. would the following work?
> >
> >
> > import json
> > import pprint
> > import sys
> >
> >
> > def hashable(element):
> > if isinstance(element, (list, set)):
> > return tuple(sorted(hashable(item) for item in element))
> > elif isinstance(element, dict):
> > return tuple(sorted((hashable(key), hashable(value)) for (key, value) in element.items()))
> > return element
> >
> >
> > pprint.pprint(hashable(json.load(sys.stdin)))
> >
> >
> >
> > mba=~=; unzip -p ~/.pants.d/bin/pants.pex/pants-0.0.4-science-py26.pex PEX-INFO | python foo.py
> > ((u'always_write_cache', False),
> > (u'build_properties',
> > ((u'branch', u'git-review/branches/243585'),
> > (u'class', u'CPython'),
> > (u'date', u'Wednesday Dec 04, 2013'),
> > (u'machine', u'ramanujan.uthcode.com'),
> > (u'path', u'/Users/skumaran/twitter/science'),
> > (u'platform', u'macosx-10.4-x86_64'),
> > (u'sha', u'6084f8a289767979535575053a5b26aa8c1fe19b'),
> > (u'tag', u'com.twitter-ibis-thrift-service-4.0.8-24-g6084f8a'),
> > (u'time', u'18:28:38'),
> > (u'timestamp', u'12.04.2013 18:28'),
> > (u'user', u'skumaran'),
> > (u'version', (2, 6, 8)))),
> > (u'egg_caches', ()),
> > (u'entry_point', u'twitter.pants.bin.pants_exe:main'),
> > (u'ignore_errors', False),
> > (u'indices', ()),
> > (u'inherit_path', False),
> > (u'repositories', ()),
> > (u'requirements',
> > ((None, False, u'ansicolors'),
> > (None, False, u'elementtree'),
> > (None, False, u'mako'),
> > (None, False, u'markdown'),
> > (None, False, u'psutil==1.1.2'),
> > (None, False, u'pygments'),
> > (None, False, u'pylint'),
> > (None, False, u'pytest'),
> > (None, False, u'pytest-cov'),
> > (None, False, u'setuptools==1.1.7'))),
> > (u'zip_safe', True))
> >
>
> Maxim Khutornenko wrote:
> Sure, this will work too. It's a bit less readable due to tuple syntax though:
>
> tuples:
> ((u'constraints', (((u'constraint', ((u'limit', ((u'limit', 10),)),)), (u'name', u'limit')), ((u'constraint', ((u'values', (u'1', u'2')),)), (u'name', u'value')))),)
>
> list/tuples:
> [(u'constraints', [[(u'constraint', [(u'limit', [(u'limit', 10)])]), (u'name', u'limit')], [(u'constraint', [(u'values', [u'1', u'2'])]), (u'name', u'value')]])]
>
> However, I can definitely appreciate code brevity of your approach. Changed.
>
> Brian Wickman wrote:
> Hmm. Does unified_diff(pformat(config_in), pformat(config_out)) help? this way it at least pretty prints/indents the repr.
>
> (from pprint import pformat)
The added extra newlines are irrelevant for the unified diff format that displays only deltas:
"((u'constraints',\n (((u'constraint', ((u'limit', ((u'limit', 10),)),)), (u'name', u'limit')),\n ((u'constraint', ((u'values', (u'1', u'2')),)), (u'name', u'value')))),)"
I'd rather avoid extra formatting to reduce diffing surface.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review31064
-----------------------------------------------------------
On Jan. 2, 2014, 10:09 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 10:09 p.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
> src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16311: Fixing updater diff.
Posted by Brian Wickman <wi...@gmail.com>.
> On Jan. 2, 2014, 9:11 p.m., Brian Wickman wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, lines 170-191
> > <https://reviews.apache.org/r/16311/diff/3/?file=413507#file413507line170>
> >
> > this seems a bit elaborate. would the following work?
> >
> >
> > import json
> > import pprint
> > import sys
> >
> >
> > def hashable(element):
> > if isinstance(element, (list, set)):
> > return tuple(sorted(hashable(item) for item in element))
> > elif isinstance(element, dict):
> > return tuple(sorted((hashable(key), hashable(value)) for (key, value) in element.items()))
> > return element
> >
> >
> > pprint.pprint(hashable(json.load(sys.stdin)))
> >
> >
> >
> > mba=~=; unzip -p ~/.pants.d/bin/pants.pex/pants-0.0.4-science-py26.pex PEX-INFO | python foo.py
> > ((u'always_write_cache', False),
> > (u'build_properties',
> > ((u'branch', u'git-review/branches/243585'),
> > (u'class', u'CPython'),
> > (u'date', u'Wednesday Dec 04, 2013'),
> > (u'machine', u'ramanujan.uthcode.com'),
> > (u'path', u'/Users/skumaran/twitter/science'),
> > (u'platform', u'macosx-10.4-x86_64'),
> > (u'sha', u'6084f8a289767979535575053a5b26aa8c1fe19b'),
> > (u'tag', u'com.twitter-ibis-thrift-service-4.0.8-24-g6084f8a'),
> > (u'time', u'18:28:38'),
> > (u'timestamp', u'12.04.2013 18:28'),
> > (u'user', u'skumaran'),
> > (u'version', (2, 6, 8)))),
> > (u'egg_caches', ()),
> > (u'entry_point', u'twitter.pants.bin.pants_exe:main'),
> > (u'ignore_errors', False),
> > (u'indices', ()),
> > (u'inherit_path', False),
> > (u'repositories', ()),
> > (u'requirements',
> > ((None, False, u'ansicolors'),
> > (None, False, u'elementtree'),
> > (None, False, u'mako'),
> > (None, False, u'markdown'),
> > (None, False, u'psutil==1.1.2'),
> > (None, False, u'pygments'),
> > (None, False, u'pylint'),
> > (None, False, u'pytest'),
> > (None, False, u'pytest-cov'),
> > (None, False, u'setuptools==1.1.7'))),
> > (u'zip_safe', True))
> >
>
> Maxim Khutornenko wrote:
> Sure, this will work too. It's a bit less readable due to tuple syntax though:
>
> tuples:
> ((u'constraints', (((u'constraint', ((u'limit', ((u'limit', 10),)),)), (u'name', u'limit')), ((u'constraint', ((u'values', (u'1', u'2')),)), (u'name', u'value')))),)
>
> list/tuples:
> [(u'constraints', [[(u'constraint', [(u'limit', [(u'limit', 10)])]), (u'name', u'limit')], [(u'constraint', [(u'values', [u'1', u'2'])]), (u'name', u'value')]])]
>
> However, I can definitely appreciate code brevity of your approach. Changed.
Hmm. Does unified_diff(pformat(config_in), pformat(config_out)) help? this way it at least pretty prints/indents the repr.
(from pprint import pformat)
- Brian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review31064
-----------------------------------------------------------
On Jan. 2, 2014, 10:09 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 10:09 p.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
> src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16311: Fixing updater diff.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Jan. 2, 2014, 9:11 p.m., Brian Wickman wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, lines 170-191
> > <https://reviews.apache.org/r/16311/diff/3/?file=413507#file413507line170>
> >
> > this seems a bit elaborate. would the following work?
> >
> >
> > import json
> > import pprint
> > import sys
> >
> >
> > def hashable(element):
> > if isinstance(element, (list, set)):
> > return tuple(sorted(hashable(item) for item in element))
> > elif isinstance(element, dict):
> > return tuple(sorted((hashable(key), hashable(value)) for (key, value) in element.items()))
> > return element
> >
> >
> > pprint.pprint(hashable(json.load(sys.stdin)))
> >
> >
> >
> > mba=~=; unzip -p ~/.pants.d/bin/pants.pex/pants-0.0.4-science-py26.pex PEX-INFO | python foo.py
> > ((u'always_write_cache', False),
> > (u'build_properties',
> > ((u'branch', u'git-review/branches/243585'),
> > (u'class', u'CPython'),
> > (u'date', u'Wednesday Dec 04, 2013'),
> > (u'machine', u'ramanujan.uthcode.com'),
> > (u'path', u'/Users/skumaran/twitter/science'),
> > (u'platform', u'macosx-10.4-x86_64'),
> > (u'sha', u'6084f8a289767979535575053a5b26aa8c1fe19b'),
> > (u'tag', u'com.twitter-ibis-thrift-service-4.0.8-24-g6084f8a'),
> > (u'time', u'18:28:38'),
> > (u'timestamp', u'12.04.2013 18:28'),
> > (u'user', u'skumaran'),
> > (u'version', (2, 6, 8)))),
> > (u'egg_caches', ()),
> > (u'entry_point', u'twitter.pants.bin.pants_exe:main'),
> > (u'ignore_errors', False),
> > (u'indices', ()),
> > (u'inherit_path', False),
> > (u'repositories', ()),
> > (u'requirements',
> > ((None, False, u'ansicolors'),
> > (None, False, u'elementtree'),
> > (None, False, u'mako'),
> > (None, False, u'markdown'),
> > (None, False, u'psutil==1.1.2'),
> > (None, False, u'pygments'),
> > (None, False, u'pylint'),
> > (None, False, u'pytest'),
> > (None, False, u'pytest-cov'),
> > (None, False, u'setuptools==1.1.7'))),
> > (u'zip_safe', True))
> >
Sure, this will work too. It's a bit less readable due to tuple syntax though:
tuples:
((u'constraints', (((u'constraint', ((u'limit', ((u'limit', 10),)),)), (u'name', u'limit')), ((u'constraint', ((u'values', (u'1', u'2')),)), (u'name', u'value')))),)
list/tuples:
[(u'constraints', [[(u'constraint', [(u'limit', [(u'limit', 10)])]), (u'name', u'limit')], [(u'constraint', [(u'values', [u'1', u'2'])]), (u'name', u'value')]])]
However, I can definitely appreciate code brevity of your approach. Changed.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review31064
-----------------------------------------------------------
On Jan. 2, 2014, 8:57 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 8:57 p.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
> src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16311: Fixing updater diff.
Posted by Brian Wickman <wi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review31064
-----------------------------------------------------------
src/main/python/twitter/aurora/client/api/updater.py
<https://reviews.apache.org/r/16311/#comment59405>
this seems a bit elaborate. would the following work?
import json
import pprint
import sys
def hashable(element):
if isinstance(element, (list, set)):
return tuple(sorted(hashable(item) for item in element))
elif isinstance(element, dict):
return tuple(sorted((hashable(key), hashable(value)) for (key, value) in element.items()))
return element
pprint.pprint(hashable(json.load(sys.stdin)))
mba=~=; unzip -p ~/.pants.d/bin/pants.pex/pants-0.0.4-science-py26.pex PEX-INFO | python foo.py
((u'always_write_cache', False),
(u'build_properties',
((u'branch', u'git-review/branches/243585'),
(u'class', u'CPython'),
(u'date', u'Wednesday Dec 04, 2013'),
(u'machine', u'ramanujan.uthcode.com'),
(u'path', u'/Users/skumaran/twitter/science'),
(u'platform', u'macosx-10.4-x86_64'),
(u'sha', u'6084f8a289767979535575053a5b26aa8c1fe19b'),
(u'tag', u'com.twitter-ibis-thrift-service-4.0.8-24-g6084f8a'),
(u'time', u'18:28:38'),
(u'timestamp', u'12.04.2013 18:28'),
(u'user', u'skumaran'),
(u'version', (2, 6, 8)))),
(u'egg_caches', ()),
(u'entry_point', u'twitter.pants.bin.pants_exe:main'),
(u'ignore_errors', False),
(u'indices', ()),
(u'inherit_path', False),
(u'repositories', ()),
(u'requirements',
((None, False, u'ansicolors'),
(None, False, u'elementtree'),
(None, False, u'mako'),
(None, False, u'markdown'),
(None, False, u'psutil==1.1.2'),
(None, False, u'pygments'),
(None, False, u'pylint'),
(None, False, u'pytest'),
(None, False, u'pytest-cov'),
(None, False, u'setuptools==1.1.7'))),
(u'zip_safe', True))
- Brian Wickman
On Jan. 2, 2014, 8:57 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 8:57 p.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
> src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16311: Fixing updater diff.
Posted by Brian Wickman <wi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review31077
-----------------------------------------------------------
Ship it!
Ship It!
- Brian Wickman
On Jan. 2, 2014, 10:09 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 10:09 p.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
> src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16311: Fixing updater diff.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/
-----------------------------------------------------------
(Updated Jan. 2, 2014, 10:44 p.m.)
Review request for Aurora and Brian Wickman.
Changes
-------
Removing wfarner from required list per request.
Repository: aurora
Description
-------
Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
Diffs
-----
src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
Diff: https://reviews.apache.org/r/16311/diff/
Testing
-------
./pants src/test/python/twitter/aurora/client/api:updater
Thanks,
Maxim Khutornenko
Re: Review Request 16311: Fixing updater diff.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review31078
-----------------------------------------------------------
Mind booting me from the people line? I'm going to formally bow out of this review.
- Bill Farner
On Jan. 2, 2014, 10:09 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2014, 10:09 p.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
> src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16311: Fixing updater diff.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/
-----------------------------------------------------------
(Updated Jan. 2, 2014, 10:09 p.m.)
Review request for Aurora, Bill Farner and Brian Wickman.
Changes
-------
CR comments.
Repository: aurora
Description
-------
Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
Diffs (updated)
-----
src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
Diff: https://reviews.apache.org/r/16311/diff/
Testing
-------
./pants src/test/python/twitter/aurora/client/api:updater
Thanks,
Maxim Khutornenko
Re: Review Request 16311: Fixing updater diff.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/
-----------------------------------------------------------
(Updated Jan. 2, 2014, 8:57 p.m.)
Review request for Aurora, Bill Farner and Brian Wickman.
Changes
-------
Updated with CR comments.
Repository: aurora
Description
-------
Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
Diffs (updated)
-----
src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
Diff: https://reviews.apache.org/r/16311/diff/
Testing
-------
./pants src/test/python/twitter/aurora/client/api:updater
Thanks,
Maxim Khutornenko
Re: Review Request 16311: Fixing updater diff.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Dec. 24, 2013, 1:13 a.m., Jonathan Boulle wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, line 170
> > <https://reviews.apache.org/r/16311/diff/2/?file=402814#file402814line170>
> >
> > camelcase, ye heathen
Duh.. Thanks, fixed.
> On Dec. 24, 2013, 1:13 a.m., Jonathan Boulle wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, line 180
> > <https://reviews.apache.org/r/16311/diff/2/?file=402814#file402814line180>
> >
> > camelcase
Fixed.
> On Dec. 24, 2013, 1:13 a.m., Jonathan Boulle wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, lines 172-174
> > <https://reviews.apache.org/r/16311/diff/2/?file=402814#file402814line172>
> >
> > what if this passes a list to self._convertDict?
Not sure I understand. This function pair is designed to recursively pass control from _convert_dict to _convert_list and back as many times as the nesting requires. What's your concern here?
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review30845
-----------------------------------------------------------
On Dec. 23, 2013, 6:51 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2013, 6:51 p.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
> src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16311: Fixing updater diff.
Posted by Jonathan Boulle <jo...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review30845
-----------------------------------------------------------
src/main/python/twitter/aurora/client/api/updater.py
<https://reviews.apache.org/r/16311/#comment59044>
camelcase, ye heathen
src/main/python/twitter/aurora/client/api/updater.py
<https://reviews.apache.org/r/16311/#comment59048>
what if this passes a list to self._convertDict?
src/main/python/twitter/aurora/client/api/updater.py
<https://reviews.apache.org/r/16311/#comment59045>
camelcase
- Jonathan Boulle
On Dec. 23, 2013, 6:51 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2013, 6:51 p.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
> src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16311: Fixing updater diff.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/
-----------------------------------------------------------
(Updated Dec. 23, 2013, 6:51 p.m.)
Review request for Aurora, Bill Farner and Brian Wickman.
Changes
-------
This has been a huge time drain on my side as none of the approaches with customizing thrift json serialization or playing with json key sorting has helped to sort out all 3 of the observed issues at once:
- Sets are not equal even though their reprs are identical;
- Set elements are reordered;
- TaskConfig elements are reordered.
The randomness in behavior is due to the combination of python set types being unhashable and the python dicts not providing a stable ordering. The proposed solution here is to get rid of both sets and dicts by converting json-serialized thrift dicts into sorted lists of tuples where every tuple's second element is either a simple value, list or another tuple.
Repository: aurora
Description (updated)
-------
Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to sorted tuples via json serialization.
Diffs (updated)
-----
src/main/python/twitter/aurora/client/api/updater.py 6e8e8a9fdfcf63a8fe0558d5ac36298601fd0552
src/test/python/twitter/aurora/client/api/test_updater.py ae4de2b57be16f8daf030210abda1ba05f9f30a0
Diff: https://reviews.apache.org/r/16311/diff/
Testing
-------
./pants src/test/python/twitter/aurora/client/api:updater
Thanks,
Maxim Khutornenko
Re: Review Request 16311: Fixing updater diff.
Posted by Brian Wickman <wi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16311/#review30557
-----------------------------------------------------------
src/main/python/twitter/aurora/client/api/updater.py
<https://reviews.apache.org/r/16311/#comment58520>
thirdparty imports go last
src/main/python/twitter/aurora/client/api/updater.py
<https://reviews.apache.org/r/16311/#comment58521>
TSerialization takes a little bit of the ceremony out of this:
from thrift.TSerialization import serialize
from thrift.protocol import TJSONProtocol
return serialize(config, protocol_factory=TJSONProtocol.TSimpleJSONProtocolFactory())
- Brian Wickman
On Dec. 17, 2013, 2:34 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16311/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 2:34 a.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Hopefully fixing the thrift object diff once an for all. Instead of sorting __dict__ that proved to not cover all cases, converting thrift to json and sorting json dict keys.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
> src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
>
> Diff: https://reviews.apache.org/r/16311/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/twitter/aurora/client/api:updater
>
>
> Thanks,
>
> Maxim Khutornenko
>
>