Merge lp:~hazmat/pyjuju/rapi-annotation into lp:~hazmat/pyjuju/rapi-rollup

[Kapil Thangavelu]

annotation api for gui and ls

Allow for annotating domain objects with 3rd party metadata for integration
purposes.

https://codereview.appspot.com/6936063/

[Kapil Thangavelu (hazmat)]

Reviewers: mp+140358_code.launchpad.net,

Message:
Please take a look.

Description:
annotation api for gui and ls

Allow for annotating domain objects with 3rd party metadata for
integration
purposes.

https://code.launchpad.net/~hazmat/juju/rapi-annotation/+merge/140358

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6936063/

Affected files:
   A [revision details]
   M juju/rapi/context.py
   M juju/rapi/delta.py
   M juju/rapi/tests/test_context.py
   M juju/rapi/tests/test_delta.py
   A juju/state/annotation.py
   M juju/state/errors.py
   M juju/state/relation.py
   A juju/state/tests/test_annotation.py
   M juju/state/tests/test_topology.py
   M juju/state/topology.py

[Gary Poster (gary)]

Hi Kapil. This will be great to have, thank you.

All my comments are questions/thoughts. Discard if they don't make
sense to you.

I'd like to spend more time reading annotations.py and the
test_annotations.py but I ran out of time. If you haven't landed this
by the time I get more free time, I'll read that some more and see if I
have any more comments.

Gary

https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py#newcode95
juju/rapi/context.py:95: """Annotate an entity by name.
Would be nice to specify what data is (dict AIUI?)

Should keys always be strings? If so, should that be documented? If
not, specifying expected key types might still be nice.

I'm guessing that the dict is merged with the existing data, so this is
also the change annotation API. I wish that were a bit less surprising
in the name. I notice that you use "set" as the verb on the
AnnotationManager. I like that a lot better, FWIW (set_annotation, or,
per discussion below, set_annotations). "set" conveys the semantics
shown here, where "add" does not.

Generally, for all of these, the fact that the API is singular confuses
me. An API for add_annotation I would expect to be
add_annotation(entity, name, value) or change_annotation(entity, name,
map). I would call this API add_annotations(entity, map) or even
change_annotations(entity, map). This "let's use plurals" comment also
applies to get_annotation => get_annotations and remove_annotation =>
remove_annotations.

https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py#newcode121
juju/rapi/context.py:121: """Retrieve annotations for an entity.
by entity, I'm guessing this is name, as with add_annotation. Might as
well clarify it in both doc strings (or neither, if it really should be
understood?)

https://codereview.appspot.com/6936063/diff/1/juju/rapi/delta.py
File juju/rapi/delta.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/rapi/delta.py#newcode104
juju/rapi/delta.py:104: for u in status['services'][s].get('units', ()):
Isn't this inner loop on units potentially problematic at large scale?
I guess we can't jump to the unit somehow without iterating? I'd wonder
if we could change the inner (unexposed) APIs to make this kind of
iteration unnecessary.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py
File juju/state/annotation.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode23
juju/state/annotation.py:23: Annotations are cumalative::
typo: cumulative

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode53
juju/state/annotation.py:53: Annotations are expected to be high volume
of change, as such the
run-on: ...volume of change. As such, the...

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode64
juju/state/annotation.py:64: gc. The gc is soley for space management.
typo: solely

https://codereview.appspot.com/6936063/

[Gary Poster (gary)]

Hi Kapil. I got some time to review annotation.py, with a few comments,
at least one of which I suspect is a bit important (the isdigit).

Thanks

Gary

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py
File juju/state/annotation.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode100
juju/state/annotation.py:100: def _get_key(self, context, topology):
Might be nice to have a docstring that clarified that context was a
string or an integer. And that returning None indicates no match,
maybe?

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode104
juju/state/annotation.py:104: if context.isdigit() or
isinstance(context, int):
I suspect you want to reverse those conditions. Ints don't have an
"isdigit" method.

