[Findit] Extending versioned_model.py to support versioning multiple entities of the same class.

[Findit] Extending versioned_model.py to support versioning multiple entities of the same class.

Previously, versioned_model.py supports creating a singular versioned entity within Findit, and with each update of that singular entity a new version is created and saved. An example of this is wf_config.py, of which there is only 1 entity at any point that is periodically updated.

This change extends versioned_model.py to support creating multiple entities of the same base type by allowing for a string id for the root. For example, MasterFlakeAnalysis is to be versioned, so would have each instance's root key specified as 'master/builder/build_number/step/test' and the integer_id() component responsible for keeping track of which version number is being represented.

BUG=649396
Committed: https://chromium.googlesource.com/infra/infra/+/b5d534432bbd9697a0fed9ff6b8d1b0f3504fadb

[lijeffrey, 2016-09-15 21:38:02]

Description was changed from

==========
[Findit] Extending versioned_model.py to support versioning multiple entities of
the same class.

BUG=
==========

to

==========
[Findit] Extending versioned_model.py to support versioning multiple entities of
the same class.

Previously, versioned_model.py supports creating a singular versioned entity
within Findit, and with each update of that singular entity a new version is
created and saved. An example of this is wf_config.py, of which there is only 1
entity at any point that is periodically updated.

This change extends versioned_mode.py to support creating multiple entities of
the same base type while only using 1 root model. For example,
MasterFlakeAnalysis is to be versioned, and each has a key
'master/builder/build_number/step/test'. In order to support versioning to keep
track of rerun data, a MasterFlakeAnalysisRoot entity would be created for each
master/builder/build_number/step/test and manage several MasterFlakeAnalysis
entities whose keys are the same as the root's key, but with '/version_number'
appended to keep track.


BUG=
==========

[lijeffrey, 2016-09-15 21:38:02]

lijeffrey@chromium.org changed reviewers:
+ chanli@chromium.org, stgao@chromium.org

[lijeffrey, 2016-09-15 21:38:33]

ptal

[lijeffrey, 2016-09-15 21:39:26]

ptal

[stgao, 2016-09-15 22:39:02]

For children of different parents, I think they could have the same id. Do you
mind a check?

Eg: Key('MFARoot', 'parent1', 'MFA', 1) and Key('MFARoot', 'parent2', 'MFA', 1)
are not equal.

[lijeffrey, 2016-09-16 19:54:17]

On 2016/09/15 22:39:02, stgao (slow) wrote:
> For children of different parents, I think they could have the same id. Do you
> mind a check?
> 
> Eg: Key('MFARoot', 'parent1', 'MFA', 1) and Key('MFARoot', 'parent2', 'MFA',
1)
> are not equal.

Yes, this is possible to have Key('MFARoot', 'parent1', 'MFA', 1) and
Key('MFARoot', 'parent2', 'MFA', 1) be 2 unique entities.

[lijeffrey, 2016-09-21 23:20:38]

ptal

[chanli, 2016-09-22 00:11:25]

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:152: return self.key, False
Based on https://docs.python.org/3/tutorial/errors.html#handling-exceptions :
'The try ... except statement has an optional else clause, which, when present,
must follow all except clauses. It is useful for code that must be executed if
the try clause does not raise an exception. '

In our case, if try clause does not raise an exception, the function will
already complete by the return sentence. So this else clause here will never be
reached, right? Besides, why do we want to set a new key when we've done with
save anyway?

[stgao, 2016-09-22 00:44:01]

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:34: def root_string_id(self):
Why do we care whether the root id is a string or an integer?

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:34: def root_string_id(self):
Also not make it a public property.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:59: # Key(root, version_number) or
Key(kind, string_id). The latter is used
This is the child entity of the root.

Per discussion, this change is not needed. The original code should work as
expected.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:92: def Save(self,
should_try_next_version_number=True):
name nit: too long. How about retry_on_conflict or a shorter one?

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:96: should_try_next_version_number:
Boolean whether or not the next version
Format nit:

Args:
  param (type): comment.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:104: root_string_id =
self.root_string_id
nit: root_string_id is used only once.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:152: return self.key, False
On 2016/09/22 00:11:25, chanli wrote:
> Based on https://docs.python.org/3/tutorial/errors.html#handling-exceptions :
> 'The try ... except statement has an optional else clause, which, when
present,
> must follow all except clauses. It is useful for code that must be executed if
> the try clause does not raise an exception. '
> 
> In our case, if try clause does not raise an exception, the function will
> already complete by the return sentence. So this else clause here will never
be
> reached, right? Besides, why do we want to set a new key when we've done with
> save anyway?

There is a if in line #131. We return only when we succeed in creating and
saving. Otherwise, we need to try the next key.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:168: def _GetRootKey(cls,
string_id=None):
name nit: root_id to be consistent.

