> On Aug. 29, 2016, 11:52 a.m., Joshua Cohen wrote: > > src/main/python/apache/aurora/client/cli/update.py, line 193 > > <https://reviews.apache.org/r/51384/diff/4/?file=1487511#file1487511line193> > > > > Am I missing something? Shouldn't this be a set of `Metadata` structs? > > rather than a set of strings which is what this will currently pass? > > Santhosh Kumar Shanmugham wrote: > You are totally correct. Is there any way to make the python tests do > type-checking? > > This however makes the "assert mock_calls" on mock_api to fail. python's > assert, TestCase.assertEquals and testfixtures.compare all are unable to deal > with Metadata object. Any ideas around this would be welcome. > > Joshua Cohen wrote: > I think what I would recommend is updating `_job_update_request` to take > a dict of metadata key/value pairs and performing the thrift conversion > there. That has the benefit of encapsulating knowledge of the thrift API > within the API client. It also means you should be able to assert on the dict > that's passed in test_supdate? > > That said, simple equality on the generated thrift types should work? E.g. > > $ PEX_INTERPRETER=1 aurora > >>> from gen.apache.aurora.api.ttypes import Metadata > >>> a = Metadata('foo', 'bar') > >>> b = Metadata('foo', 'bar') > >>> a == b > True > >>> c = Metadata('baz', 'qux') > >>> a == c > False > > Santhosh Kumar Shanmugham wrote: > Items of a set are not getting deep equality check. Is there an > assertDeepEquals() ? > > $ PEX_INTERPRETER=1 aurora > >>> from gen.apache.aurora.api.ttypes import Metadata > >>> a = Metadata('foo', 'bar') > >>> b = Metadata('foo', 'bar') > >>> a == b > True > >>> {a} == {b} > False > >>> set([a]) == set([b]) > False > >>> set([a]) == set([a]) > True > > Joshua Cohen wrote: > Ahh, the thrift python compiler did not generate `__hash__` methods until > this commit: > https://github.com/apache/thrift/commit/e841b3dac619a5e5d3523d059d48db1a12e41360, > which does not seem to have made it into an official release yet (not that > it would matter if it did, since we're pinned to an older version of thrift > for reasons I cannot recall at the moment). > > So I think you'd have to iterate the sets to check for equality (which > would be made more complicated by the fact that ordering is not guaranteed). > All the more reason to just bury the set of thrift structs internal to the > API impl and just pass a dict instead?
Done. - Santhosh Kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51384/#review147184 ----------------------------------------------------------- On Aug. 27, 2016, 8:58 p.m., Santhosh Kumar Shanmugham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51384/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2016, 8:58 p.m.) > > > Review request for Aurora, David McLaughlin, Joshua Cohen, and Maxim > Khutornenko. > > > Bugs: AURORA-1711 > https://issues.apache.org/jira/browse/AURORA-1711 > > > Repository: aurora > > > Description > ------- > > Allow custom metadata to be stored with the job updates. This feature > is to help case the reconciliation on job update request on the clients. > > Today when a client's requests are proxied via a middle-man, > if the middle-man times out before the scheduler responds with success, > the client is left the only option of retry. In the case of updates, > since multiple updates are not allowed in parallel, the retries fail, > with INVALID_REQUEST. Although the original update is already accepted > by the scheduler and is in-progress. > > With this feature, clients can reconcile the job updates, > - by marking updates them with a unique id in the metadata field > - scheduler returns the in-progress job update in its response > - client can match the client-generated ids to reconcile its state. > > > Diffs > ----- > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > c5765b70501c101f0535b4eed94e9948c36808f9 > src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java > d2673e6b328cb1e249fbe91d18e0d9e935636eaa > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java > a3b04949f1726f110638e4f93ef45947cdb9e7f8 > > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V008_CreateUpdateMetadataTable.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/db/views/DbJobUpdate.java > 78703e92c932cd5e93ff0b70f2a0b6928f6b4003 > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 929d021a336c6a3438613c9340c84a1096dc9069 > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java > 594bb6219294dcc77d48dcad14e2a6f9caa0c534 > > src/main/java/org/apache/aurora/scheduler/updater/UpdateInProgressException.java > PRE-CREATION > src/main/python/apache/aurora/client/api/__init__.py > 9149c3018ae58d405f284fcbd4076d251ccc8192 > src/main/python/apache/aurora/client/cli/context.py > f1a256a8d09d23d8d4d4ee7d264be0fe376398c4 > src/main/python/apache/aurora/client/cli/options.py > 1245ff15a69a4b4347672f7b556985521e813a00 > src/main/python/apache/aurora/client/cli/update.py > 23aaa2c1b67599420408633733e4581553f7151b > src/main/python/apache/aurora/client/hooks/hooked_api.py > 542f165add0f1d01a782fe6d6089bff3e736fb82 > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml > 8563630a179cb6d1e3b0e22c730ccbfe1c9291e2 > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > a40830fd668aa650c22a48cbc757b45aef27e961 > > src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java > 08530397ff75081bde6f07f9d53317b5486e0da4 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > 779dc302602ae8842084807ca868a491ea99b676 > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java > 04551f17999d742c53dfb1b36286b119b448550e > src/test/python/apache/aurora/client/cli/test_supdate.py > 92fb039fb7d3e368d7c0dfa5ebdb465c7f112416 > src/test/python/apache/aurora/client/cli/util.py > aac9f9c802e4ad1f06cee8cf3f56749760b33af5 > > Diff: https://reviews.apache.org/r/51384/diff/ > > > Testing > ------- > > ./build-support/jenkins/build.sh > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > ./pants test src/test/python/apache/aurora/client/cli:cli > > > Thanks, > > Santhosh Kumar Shanmugham > >