Ideally, this would suggest a test as well.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode122
juju/state/annotation.py:122: def _get_context(self, key, topology):
maybe clarify in docstring that this is the inverse of _get_key?

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode140
juju/state/annotation.py:140: # json with unicode keys can bypass c ext,
how to delete key
Is this an XXX? It sounds like "how to delete key" is an answered
issue, and I don't understand what the point of the other unicode
comment is, so it could either be expanded or removed.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode146
juju/state/annotation.py:146: return self._dump(store)
Oh! So, this is just a big dict of all annotations, which we load and
dump altogether, right? Huh. I guess this is your point in the module
docstring: this is OK for our use case of annotating services, but
otherwise is potentially problematic in the general case for scaling.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode154
juju/state/annotation.py:154: k = context and (yield
self._verify_context(context))
So...if context is None or an empty string we just use that as the key,
which will return an empty dict. I don't really see the point, but I
don't see a problem either.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode208
juju/state/annotation.py:208: waste = set(used_keys) - set(valid_keys)
I can see a couple of ways to make this a bit more efficient, but
there's probably not much point. The kinds of things I have in mind:
valid_keys could be a set to begin with. You could just define waste as
set(store) - valid_keys and eliminate an intermediate data structure
(used_keys). Probably not worth it. If you like it, take it, and
otherwise ignore it.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode216
juju/state/annotation.py:216: return AnnotationStream(self._client,
self)
We are always making one...interesting. I guess you are thinking of it
like __iter__. Why didn't you just call this method __iter__, in fact?

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode245
juju/state/annotation.py:245: # s-a added, annotated, s-a removed, s-a
added.. in the span
"s-a"? service A?

...

[Kapil Thangavelu (hazmat)]

On 2012/12/18 14:42:58, gary.poster wrote:
> Hi Kapil. This will be great to have, thank you.

> All my comments are questions/thoughts. Discard if they don't make
sense to
> you.

> I'd like to spend more time reading annotations.py and the
test_annotations.py
> but I ran out of time. If you haven't landed this by the time I get
more free
> time, I'll read that some more and see if I have any more comments.

> Gary

> https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py
> File juju/rapi/context.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py#newcode95
> juju/rapi/context.py:95: """Annotate an entity by name.
> Would be nice to specify what data is (dict AIUI?)

> Should keys always be strings? If so, should that be documented? If
not,
> specifying expected key types might still be nice.

at the moments its free form, reasonably keys should always be strings.
i'll add some enforcement of that (in annotation manager). we might end
up restricting to known keys (ls & gui) but i prefer to key it open for
exploration for now. there is enforcement that data is a dict at the
annotation manager level.

> I'm guessing that the dict is merged with the existing data, so this
is also the
> change annotation API. I wish that were a bit less surprising in the
name. I
> notice that you use "set" as the verb on the AnnotationManager. I
like that a
> lot better, FWIW (set_annotation, or, per discussion below,
set_annotations).
> "set" conveys the semantics shown here, where "add" does not.

naming is hard :-)

that's interesting, i changed the name at the exposed api/rapi level
because i had a different i went with add because its clearly
cumulative.. while set might be taken for an equality/total value.

from our irc discussion, i'll go with update_annotations. its a little
ambigious that you update an annotation to create it, but in practice i
think it should be okay.

> Generally, for all of these, the fact that the API is singular
confuses me. An
> API for add_annotation I would expect to be add_annotation(entity,
name, value)
> or change_annotation(entity, name, map).

i eschewed single value forms to avoid round trips. i originally had a
notion of entity, namespace, map but i realized its just adding
complexity when simplicity would serve. our annotations aren't private
to the creator ala zope.annotation, we have cross annotation usage so a
shared space serves nicely, and makes the consumption a bit simpler as
well.

> I would call this API
> add_annotations(entity, map) or even change_annotations(entity, map).
This
> "let's use plurals" comment also applies to get_annotation =>
get_annotations
> and remove_annotation => remove_annotations.

plurals +1

https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py#newcode121
> juju/rapi/context.py:121: """Retrieve annotations for an entity.
> by entity, I'm guessing this is name, as with add_annotation. Might
as well
> clarify it in both doc strings (or neither, if it really should be
understood?)

sounds good re doc string w/ entity def.