[lijeffrey, 2016-09-22 17:30:03]

Description was changed from

==========
[Findit] Extending versioned_model.py to support versioning multiple entities of
the same class.

Previously, versioned_model.py supports creating a singular versioned entity
within Findit, and with each update of that singular entity a new version is
created and saved. An example of this is wf_config.py, of which there is only 1
entity at any point that is periodically updated.

This change extends versioned_mode.py to support creating multiple entities of
the same base type while only using 1 root model. For example,
MasterFlakeAnalysis is to be versioned, and each has a key
'master/builder/build_number/step/test'. In order to support versioning to keep
track of rerun data, a MasterFlakeAnalysisRoot entity would be created for each
master/builder/build_number/step/test and manage several MasterFlakeAnalysis
entities whose keys are the same as the root's key, but with '/version_number'
appended to keep track.


BUG=
==========

to

==========
[Findit] Extending versioned_model.py to support versioning multiple entities of
the same class.

Previously, versioned_model.py supports creating a singular versioned entity
within Findit, and with each update of that singular entity a new version is
created and saved. An example of this is wf_config.py, of which there is only 1
entity at any point that is periodically updated.

This change extends versioned_model.py to support creating multiple entities of
the same base type by allowing for a string id for the root. For example,
MasterFlakeAnalysis is to be versioned, so would have each instance's root key
specified as 'master/builder/build_number/step/test' and the integer_id()
component responsible for keeping track of which version number is being
represented.

BUG=649396
==========

[lijeffrey, 2016-09-22 17:36:15]

Comments addressed, ptal

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:34: def root_string_id(self):
On 2016/09/22 00:44:01, stgao (slow) wrote:
> Also not make it a public property.

Done.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:59: # Key(root, version_number) or
Key(kind, string_id). The latter is used
On 2016/09/22 00:44:01, stgao (slow) wrote:
> This is the child entity of the root.
> 
> Per discussion, this change is not needed. The original code should work as
> expected.

A slight change is actually needed:

When creating a versioned entity with a key specified, such as MFA, the design
of this code gives an initial key before the entity is saved looks like

ndb.Key('MFA', 'master/builder/number/step/test')

once it's saved it then gets assigned a new key:
ndb.Key('MFARoot', 'master/builder/number/step/test', 'MFA', 1)

In the pre-saved version, the integer_id() component of key is None since a
string_id is supplied, but we want that to return 0.

In the post-saved version, an integer_id is assigned as the version number and
the string_id supplied component is assigned to the root key.

Changed to:

return self.key.integer_id() or 0 if self.key else 0

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:92: def Save(self,
should_try_next_version_number=True):
On 2016/09/22 00:44:01, stgao (slow) wrote:
> name nit: too long. How about retry_on_conflict or a shorter one?

Done.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:96: should_try_next_version_number:
Boolean whether or not the next version
On 2016/09/22 00:44:01, stgao (slow) wrote:
> Format nit:
> 
> Args:
>   param (type): comment.

Done.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:104: root_string_id =
self.root_string_id
On 2016/09/22 00:44:01, stgao (slow) wrote:
> nit: root_string_id is used only once.

Done.

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:168: def _GetRootKey(cls,
string_id=None):
On 2016/09/22 00:44:01, stgao (slow) wrote:
> name nit: root_id to be consistent.

Done.

[stgao, 2016-09-23 06:27:14]

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:59: # Key(root, version_number) or
Key(kind, string_id). The latter is used
On 2016/09/22 17:36:14, lijeffrey wrote:
> On 2016/09/22 00:44:01, stgao (slow) wrote:
> > This is the child entity of the root.
> > 
> > Per discussion, this change is not needed. The original code should work as
> > expected.
> 
> A slight change is actually needed:
> 
> When creating a versioned entity with a key specified, such as MFA, the design
> of this code gives an initial key before the entity is saved looks like
> 
> ndb.Key('MFA', 'master/builder/number/step/test')
> 
> once it's saved it then gets assigned a new key:
> ndb.Key('MFARoot', 'master/builder/number/step/test', 'MFA', 1)
> 
> In the pre-saved version, the integer_id() component of key is None since a
> string_id is supplied, but we want that to return 0.
> 
> In the post-saved version, an integer_id is assigned as the version number and
> the string_id supplied component is assigned to the root key.
> 
> Changed to:
> 
> return self.key.integer_id() or 0 if self.key else 0

Should we fix the problem in the subclasses (MFA here) of this VersionedModel
instead?

[stgao, 2016-09-23 16:35:53]