> https://codereview.appspot.com/6936063/diff/1/juju/rapi/delta.py
> File juju/rapi/delta.py (righ...

[Kapil Thangavelu (hazmat)]

On 2012/12/18 16:43:15, gary.poster wrote:
> Hi Kapil. I got some time to review annotation.py, with a few
comments, at
> least one of which I suspect is a bit important (the isdigit).

> Thanks

> Gary

> https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py
> File juju/state/annotation.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode100
> juju/state/annotation.py:100: def _get_key(self, context, topology):
> Might be nice to have a docstring that clarified that context was a
string or an
> integer. And that returning None indicates no match, maybe?

sounds reasonable.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode104
> juju/state/annotation.py:104: if context.isdigit() or
isinstance(context, int):
> I suspect you want to reverse those conditions. Ints don't have an
"isdigit"
> method.

> Ideally, this would suggest a test as well.

nice catch, thanks!

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode122
> juju/state/annotation.py:122: def _get_context(self, key, topology):
> maybe clarify in docstring that this is the inverse of _get_key?

sounds good.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode140
> juju/state/annotation.py:140: # json with unicode keys can bypass c
ext, how to
> delete key
> Is this an XXX? It sounds like "how to delete key" is an answered
issue, and I
> don't understand what the point of the other unicode comment is, so it
could
> either be expanded or removed.

yeah.. some of this was written science fiction style, and filled in
later, that how to delete key comment should get yanked. the unicode
comment was referencing a concern i had about being able to ensure the c
json encoder for encode/decode speed but looking through
json/(encoder/decoder) it seems like its not relevant, i also obsolete.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode146
> juju/state/annotation.py:146: return self._dump(store)
> Oh! So, this is just a big dict of all annotations, which we load and
dump
> altogether, right? Huh. I guess this is your point in the module
docstring:
> this is OK for our use case of annotating services, but otherwise is
potentially
> problematic in the general case for scaling.

yeah.. the upper bound in zk is roughly 1mb for the node data,
compressed that gets us to roughly 20k entities with annotations which
is fine. if need be its fairly easy to use a consistent hashing function
on the context/keyspace to partition annotations across multiple node...
there are bigger fish to fry i think though. fwiw here's a sample script
i used for a sanity check (and come up with 20k number) using random
data for the annotations, but constant key set.
http://paste.ubuntu.com/1448006/

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode154
> juju/state/annotation.py:154: k = context and (yield
> self._verify_context(context))
> So...if context is None or an empty string we just use that as the
key, which
> will return an empty dict. I don't really see the point, but I don't
see a
> problem either.
...

[Benjamin Saller (bcsaller)]

Quick review, this looks pretty good and like you said, given the
trajectory of the code base I think this is basically enough for now.
I'd like your thoughts on moving how /annotations are stored (below),
and if its worth observing annotation changes apart from the normal
delta, but I'm basically happy to move forward with this now.

https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py#newcode95
juju/rapi/context.py:95: """Annotate an entity by name.
On 2012/12/18 14:42:58, gary.poster wrote:
> Would be nice to specify what data is (dict AIUI?)

> Should keys always be strings? If so, should that be documented? If
not,
> specifying expected key types might still be nice.

> I'm guessing that the dict is merged with the existing data, so this
is also the
> change annotation API. I wish that were a bit less surprising in the
name. I
> notice that you use "set" as the verb on the AnnotationManager. I
like that a
> lot better, FWIW (set_annotation, or, per discussion below,
set_annotations).
> "set" conveys the semantics shown here, where "add" does not.

> Generally, for all of these, the fact that the API is singular
confuses me. An
> API for add_annotation I would expect to be add_annotation(entity,
name, value)
> or change_annotation(entity, name, map). I would call this API
> add_annotations(entity, map) or even change_annotations(entity, map).
This
> "let's use plurals" comment also applies to get_annotation =>
get_annotations
> and remove_annotation => remove_annotations.

I agree with this comment. Context here is a key entry point into the
API and should be outlined in more detail.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py
File juju/state/annotation.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode100
juju/state/annotation.py:100: def _get_key(self, context, topology):
Will we ever want to annotate relations directly? Juju-Gui's positioning
cases don't depend on this now, but its easy to imaging recording a path
for a manually sculpted relation line. Just something to keep in mind.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode137
juju/state/annotation.py:137: assert isinstance(data, dict), "Invalid
annotation"
An assertion error seems a little harsh here. Client side things haven't
been aggressively typechecked so its possible to pass something that
json can pull out as a dict.

You already have InvalidAnnotationContext, maybe this is another
specific error that can be caught.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode146
juju/state/annotation.py:146: return self._dump(store)
On 2012/12/18 16:43:15, gary.poster wrote:
> Oh! So, this is just a big dict of all annotations, which we load and
dump
> altogether, right? Huh. I guess this is your point in the module
docstring:
> this is OK for our use case of annotating services, but otherwise is
potentially
> problematic in the general case for scaling.

The way the .stream stuff is hooked up I'm not sure we can/should couple
th...

[Kapil Thangavelu (hazmat)]

On 2012/12/19 14:44:36, benjamin.saller wrote:
> Quick review, this looks pretty good and like you said, given the
trajectory of
> the code base I think this is basically enough for now. I'd like your
thoughts
> on moving how /annotations are stored (below), and if its worth
observing
> annotation changes apart from the normal delta, but I'm basically
happy to move
> forward with this now.

> https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py
> File juju/rapi/context.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/rapi/context.py#newcode95
> juju/rapi/context.py:95: """Annotate an entity by name.
> On 2012/12/18 14:42:58, gary.poster wrote:
> > Would be nice to specify what data is (dict AIUI?)
> >
> > Should keys always be strings? If so, should that be documented?
If not,
> > specifying expected key types might still be nice.
> >
> > I'm guessing that the dict is merged with the existing data, so this
is also
> the
> > change annotation API. I wish that were a bit less surprising in the
name. I
> > notice that you use "set" as the verb on the AnnotationManager. I
like that a
> > lot better, FWIW (set_annotation, or, per discussion below,
set_annotations).
> > "set" conveys the semantics shown here, where "add" does not.
> >
> > Generally, for all of these, the fact that the API is singular
confuses me.
> An
> > API for add_annotation I would expect to be add_annotation(entity,
name,
> value)
> > or change_annotation(entity, name, map). I would call this API
> > add_annotations(entity, map) or even change_annotations(entity,
map). This
> > "let's use plurals" comment also applies to get_annotation =>
get_annotations
> > and remove_annotation => remove_annotations.

> I agree with this comment. Context here is a key entry point into the
API and
> should be outlined in more detail.

> https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py
> File juju/state/annotation.py (right):

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode100
> juju/state/annotation.py:100: def _get_key(self, context, topology):
> Will we ever want to annotate relations directly? Juju-Gui's
positioning cases
> don't depend on this now, but its easy to imaging recording a path for
a
> manually sculpted relation line. Just something to keep in mind.

easy enough to add when we need it, till then yagni.

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode137
> juju/state/annotation.py:137: assert isinstance(data, dict), "Invalid
> annotation"
> An assertion error seems a little harsh here. Client side things
haven't been
> aggressively typechecked so its possible to pass something that json
can pull
> out as a dict.

> You already have InvalidAnnotationContext, maybe this is another
specific error
> that can be caught.

switched to stateerror w/ msg

https://codereview.appspot.com/6936063/diff/1/juju/state/annotation.py#newcode146
> juju/state/annotation.py:146: return self._dump(store)
> On 2012/12/18 16:43:15, gary.poster wrote:
> > Oh! So, this is just a big dict of all annotations, which we load
and dump
> > altogether, right? Huh. I guess this is your po...

[Kapil Thangavelu (hazmat)]

Please take a look.

https://codereview.appspot.com/6936063/

[Gary Poster (gary)]

Land as is.

Looks good to me. Thank you.

Gary

https://codereview.appspot.com/6936063/

[Kapil Thangavelu (hazmat)]

Please take a look.

https://codereview.appspot.com/6936063/

=== modified file 'juju/rapi/delta.py'
--- juju/rapi/delta.py	2013-01-10 15:38:31 +0000
+++ juju/rapi/delta.py	2013-01-18 16:59:22 +0000
@@ -59,7 +59,7 @@
         try:
             current = (yield self.context.status())['result']
             self._annotate(
-                current, (yield self.annotation_stream.next()))
+                current, (yield self.annotations.get()))
             yield self.annotations.gc()
         except Exception, e:
             # The delta pump must go on.

=== modified file 'juju/rapi/transport/tests/test_ws.py'
--- juju/rapi/transport/tests/test_ws.py	2012-10-04 15:58:43 +0000
+++ juju/rapi/transport/tests/test_ws.py	2013-01-18 16:59:22 +0000
@@ -139,6 +139,38 @@
         return super(DeltaSocketTest, self).tearDown()
 
     @inlineCallbacks
+    def test_annotations_persist_in_delta(self):
+        yield self.mock_store_charms(["precise/mysql"])
+        self.mocker.replay()
+        yield self.context.deploy("precise/mysql")
+        yield self.context.update_annotations(
+            "mysql", {'gui.x': 21, 'gui.y': 64})
+        yield self.stream_manager.pump()
+        yield self.stream_manager.pump()
+
+        d = self.transport.watch_kv('op', 'delta')
+        self.ws.connectionMade()
+        yield d
+        delta = self.transport.stream[-1]
+        self.assertEqual(
+            delta,
+            {u'op': u'delta',
+             u'result': [
+                 [u'service',
+                  u'add',
+                  {u'annotations': {u'gui.x': 21, u'gui.y': 64},
+                   u'charm': u'cs:precise/mysql-1',
+                   u'id': u'mysql'}],
+                 [u'machine', u'add', {u'id': 0, u'instance-id': u'pending'}],
+                 [u'unit',
+                  u'add',
+                  {u'agent-state': u'pending',
+                   u'id': u'mysql/0',
+                   u'machine': 0,
+                   u'public-address': None}]]}
+        )
+
+    @inlineCallbacks
     def test_delta_stream(self):
         yield self.mock_store_charms(["precise/wordpress", "precise/mysql"])
         self.mocker.replay()
@@ -150,6 +182,7 @@
         # Modify state and send events out.
         yield self.context.deploy("precise/mysql")
         yield self.context.deploy("wordpress")
+
         yield self.stream_manager.pump()
 
         delta = self.transport.stream[-1]