Another question: after we versionize the MasterFlakeAnalysis, do we plan to
show all instances in the dashboard?

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/20001/appengine/findit/model/...
appengine/findit/model/versioned_model.py:59: # Key(root, version_number) or
Key(kind, string_id). The latter is used
On 2016/09/23 06:27:14, stgao (slow) wrote:
> On 2016/09/22 17:36:14, lijeffrey wrote:
> > On 2016/09/22 00:44:01, stgao (slow) wrote:
> > > This is the child entity of the root.
> > > 
> > > Per discussion, this change is not needed. The original code should work
as
> > > expected.
> > 
> > A slight change is actually needed:
> > 
> > When creating a versioned entity with a key specified, such as MFA, the
design
> > of this code gives an initial key before the entity is saved looks like
> > 
> > ndb.Key('MFA', 'master/builder/number/step/test')
> > 
> > once it's saved it then gets assigned a new key:
> > ndb.Key('MFARoot', 'master/builder/number/step/test', 'MFA', 1)
> > 
> > In the pre-saved version, the integer_id() component of key is None since a
> > string_id is supplied, but we want that to return 0.
> > 
> > In the post-saved version, an integer_id is assigned as the version number
and
> > the string_id supplied component is assigned to the root key.
> > 
> > Changed to:
> > 
> > return self.key.integer_id() or 0 if self.key else 0
> 
> Should we fix the problem in the subclasses (MFA here) of this VersionedModel
> instead?

Another option is to add a function here for instance creation, and make it an
API contract between this class and its subclasses.

[stgao, 2016-09-23 16:45:20]

https://codereview.chromium.org/2345093002/diff/60001/appengine/findit/model/...
File appengine/findit/model/test/versioned_model_test.py (right):

https://codereview.chromium.org/2345093002/diff/60001/appengine/findit/model/...
appengine/findit/model/test/versioned_model_test.py:172: # When the call to
SaveData() starts, another transaction had just beat this
SaveData() is an internal implementation in VersionedModel.Save, we don't have
to test it.

https://codereview.chromium.org/2345093002/diff/60001/appengine/findit/model/...
appengine/findit/model/test/versioned_model_test.py:196: def MockSave(*args,
**kwargs):
What we want to test is VersionedModel.Save, why do we mock here though? After
mocking, what's the purpose of this test?

[lijeffrey, 2016-09-23 19:56:09]

comments addressed, ptal

for getting version, I had to keep the check to return self.key.integer_id() or
0, since setting an integer id of 0 will be treated as None.

For eample,

_Entity(key=ndb.Key('_EntityRoot', 'm1', '_Entity', 0)).key will return
ndb.Key('_EntityRoot', 'm1', '_Entity', None)

https://codereview.chromium.org/2345093002/diff/60001/appengine/findit/model/...
File appengine/findit/model/test/versioned_model_test.py (right):

https://codereview.chromium.org/2345093002/diff/60001/appengine/findit/model/...
appengine/findit/model/test/versioned_model_test.py:172: # When the call to
SaveData() starts, another transaction had just beat this
On 2016/09/23 16:45:20, stgao (slow) wrote:
> SaveData() is an internal implementation in VersionedModel.Save, we don't have
> to test it.

This test is to cover the case that a save conflict occurred.
testSaveDataNewVersion... was a very poor choice of naming that I forgot to come
up with a better one for. Renaming to testSaveVersionWithConflict()

https://codereview.chromium.org/2345093002/diff/60001/appengine/findit/model/...
appengine/findit/model/test/versioned_model_test.py:196: def MockSave(*args,
**kwargs):
On 2016/09/23 16:45:20, stgao (slow) wrote:
> What we want to test is VersionedModel.Save, why do we mock here though? After
> mocking, what's the purpose of this test?

This is to test the branch of the code that does the first SetNewKey() and
validates that no other transaction has reserved that proposed key yet. Renamed
this test testSaveNewVersionConflictBeforeSaving

[stgao, 2016-09-24 01:43:12]

lgtm with nits.

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
File appengine/findit/model/test/versioned_model_test.py (right):

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:48:
self.assertIsNone(_Entity.Create('m1').GetVersion())
Why testing instance.GetVersion?

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:72: def
testGetVersionForEntitWithKeyId(self):
typo: Entit -> Entity

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
appengine/findit/model/versioned_model.py:29: Subclass will automatically be
versioned, if use GetVersion() to
Update this to reflect the addition of Create below.
With this change, all subclass should use the Create function to create all
instances instead of using ModelName().

Please also update existing testcases too.

[lijeffrey, 2016-09-24 06:13:15]

Comments addressed, mind a quick last-pass (mostly for the updated testcases)?

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
File appengine/findit/model/test/versioned_model_test.py (right):

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:48:
self.assertIsNone(_Entity.Create('m1').GetVersion())
On 2016/09/24 01:43:11, stgao (slow) wrote:
> Why testing instance.GetVersion?

oops this was supposed to be _Entity.GetVersion('m1')

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:72: def
testGetVersionForEntitWithKeyId(self):
On 2016/09/24 01:43:11, stgao (slow) wrote:
> typo: Entit -> Entity

Done.

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/100001/appengine/findit/model...
appengine/findit/model/versioned_model.py:29: Subclass will automatically be
versioned, if use GetVersion() to
On 2016/09/24 01:43:12, stgao (slow) wrote:
> Update this to reflect the addition of Create below.
> With this change, all subclass should use the Create function to create all
> instances instead of using ModelName().
> 
> Please also update existing testcases too.

Done.

[stgao, 2016-09-24 18:00:37]

lgtm with some more nits.

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
File appengine/findit/model/test/versioned_model_test.py (right):

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:26: self.assertEqual(0,
_Entity().version)
nit: all _Entity() should be changed to _Entity.Create()

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:31: self.assertEqual(1,
_Entity(key=ndb.Key('_Entity', 1))._root_id)
nit: this seems not the key we want. As there is a root, it should be reflected
in the key.

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:143: entity = _Entity()
use Create()

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:159: entity = _Entity()
ditto.

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/versioned_model.py:32: read to read and Save() to write.
typo: "to read to read"

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/versioned_model.py:45: def Create(cls, root_id=None):
After a second thought, root_id is more of a concept for internal
implementation.
From the user code aspect, it is just a key. So maybe rename it to key?

[lijeffrey, 2016-09-26 19:03:53]

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
File appengine/findit/model/test/versioned_model_test.py (right):

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:26: self.assertEqual(0,
_Entity().version)
On 2016/09/24 18:00:37, stgao (slow) wrote:
> nit: all _Entity() should be changed to _Entity.Create()

Done.

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:31: self.assertEqual(1,
_Entity(key=ndb.Key('_Entity', 1))._root_id)
On 2016/09/24 18:00:37, stgao (slow) wrote:
> nit: this seems not the key we want. As there is a root, it should be
reflected
> in the key.

Done.

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:143: entity = _Entity()
On 2016/09/24 18:00:36, stgao (slow) wrote:
> use Create()

Done.

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/test/versioned_model_test.py:159: entity = _Entity()
On 2016/09/24 18:00:37, stgao (slow) wrote:
> ditto.

Done.

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
File appengine/findit/model/versioned_model.py (right):

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/versioned_model.py:32: read to read and Save() to write.
On 2016/09/24 18:00:37, stgao (slow) wrote:
> typo: "to read to read"

Done.

https://codereview.chromium.org/2345093002/diff/120001/appengine/findit/model...
appengine/findit/model/versioned_model.py:45: def Create(cls, root_id=None):
On 2016/09/24 18:00:37, stgao (slow) wrote:
> After a second thought, root_id is more of a concept for internal
> implementation.
> From the user code aspect, it is just a key. So maybe rename it to key?

Done.

[lijeffrey, 2016-09-26 19:04:08]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-09-26 19:04:09]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2345093002/#ps140001
(title: "Fixing nits")

[commit-bot: I haz the power, 2016-09-26 19:04:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 21:30:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 21:30:16]

Try jobs failed on following builders:
  Infra Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3180b3ea487ca810)

[lijeffrey, 2016-09-26 21:31:51]

The CQ bit was checked by lijeffrey@chromium.org

[commit-bot: I haz the power, 2016-09-26 21:31:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 21:43:03]

Description was changed from

==========
[Findit] Extending versioned_model.py to support versioning multiple entities of
the same class.

Previously, versioned_model.py supports creating a singular versioned entity
within Findit, and with each update of that singular entity a new version is
created and saved. An example of this is wf_config.py, of which there is only 1
entity at any point that is periodically updated.

This change extends versioned_model.py to support creating multiple entities of
the same base type by allowing for a string id for the root. For example,
MasterFlakeAnalysis is to be versioned, so would have each instance's root key
specified as 'master/builder/build_number/step/test' and the integer_id()
component responsible for keeping track of which version number is being
represented.

BUG=649396
==========

to

==========
[Findit] Extending versioned_model.py to support versioning multiple entities of
the same class.

Previously, versioned_model.py supports creating a singular versioned entity
within Findit, and with each update of that singular entity a new version is
created and saved. An example of this is wf_config.py, of which there is only 1
entity at any point that is periodically updated.

This change extends versioned_model.py to support creating multiple entities of
the same base type by allowing for a string id for the root. For example,
MasterFlakeAnalysis is to be versioned, so would have each instance's root key
specified as 'master/builder/build_number/step/test' and the integer_id()
component responsible for keeping track of which version number is being
represented.

BUG=649396

Committed:
https://chromium.googlesource.com/infra/infra/+/b5d534432bbd9697a0fed9ff6b8d1...
==========

[commit-bot: I haz the power, 2016-09-26 21:43:05]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/infra/infra/+/b5d534432bbd9697a0fed9ff6b8d1...