Code review - Issue 5541054: Subordinate Services Spechttps://codereview.appspot.com/2012-02-28T01:24:19+00:00rietveld
Message from unknown
2012-01-13T15:14:14+00:00bcsallerurn:md5:0fe6f52161181341f4e53826847634c0
Message from bcsaller@gmail.com
2012-01-13T15:14:17+00:00bcsallerurn:md5:eabed6b0d1b5e2b873c9f938956c1e53
Please take a look.
Message from rogpeppe@gmail.com
2012-01-13T15:58:22+00:00rogurn:md5:92b5133e7bb0dfed8b64af562b849c7a
a few initial minor comments on the implicit-interfaces doc.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst
File docs/source/drafts/implicit-interfaces.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode4
docs/source/drafts/implicit-interfaces.rst:4: Juju has the ability to provide specialized interfaces to any service
The meaning of this sentence is unclear to me.
How about this?
"Every juju service provides an implicit, specialized, interface to any service attempting to relate to it. It requires no specific declarations on the part of the charm author."
Although I don't quite understand why this interface is more "specialized" than any other.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode6
docs/source/drafts/implicit-interfaces.rst:6: on the part of the charm author. Implicit interface allow for
s/interface/interfaces/
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode7
docs/source/drafts/implicit-interfaces.rst:7: interested services to gather broad lifecycle oriented events and very
s/lifecycle oriented/lifecycle-oriented/
i think!
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode8
docs/source/drafts/implicit-interfaces.rst:8: general data about other services with out expecting or requiring any
s/with out/without/
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode20
docs/source/drafts/implicit-interfaces.rst:20: `juju-info` provides name, unit addresses an allows for traditional
s/name, unit addresses an /name and unit addresses, and/
Message from bcsaller@gmail.com
2012-01-13T16:06:15+00:00bcsallerurn:md5:bf0d45f64bb27716d4cfc45cd967a468
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst
File docs/source/drafts/implicit-interfaces.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode4
docs/source/drafts/implicit-interfaces.rst:4: Juju has the ability to provide specialized interfaces to any service
On 2012/01/13 15:58:22, rog wrote:
Removed the first couple sentences based on verbal feedback from Gustavo.
> The meaning of this sentence is unclear to me.
> How about this?
> "Every juju service provides an implicit, specialized, interface to any service
> attempting to relate to it. It requires no specific declarations on the part of
> the charm author."
>
> Although I don't quite understand why this interface is more "specialized" than
> any other.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode6
docs/source/drafts/implicit-interfaces.rst:6: on the part of the charm author. Implicit interface allow for
On 2012/01/13 15:58:22, rog wrote:
> s/interface/interfaces/
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode7
docs/source/drafts/implicit-interfaces.rst:7: interested services to gather broad lifecycle oriented events and very
On 2012/01/13 15:58:22, rog wrote:
> s/lifecycle oriented/lifecycle-oriented/
> i think!
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode8
docs/source/drafts/implicit-interfaces.rst:8: general data about other services with out expecting or requiring any
On 2012/01/13 15:58:22, rog wrote:
> s/with out/without/
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode20
docs/source/drafts/implicit-interfaces.rst:20: `juju-info` provides name, unit addresses an allows for traditional
On 2012/01/13 15:58:22, rog wrote:
> s/name, unit addresses an /name and unit addresses, and/
Done.
Message from kapilt@gmail.com
2012-01-18T18:32:15+00:00hazmaturn:md5:2fee962b9cb52ad3f31a713bc8db1866
Looks good, the implicit-interfaces doc could use a once over, and i wouldn't mind chatting about some of the implementation details as regards the persistence of the subordinate relation, specifically the topology modifications and the persistence of the 1-1 unit association.. Also i didn't see any docs regarding the physical zk layout of the new relation type.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst
File docs/source/drafts/implicit-interfaces.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode4
docs/source/drafts/implicit-interfaces.rst:4: Juju has the ability to provide specialized interfaces to any service
Seems like this could use a nicer intro sentence(s). ie. "Juju services normally only allow for relations based on their charm declared interfaces. Juju also provides for certain specialized interfaces automatically given to any service."
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode7
docs/source/drafts/implicit-interfaces.rst:7: interested services to gather broad lifecycle oriented events and very
i'd drop some of the adjectives here. s/broad/
and s/very general/generic
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode9
docs/source/drafts/implicit-interfaces.rst:9: participation on the part of the providing charm author.
maybe s/participation/work or perhaps more aptly categorized as 'charm modification'
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode12
docs/source/drafts/implicit-interfaces.rst:12: the arrival of this feature. Charms attempting to provide new
present tense, not future participle, Juju reserves ..
s/with the arrival of this/for implicit interfaces.
s/provide new/provide
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode14
docs/source/drafts/implicit-interfaces.rst:14: possible to augment these interfaces in a general way through the
I'd drop possible to augment in a general way, more concrete less abstract... ie.
A charm however may choose to implement hooks for these implicit interfaces to customize the charms behavior using standard hook mechanisms.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode18
docs/source/drafts/implicit-interfaces.rst:18: available from all services.
s/At the time of this writing.
Juju currently provides one implicit interface to all deployed services.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode23
docs/source/drafts/implicit-interfaces.rst:23: default implentation of the provides side.
Specify the values provided/contained by the juju-info relation data.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode28
docs/source/drafts/implicit-interfaces.rst:28: was explicitly requested.
intro The role of the ``juju-info`` implicit interface will adapt dynamically at establishment time based on the expectations of the joined services. For example..
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode29
docs/source/drafts/subordinate-internals.rst:29: file.
The user doc noted this was also a required interface, is that the case?
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode54
docs/source/drafts/subordinate-internals.rst:54:
this looks strange, why are public-addresses being stored in the topology? i thought we walked this back to a new relation type as stated earlier in this doc. its not clear why the related units are being stored here, is that to materialize the 1-1 rel for the unit instances? Couldn't that be materialized directly on the unit state.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode104
docs/source/drafts/subordinate-internals.rst:104: update the mapping using the unit_name of the principal.
This paragraph is a little unclear. Its also unclear why the mapping would best be suited to the topology. The topology stores a user defined state, the association and creation of subordinate units is going to happen both at rel def time, but also within the scope of add_unit_state, and actualized by the unit itself, so storing it on the unit state or a contained node seems more appropriate.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode127
docs/source/drafts/subordinate-internals.rst:127: `juju add-relation` and `juju remove-relation` nust trigger the
s/nust/must
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode129
docs/source/drafts/subordinate-internals.rst:129: watch machinery outlined above. The principal service will deploy a
s/A principal service unit
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode149
docs/source/drafts/subordinate-internals.rst:149: we create a relationship of the new `subordinate-principal` type.
why is it called subordinate-principal instead of just subordinate
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode7
docs/source/drafts/subordinate-services.rst:7: machine with no knowledge of other services deployed onto the same
knowledge or access
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode36
docs/source/drafts/subordinate-services.rst:36:
Nice intro.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode76
docs/source/drafts/subordinate-services.rst:76: unit (using the new `subordinate-principal` relationship type). A
s/proper.. re unit and subordinate unit, i'd add a note that this is as opposed to a standard relation which is defined and actuated between services.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode84
docs/source/drafts/subordinate-services.rst:84: Subordinate services imply changes to how we actualize relations
tense.. imply changes from what?... perhaps make the distinction against standard relations.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode123
docs/source/drafts/subordinate-services.rst:123: Charm Changes
s/Charm Changes/Declaring Subordinate Charms
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode128
docs/source/drafts/subordinate-services.rst:128: metadata is required. Declaring a required interface with `container:
if the label is subordinate for all the docs, sounds like it should just be 'subordinate' for the interface metadata as well instead of introducing a new vocab word 'container'
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode137
docs/source/drafts/subordinate-services.rst:137: Implications for Status
tense ..s/Impliciations for Status/Subordinate status output
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode216
docs/source/drafts/subordinate-services.rst:216: flag.
This information looks like it should go into the internals docs, its not really at the level of end user concern.
Message from bcsaller@gmail.com
2012-01-19T09:00:01+00:00bcsallerurn:md5:3114c55555e6a913457ff581cf83234c
ptal
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst
File docs/source/drafts/implicit-interfaces.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode7
docs/source/drafts/implicit-interfaces.rst:7: interested services to gather broad lifecycle oriented events and very
On 2012/01/18 18:32:15, hazmat wrote:
> i'd drop some of the adjectives here. s/broad/
> and s/very general/generic
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode12
docs/source/drafts/implicit-interfaces.rst:12: the arrival of this feature. Charms attempting to provide new
On 2012/01/18 18:32:15, hazmat wrote:
> present tense, not future participle, Juju reserves ..
>
> s/with the arrival of this/for implicit interfaces.
>
> s/provide new/provide
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode14
docs/source/drafts/implicit-interfaces.rst:14: possible to augment these interfaces in a general way through the
On 2012/01/18 18:32:15, hazmat wrote:
> I'd drop possible to augment in a general way, more concrete less abstract...
> ie.
>
> A charm however may choose to implement hooks for these implicit interfaces to
> customize the charms behavior using standard hook mechanisms.
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode18
docs/source/drafts/implicit-interfaces.rst:18: available from all services.
On 2012/01/18 18:32:15, hazmat wrote:
> s/At the time of this writing.
>
> Juju currently provides one implicit interface to all deployed services.
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode28
docs/source/drafts/implicit-interfaces.rst:28: was explicitly requested.
On 2012/01/18 18:32:15, hazmat wrote:
> intro The role of the ``juju-info`` implicit interface will adapt dynamically at
> establishment time based on the expectations of the joined services. For
> example..
My wording here might still be a little off, but I did update it
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode29
docs/source/drafts/subordinate-internals.rst:29: file.
On 2012/01/18 18:32:15, hazmat wrote:
> The user doc noted this was also a required interface, is that the case?
Yes, I've expanded this section to include both that information and an example interface declaration. Likewise the example is included in the user-oriented docs
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode54
docs/source/drafts/subordinate-internals.rst:54:
On 2012/01/18 18:32:15, hazmat wrote:
> this looks strange, why are public-addresses being stored in the topology? i
> thought we walked this back to a new relation type as stated earlier in this
> doc. its not clear why the related units are being stored here, is that to
> materialize the 1-1 rel for the unit instances? Couldn't that be materialized
> directly on the unit state.
Yes, this was an error, it should have read private-address, as per indicated in ServiceRelationState.add_unit_state where private address is currently stored. It seemed this was a reasonable place to keep the 1-1 mapping as well.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode104
docs/source/drafts/subordinate-internals.rst:104: update the mapping using the unit_name of the principal.
On 2012/01/18 18:32:15, hazmat wrote:
> This paragraph is a little unclear. Its also unclear why the mapping would best
> be suited to the topology. The topology stores a user defined state, the
> association and creation of subordinate units is going to happen both at rel def
> time, but also within the scope of add_unit_state, and actualized by the unit
> itself, so storing it on the unit state or a contained node seems more
> appropriate.
This section doesn't refer to the topology, only the state objects which can manipulate either the normal ZK nodes or the topology. In this case I'd only thought to explore how ZK's top level /relations tree is managed in such a way as to make the 1-1 watches simpler. The title on a previous section did refer to the topology though and that has been changed to "ZooKeeper State".
That said its still very possible these two sections are incorrect and with the additional clarity(?) I've added I hope you can again provide feedback.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode127
docs/source/drafts/subordinate-internals.rst:127: `juju add-relation` and `juju remove-relation` nust trigger the
On 2012/01/18 18:32:15, hazmat wrote:
> s/nust/must
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode129
docs/source/drafts/subordinate-internals.rst:129: watch machinery outlined above. The principal service will deploy a
On 2012/01/18 18:32:15, hazmat wrote:
> s/A principal service unit
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-internals.rst#newcode149
docs/source/drafts/subordinate-internals.rst:149: we create a relationship of the new `subordinate-principal` type.
On 2012/01/18 18:32:15, hazmat wrote:
> why is it called subordinate-principal instead of just subordinate
in reference to client-server, the endpoints then have the roles of subordinate and principal.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode7
docs/source/drafts/subordinate-services.rst:7: machine with no knowledge of other services deployed onto the same
On 2012/01/18 18:32:15, hazmat wrote:
> knowledge or access
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode36
docs/source/drafts/subordinate-services.rst:36:
On 2012/01/18 18:32:15, hazmat wrote:
> Nice intro.
Thanks :)
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode84
docs/source/drafts/subordinate-services.rst:84: Subordinate services imply changes to how we actualize relations
On 2012/01/18 18:32:15, hazmat wrote:
> tense.. imply changes from what?... perhaps make the distinction against
> standard relations.
I think I've improved this
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode123
docs/source/drafts/subordinate-services.rst:123: Charm Changes
On 2012/01/18 18:32:15, hazmat wrote:
> s/Charm Changes/Declaring Subordinate Charms
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode128
docs/source/drafts/subordinate-services.rst:128: metadata is required. Declaring a required interface with `container:
On 2012/01/18 18:32:15, hazmat wrote:
> if the label is subordinate for all the docs, sounds like it should just be
> 'subordinate' for the interface metadata as well instead of introducing a new
> vocab word 'container'
The thought here was that the edge case of two subordinate services talking to the same principal might later need a relationship between themselves that is only actualized within the same container. They wouldn't be subordinate to one another. This is why I advocated scope: container before and I still like that spelling. It seems more future proof. The spelling here was change based on feedback from Gustavo, though given this current version of things he might agree now that subordinate: true is better.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode137
docs/source/drafts/subordinate-services.rst:137: Implications for Status
On 2012/01/18 18:32:15, hazmat wrote:
> tense ..s/Impliciations for Status/Subordinate status output
Done.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-services.rst#newcode216
docs/source/drafts/subordinate-services.rst:216: flag.
On 2012/01/18 18:32:15, hazmat wrote:
> This information looks like it should go into the internals docs, its not really
> at the level of end user concern.
Indeed, this is left over from the first run through the spec and attempted to capture the higher level changes in one place. Its replaced by the other doc and will be removed, sorry
Message from unknown
2012-01-19T10:13:02+00:00bcsallerurn:md5:a88235891f3a9a88fb9879d5daeb54ad
Message from bcsaller@gmail.com
2012-01-19T10:13:05+00:00bcsallerurn:md5:c9ed79156fadbc3a4ea46b0a1f997d22
Please take a look.
Message from n13m3y3r@gmail.com
2012-01-19T15:06:18+00:00niemeyerurn:md5:ee8834da8ca938501d5957938e12e075
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst
File docs/source/drafts/implicit-interfaces.rst (right):
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-interfaces.rst#newcode6
docs/source/drafts/implicit-interfaces.rst:6: on the part of the charm author. Implicit interface allow for
I suggest starting the sentence with "Implicit interfaces ...".
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst
File docs/source/drafts/implicit-interfaces.rst (right):
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode7
docs/source/drafts/implicit-interfaces.rst:7: author.
s/author./author of the other service./
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode10
docs/source/drafts/implicit-interfaces.rst:10: to provide new interfaces in this namespace will trigger an error. A
To put this sentence in context it should look like this:
"""
Implicit interfaces are named after the juju-* reserved
namespace, which must be used only as specified.
"""
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode13
docs/source/drafts/implicit-interfaces.rst:13: mechanisms.
Please drop this sentence. It's not clear what this is really saying, and it doesn't look necessary.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode21
docs/source/drafts/implicit-interfaces.rst:21: specify the values provided by the juju-info relation data.
Please rephrase:
The `juju-info` relation, implicitly provided by all charms, enables the requiring unit to obtain basic details about the related-to unit. The following settings will be implicitly provided by the remote unit in a relation with a `juju-info` interface:
<fill it in>
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode27
docs/source/drafts/implicit-interfaces.rst:27: if the remote role were `client`.
This shouldn't be in user-facing documentation. These are implementation details that we don't talk about, and we probably shouldn't because it's very confusing without the proper background. What's a server/client here? Why isn't subordinate a client since it still "requires"? I suggest dropping that.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode15
docs/source/drafts/subordinate-internals.rst:15: communication to only service units in the same container.
The container relation type is not specific to principal and subordinate. It should be possible to establish a container relation between two subordinate services that live in the same container too.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode19
docs/source/drafts/subordinate-internals.rst:19: principals and subordinate units. The new relation type is constructed
This is too vague to be understandable at this stage, and too specific to be part of the Overview section.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode32
docs/source/drafts/subordinate-internals.rst:32: satisfied. ::
Please drop this from this file. This doesn't qualify as "internals", and is already described in detail in the high-level documented referred to in the introduction.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode45
docs/source/drafts/subordinate-internals.rst:45: units in the same container is efficient. ::
I don't think we need an entirely new relation type to handle the use case, and we don't even need changes in the ZooKeeper storage format.
Container relations are still client/server in the sense that we have two sides: a providing one, and a requiring one. They should also work between two subordinate services that sit within the same container, as described above. The only difference is really that we should only fire events across well defined pairs, and we can tell which pairs these are just by looking at the topology (e.g. in the same way you can tell which services are under which service when you render the status).
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode71
docs/source/drafts/subordinate-internals.rst:71: lifecycle.
That's not necessarily the case. If two subordinate services are linked together with a container relation, they shouldn't cause the service to be deployed. What causes the deployment is the knowledge of where to deploy the service in, and this may occur either by linking a subordinate with a principal, or by linking a subordinate with another subordinate that already has a principal. When the subordinate is linked with a principal, it should also check if any other subordinates are linked to it and should be deployed together as well. It should be an error to attempt to link any of the subordinates in the group to two different principal services.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode75
docs/source/drafts/subordinate-internals.rst:75: subordinate units in the same container.
This should indeed only fire when appropriate, but there's no need for a new relation type.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode82
docs/source/drafts/subordinate-internals.rst:82: satisfied.
Must be adapted.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode95
docs/source/drafts/subordinate-internals.rst:95: running it, i.e, that of the principal's service unit.
"inherit" here is making things less clear than they could be. There's no such thing as an inheritance of settings. What is happening in practice, and why?
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode102
docs/source/drafts/subordinate-internals.rst:102: of.
Sounds good. It should probably be an error to attempt to assign a machine to a subordinate service then.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode111
docs/source/drafts/subordinate-internals.rst:111: update the mapping using the unit_name of the principal.
I won't review the Python-related changes in detail, but this must be adapted as well to consider the previous comments.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode117
docs/source/drafts/subordinate-internals.rst:117: subordinate.
Ditto.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode138
docs/source/drafts/subordinate-internals.rst:138: relationship is added.
Ditto.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode148
docs/source/drafts/subordinate-internals.rst:148: -------------------
This section feels a bit confusing given what's actually going on. There's nothing special about implicit interfaces in this context. I suggest dropping it.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode154
docs/source/drafts/subordinate-internals.rst:154: fallback to an implicit `juju-info` interface. When we detect that we
No fallback seems necessary in relation matching. The implementation of juju-info should put it as a normal interface in all services, and consuming that interface by a subordinate service should be done explicitly.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode161
docs/source/drafts/subordinate-internals.rst:161: this can be added at a later time however.
The existence of juju-info and subordinate services is orthogonal. If you're doing something that requires further action down the road, it's probably not being implemented correctly.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode10
docs/source/drafts/subordinate-services.rst:10: of each other.
This sentence is very good.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode14
docs/source/drafts/subordinate-services.rst:14: -----------------
Please realign the underlining of all headers with the text.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode19
docs/source/drafts/subordinate-services.rst:19: subordinate to their service.
This sentence reads very poorly. I suggest dropping it entirely. The next sentence explains the same idea more clearly.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode25
docs/source/drafts/subordinate-services.rst:25: interface pairing. Requiring specified relationships implies that
s/interface/relation/
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode31
docs/source/drafts/subordinate-services.rst:31:
Please remove one of the empty lines here.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode42
docs/source/drafts/subordinate-services.rst:42: A traditional charm/ service in whose container subordinate
charm/ service => service or charm
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode46
docs/source/drafts/subordinate-services.rst:46: A service designed for and deployed to the running container of
s/service/service or charm/
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode47
docs/source/drafts/subordinate-services.rst:47: another service.
s/service/service unit/
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode49
docs/source/drafts/subordinate-services.rst:49: Subordinate Relation
s/Rel/rel/, since the other two are lowercase too.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode52
docs/source/drafts/subordinate-services.rst:52: traditional relationships juju only implements the relationship
s/juju/, juju/
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode78
docs/source/drafts/subordinate-services.rst:78: relations established with any unit in its environment as usual.
This section is problematic. You have to keep in mind that thus far the user never heard about subordinate units before. He's got no idea about how to tag such a unit as a subordinate unit, and doesn't know what "subordinate-principal" relationship type is either. To the user, you just said "Here is a subordinate deployment", and showed commands that he's used to but that just because you say, it's doing something magic that he doesn't understand. Which of those services/charm are subordinate? Why are you using the same command set? Why and how does it work differently?
I suggest moving that to the end of this file, and removing the "subordinate-principal relationship type" mention altogether (the term doesn't exist in this file).
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode96
docs/source/drafts/subordinate-services.rst:96: traditional way.
It is actually not different from traditional relationships in that way, because "this" is a description of how subordinate services are deployed, which works fine with traditional relationships too.
Please use this in place of the above paragraph:
"""
When a traditional relation is added between two services,
all the service units for the first service will receive
relation events about all service units for the second
service. Subordinate services have a very tight relationship
with their principal service, so it makes sense to be able
to restrict that communication in some cases so that they
only receive events about each other. That's precisely what
happens when a relation is tagged as being a scoped for the
container.
"""
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode98
docs/source/drafts/subordinate-services.rst:98: Subordinate relations exist because they simplify responsibilities for
s/Subordinate/Container/
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode103
docs/source/drafts/subordinate-services.rst:103: properly (and the defining need for subordinate services).
Please drop the sentence starting in "This change". It's not clear what it means, and it's not necessary.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode109
docs/source/drafts/subordinate-services.rst:109: aware of the connection.
I don't see why that'd be the case. It could just ask the principal service what state it is in through the container relation.
It'd be more clear with something like:
"""
If a subordinate service needs to communicate with all
units of the principal service, it can still establish a
traditional (non-container) relationship to it.
"""
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode122
docs/source/drafts/subordinate-services.rst:122: principal defines the network setup.
Please drop the sentence starting in "This works because". It will work because it's implemented properly, but we don't have to tell all the small details about how we did it in user-facing documentation.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode132
docs/source/drafts/subordinate-services.rst:132: in a subordinate deployment. Subordinate services can still declare
There's a mismatch here. Your argument to have *container* relations (which I agreed with) was that they are a more general concept. But here you're saying that the way to mark a subordinate service as such is simply by adding a container relation. This feels improper. If container relations are a concept on their own, they should stand on their own in the declaration too, and we should mark a subordinate service as such by having a top-level "subordinate: true" setting. Otherwise you're restricting the existence of container relations between principal and subordinate.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode135
docs/source/drafts/subordinate-services.rst:135: satisfied.
Please reword the sentence starting in "Subordinate services" as:
"""
Subordinate services may still declare traditional
relations to any service. The deployment is delayed
until a container relation is added.
"""
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode137
docs/source/drafts/subordinate-services.rst:137: The example below shows adding a contained relationship to a charm. ::
s/contained relationship/container relation/
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode149
docs/source/drafts/subordinate-services.rst:149: Status reports subordinates under each existing `units` entry of each
Please reword as:
"""
The status output contains details about subordinate units
under the status of the principal service unit that it is
sharing the container with.
"""
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode150
docs/source/drafts/subordinate-services.rst:150: service. The subordinates unit output matches the formatting of
s/subordinates/subordinate's/
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode152
docs/source/drafts/subordinate-services.rst:152: `subordinates` from its output (those are all inherited from the
s/are all inherited from/are the same as/
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode156
docs/source/drafts/subordinate-services.rst:156: dictionary in an abbreviated form. The `subordinate: true` indicator
Rather than subordinate: true, we can be more helpful by saying, e.g.:
subordinate-to: [ wordpress ]
It should be listed even if there are no services related to yet, as:
subordinate-to: []
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode158
docs/source/drafts/subordinate-services.rst:158: relationships showing only any possible relations between this service
Why? The relation information continues to be interesting, and it's very awkward to say "add-relation A B" and then see that A has an empty relation list. Let's please keep the relation value as usual for subordinate services.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode161
docs/source/drafts/subordinate-services.rst:161: unit names under which instances of this service are found.
Let's please drop the sentence and the described content from the status. Imagine how this will end up with:
units: [wordpress/0, wordpress/1, ..., wordpress/42].
The same information is conveyed by the suggested:
subordinate-to: [ wordpress ]
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode167
docs/source/drafts/subordinate-services.rst:167: charm local:series/logging-1
Missing colon.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode168
docs/source/drafts/subordinate-services.rst:168: subordinate: true
subordinate-to: ...
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode170
docs/source/drafts/subordinate-services.rst:170: units: [ wordpress/0]
Let's drop that.
Message from bcsaller@gmail.com
2012-01-22T14:27:41+00:00bcsallerurn:md5:07be12ce64a298a141e159f4b6843214
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst
File docs/source/drafts/implicit-interfaces.rst (right):
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode7
docs/source/drafts/implicit-interfaces.rst:7: author.
On 2012/01/19 15:06:18, niemeyer wrote:
> s/author./author of the other service./
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode10
docs/source/drafts/implicit-interfaces.rst:10: to provide new interfaces in this namespace will trigger an error. A
On 2012/01/19 15:06:18, niemeyer wrote:
> To put this sentence in context it should look like this:
>
> """
> Implicit interfaces are named after the juju-* reserved
> namespace, which must be used only as specified.
> """
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode13
docs/source/drafts/implicit-interfaces.rst:13: mechanisms.
On 2012/01/19 15:06:18, niemeyer wrote:
> Please drop this sentence. It's not clear what this is really saying, and it
> doesn't look necessary.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode21
docs/source/drafts/implicit-interfaces.rst:21: specify the values provided by the juju-info relation data.
On 2012/01/19 15:06:18, niemeyer wrote:
> Please rephrase:
>
> The `juju-info` relation, implicitly provided by all charms, enables the
> requiring unit to obtain basic details about the related-to unit. The following
> settings will be implicitly provided by the remote unit in a relation with a
> `juju-info` interface:
>
> <fill it in>
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-interfaces.rst#newcode27
docs/source/drafts/implicit-interfaces.rst:27: if the remote role were `client`.
On 2012/01/19 15:06:18, niemeyer wrote:
> This shouldn't be in user-facing documentation. These are implementation details
> that we don't talk about, and we probably shouldn't because it's very confusing
> without the proper background. What's a server/client here? Why isn't
> subordinate a client since it still "requires"? I suggest dropping that.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode148
docs/source/drafts/subordinate-internals.rst:148: -------------------
On 2012/01/19 15:06:18, niemeyer wrote:
> This section feels a bit confusing given what's actually going on. There's
> nothing special about implicit interfaces in this context. I suggest dropping
> it.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-internals.rst#newcode161
docs/source/drafts/subordinate-internals.rst:161: this can be added at a later time however.
On 2012/01/19 15:06:18, niemeyer wrote:
> The existence of juju-info and subordinate services is orthogonal. If you're
> doing something that requires further action down the road, it's probably not
> being implemented correctly.
We talked about this on G+ and came to some conclusions reflected in the updated specs.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode14
docs/source/drafts/subordinate-services.rst:14: -----------------
On 2012/01/19 15:06:18, niemeyer wrote:
> Please realign the underlining of all headers with the text.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode19
docs/source/drafts/subordinate-services.rst:19: subordinate to their service.
On 2012/01/19 15:06:18, niemeyer wrote:
> This sentence reads very poorly. I suggest dropping it entirely. The next
> sentence explains the same idea more clearly.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode25
docs/source/drafts/subordinate-services.rst:25: interface pairing. Requiring specified relationships implies that
On 2012/01/19 15:06:18, niemeyer wrote:
> s/interface/relation/
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode31
docs/source/drafts/subordinate-services.rst:31:
On 2012/01/19 15:06:18, niemeyer wrote:
> Please remove one of the empty lines here.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode42
docs/source/drafts/subordinate-services.rst:42: A traditional charm/ service in whose container subordinate
On 2012/01/19 15:06:18, niemeyer wrote:
> charm/ service => service or charm
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode46
docs/source/drafts/subordinate-services.rst:46: A service designed for and deployed to the running container of
On 2012/01/19 15:06:18, niemeyer wrote:
> s/service/service or charm/
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode47
docs/source/drafts/subordinate-services.rst:47: another service.
On 2012/01/19 15:06:18, niemeyer wrote:
> s/service/service unit/
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode49
docs/source/drafts/subordinate-services.rst:49: Subordinate Relation
On 2012/01/19 15:06:18, niemeyer wrote:
> s/Rel/rel/, since the other two are lowercase too.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode52
docs/source/drafts/subordinate-services.rst:52: traditional relationships juju only implements the relationship
On 2012/01/19 15:06:18, niemeyer wrote:
> s/juju/, juju/
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode78
docs/source/drafts/subordinate-services.rst:78: relations established with any unit in its environment as usual.
On 2012/01/19 15:06:18, niemeyer wrote:
> This section is problematic. You have to keep in mind that thus far the user
> never heard about subordinate units before. He's got no idea about how to tag
> such a unit as a subordinate unit, and doesn't know what "subordinate-principal"
> relationship type is either. To the user, you just said "Here is a subordinate
> deployment", and showed commands that he's used to but that just because you
> say, it's doing something magic that he doesn't understand. Which of those
> services/charm are subordinate? Why are you using the same command set? Why and
> how does it work differently?
>
> I suggest moving that to the end of this file, and removing the
> "subordinate-principal relationship type" mention altogether (the term doesn't
> exist in this file).
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode96
docs/source/drafts/subordinate-services.rst:96: traditional way.
On 2012/01/19 15:06:18, niemeyer wrote:
> It is actually not different from traditional relationships in that way, because
> "this" is a description of how subordinate services are deployed, which works
> fine with traditional relationships too.
>
> Please use this in place of the above paragraph:
>
> """
> When a traditional relation is added between two services,
> all the service units for the first service will receive
> relation events about all service units for the second
> service. Subordinate services have a very tight relationship
> with their principal service, so it makes sense to be able
> to restrict that communication in some cases so that they
> only receive events about each other. That's precisely what
> happens when a relation is tagged as being a scoped for the
> container.
> """
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode109
docs/source/drafts/subordinate-services.rst:109: aware of the connection.
On 2012/01/19 15:06:18, niemeyer wrote:
> I don't see why that'd be the case. It could just ask the principal service what
> state it is in through the container relation.
>
> It'd be more clear with something like:
>
> """
> If a subordinate service needs to communicate with all
> units of the principal service, it can still establish a
> traditional (non-container) relationship to it.
> """
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode122
docs/source/drafts/subordinate-services.rst:122: principal defines the network setup.
On 2012/01/19 15:06:18, niemeyer wrote:
> Please drop the sentence starting in "This works because". It will work because
> it's implemented properly, but we don't have to tell all the small details about
> how we did it in user-facing documentation.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode132
docs/source/drafts/subordinate-services.rst:132: in a subordinate deployment. Subordinate services can still declare
On 2012/01/19 15:06:18, niemeyer wrote:
> There's a mismatch here. Your argument to have *container* relations (which I
> agreed with) was that they are a more general concept. But here you're saying
> that the way to mark a subordinate service as such is simply by adding a
> container relation. This feels improper. If container relations are a concept on
> their own, they should stand on their own in the declaration too, and we should
> mark a subordinate service as such by having a top-level "subordinate: true"
> setting. Otherwise you're restricting the existence of container relations
> between principal and subordinate.
My explination depended on the notion of actually required relations, a required container relation was the same as a subordinate charm. Given that we are not attempting to implement proper required relations in this cycle adding subordinate true (or checking optional: False) on the relation in this specific case would both work. Preferences welcome.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode135
docs/source/drafts/subordinate-services.rst:135: satisfied.
On 2012/01/19 15:06:18, niemeyer wrote:
> Please reword the sentence starting in "Subordinate services" as:
>
> """
> Subordinate services may still declare traditional
> relations to any service. The deployment is delayed
> until a container relation is added.
> """
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode137
docs/source/drafts/subordinate-services.rst:137: The example below shows adding a contained relationship to a charm. ::
On 2012/01/19 15:06:18, niemeyer wrote:
> s/contained relationship/container relation/
How we update the example below, to include either subordinate: true at the top level or optional: False to the relation will change the example.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode149
docs/source/drafts/subordinate-services.rst:149: Status reports subordinates under each existing `units` entry of each
On 2012/01/19 15:06:18, niemeyer wrote:
> Please reword as:
>
> """
> The status output contains details about subordinate units
> under the status of the principal service unit that it is
> sharing the container with.
> """
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode152
docs/source/drafts/subordinate-services.rst:152: `subordinates` from its output (those are all inherited from the
On 2012/01/19 15:06:18, niemeyer wrote:
> s/are all inherited from/are the same as/
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode156
docs/source/drafts/subordinate-services.rst:156: dictionary in an abbreviated form. The `subordinate: true` indicator
On 2012/01/19 15:06:18, niemeyer wrote:
> Rather than subordinate: true, we can be more helpful by saying, e.g.:
>
> subordinate-to: [ wordpress ]
>
> It should be listed even if there are no services related to yet, as:
>
> subordinate-to: []
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode158
docs/source/drafts/subordinate-services.rst:158: relationships showing only any possible relations between this service
On 2012/01/19 15:06:18, niemeyer wrote:
> Why? The relation information continues to be interesting, and it's very awkward
> to say "add-relation A B" and then see that A has an empty relation list. Let's
> please keep the relation value as usual for subordinate services.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode161
docs/source/drafts/subordinate-services.rst:161: unit names under which instances of this service are found.
On 2012/01/19 15:06:18, niemeyer wrote:
> Let's please drop the sentence and the described content from the status.
> Imagine how this will end up with:
>
> units: [wordpress/0, wordpress/1, ..., wordpress/42].
>
> The same information is conveyed by the suggested:
>
> subordinate-to: [ wordpress ]
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode167
docs/source/drafts/subordinate-services.rst:167: charm local:series/logging-1
On 2012/01/19 15:06:18, niemeyer wrote:
> Missing colon.
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode168
docs/source/drafts/subordinate-services.rst:168: subordinate: true
On 2012/01/19 15:06:18, niemeyer wrote:
> subordinate-to: ...
Done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode170
docs/source/drafts/subordinate-services.rst:170: units: [ wordpress/0]
On 2012/01/19 15:06:18, niemeyer wrote:
> Let's drop that.
Done.
Message from unknown
2012-01-22T15:20:31+00:00bcsallerurn:md5:e713a6c20d65a5e54fcec688605ae118
Message from bcsaller@gmail.com
2012-01-22T15:20:35+00:00bcsallerurn:md5:ae6c152ecf847e8ba0875683d154f479
Please take a look.
Message from n13m3y3r@gmail.com
2012-01-23T14:49:41+00:00niemeyerurn:md5:1a65ed09ca3a31fc17fd69ccc89f3bb6
Good changes overall, but you've missed comments for a whole file in the review (the internals spec).
Even then, to speed things up some further feedback on your current changes follows.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode46
docs/source/drafts/subordinate-services.rst:46: A service designed for and deployed to the running container of
On 2012/01/22 14:27:41, bcsaller wrote:
> On 2012/01/19 15:06:18, niemeyer wrote:
> > s/service/service or charm/
>
> Done.
It wasn't actually done.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode132
docs/source/drafts/subordinate-services.rst:132: in a subordinate deployment. Subordinate services can still declare
> My explination depended on the notion of actually required relations, a required
> container relation was the same as a subordinate charm. Given that we are not
> attempting to implement proper required relations in this cycle adding
> subordinate true (or checking optional: False) on the relation in this specific
> case would both work. Preferences welcome.
Even ignoring the fact that we can't write text that depends on required relations now, since they don't exist, required relations would still not solve our problem in this case. If a container relation is to be a generic concept, imagine someone decides to tag a relation as "scope: container" in the *principal* side. There's nothing wrong with that, and it's just a nice hint to the system that the principal service explicitly states that this relation only makes sense within the container, and it's not willing to talk to remotes about it. The principal shouldn't become a subordinate because it's got such a relation.
The way to go is what someone (Kapil?) suggested earlier:
subordinate: true
At the top level. Explicit and unambiguous. If we find a "subordinate: true" without a "scope: container" relation, though, then we should error out because clearly that was a mistake.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordinate-services.rst#newcode137
docs/source/drafts/subordinate-services.rst:137: The example below shows adding a contained relationship to a charm. ::
> How we update the example below, to include either subordinate: true at the top
> level or optional: False to the relation will change the example.
See comment above.
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst
File docs/source/drafts/implicit-relations.rst (right):
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst#newcode2
docs/source/drafts/implicit-relations.rst:2: -------------------
Please realign the underlining.
Also, please check all headers and replace them by "Phrase casing". IIRC, I did a pass a while ago to standardize on that format in all documentation, to avoid the several styles we had mixed up.
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst#newcode17
docs/source/drafts/implicit-relations.rst:17: `juju-info`, specified as ::
Let's clarify a bit:
`juju-info`, which if specified (it must not be) would look like::
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst#newcode28
docs/source/drafts/implicit-relations.rst:28: private-address
What about the public-address?
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst#newcode35
docs/source/drafts/implicit-relations.rst:35: Support logging is a :doc:`subordinate charm<subordinate-services>`
s/Support/Suppose/?
Also, s/logging/rsyslog, to avoid mixing up relation name and charm name below.
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst#newcode54
docs/source/drafts/implicit-relations.rst:54: juju add-relation wordpress logging
s/logging/rsyslog/
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst#newcode57
docs/source/drafts/implicit-relations.rst:57: interface we can safely fallback to using the less-specific (in the
s/we can safely/juju will/
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-scope.rst
File docs/source/drafts/relation-scope.rst (right):
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-scope.rst#newcode2
docs/source/drafts/relation-scope.rst:2: --------------
This doesn't look like a good fit for its own file. Where are we defining the rest of the relation fields? This should be placed there and formatted similarly to the other fields.
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-scope.rst#newcode17
docs/source/drafts/relation-scope.rst:17: containers. See :doc:`subordinate-services`.
This isn't entirely clear/correct. Relations are added between services. Service units have containers. Services themselves do not have containers. You can add a relation between a principal service and a subordinate service that currently have no units sharing containers, and that will cause the system to deploy the subordinate service unit onto the principal service unit's container. On the other hand, two principals cannot have container relationships established between them.
I'm not sure how much of that needs to be explained here. I'd suggest keeping the explanation light, putting more details onto the spec itself, and referring to it from here as already being done.
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/subordinate-services.rst#newcode131
docs/source/drafts/subordinate-services.rst:131: relations: {}
This looks bogus. There's a relation in the example.
Message from unknown
2012-01-26T19:51:15+00:00bcsallerurn:md5:bd596bbdde753b980282e54c6c227cf3
Message from bcsaller@gmail.com
2012-01-26T19:51:19+00:00bcsallerurn:md5:1a95fbc1ac057858e785e1e18f58615a
Please take a look.
Message from bcsaller@gmail.com
2012-01-26T21:01:19+00:00bcsallerurn:md5:7b9f1ef83d26ca3d309e3599c9391218
forgot to mail this on the last lbox propose
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst
File docs/source/drafts/implicit-relations.rst (right):
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst#newcode28
docs/source/drafts/implicit-relations.rst:28: private-address
On 2012/01/23 14:49:41, niemeyer wrote:
> What about the public-address?
Included. I was copying from the ZK state but this is added in the protocol handler so I missed it.
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-scope.rst
File docs/source/drafts/relation-scope.rst (right):
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-scope.rst#newcode2
docs/source/drafts/relation-scope.rst:2: --------------
On 2012/01/23 14:49:41, niemeyer wrote:
> This doesn't look like a good fit for its own file. Where are we defining the
> rest of the relation fields? This should be placed there and formatted similarly
> to the other fields.
Yeah, I was keeping it here to keep it in drafts. Moved to charm.rst in the top level
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-scope.rst#newcode17
docs/source/drafts/relation-scope.rst:17: containers. See :doc:`subordinate-services`.
On 2012/01/23 14:49:41, niemeyer wrote:
> This isn't entirely clear/correct. Relations are added between services. Service
> units have containers. Services themselves do not have containers. You can add a
> relation between a principal service and a subordinate service that currently
> have no units sharing containers, and that will cause the system to deploy the
> subordinate service unit onto the principal service unit's container. On the
> other hand, two principals cannot have container relationships established
> between them.
>
> I'm not sure how much of that needs to be explained here. I'd suggest keeping
> the explanation light, putting more details onto the spec itself, and referring
> to it from here as already being done.
I only say that you can't container related principals now.
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/subordinate-services.rst#newcode131
docs/source/drafts/subordinate-services.rst:131: relations: {}
On 2012/01/23 14:49:41, niemeyer wrote:
> This looks bogus. There's a relation in the example.
Indeed. Good catch
Message from n13m3y3r@gmail.com
2012-02-06T19:46:19+00:00niemeyerurn:md5:8af16d32b94cf45f1cca76dc0931c448
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst
File docs/source/drafts/implicit-relations.rst (right):
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-relations.rst#newcode17
docs/source/drafts/implicit-relations.rst:17: `juju-info`, specified as ::
On 2012/01/23 14:49:41, niemeyer wrote:
> Let's clarify a bit:
>
> `juju-info`, which if specified (it must not be) would look like::
This wasn't clarified. You can use a different wording if you want, but we have to make clear that the example is *not* to be used/specified, but rather just informative.
https://codereview.appspot.com/5541054/diff/15003/docs/source/charm.rst
File docs/source/charm.rst (right):
https://codereview.appspot.com/5541054/diff/15003/docs/source/charm.rst#newcode89
docs/source/charm.rst:89: :doc:`subordinate-services`.
Very nice, thanks.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit-relations.rst
File docs/source/drafts/implicit-relations.rst (right):
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit-relations.rst#newcode3
docs/source/drafts/implicit-relations.rst:3: ====================
Let's please try to use a single header convention for our specifications: === for top headers, --- for inner ones. Please see the other files to make sure.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit-relations.rst#newcode42
docs/source/drafts/implicit-relations.rst:42: specified (it must not be) would look like ::
That's wrong. You've got the wrong context for the comment. This juju-info may actually be specified.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordinate-internals.rst#newcode3
docs/source/drafts/subordinate-internals.rst:3: ============================================
Header needs updating too.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordinate-internals.rst#newcode33
docs/source/drafts/subordinate-internals.rst:33: subordinate: subordinate_id-000000002
That's the settings node, and is charm-modifiable. We can't depend on that data to implement internal relation establishment logic. Let's talk about this live.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordinate-internals.rst#newcode61
docs/source/drafts/subordinate-internals.rst:61: relations are not deployed until those relationships are satisfied.
If a service has multiple container relationships, it doesn't need to satisfy all of them for the charm to be deployed.
Message from unknown
2012-02-07T07:54:02+00:00bcsallerurn:md5:c000947b41b2d1ac47a4854f0181c37b
Message from bcsaller@gmail.com
2012-02-07T07:54:04+00:00bcsallerurn:md5:4167a7e8b82dcbf062881ec5829b4922
Please take a look.
Message from bcsaller@gmail.com
2012-02-07T08:14:03+00:00bcsallerurn:md5:d0eb8a1d33c4d44be42d79329b29b5cd
https://codereview.appspot.com/5541054/diff/15003/docs/source/charm.rst
File docs/source/charm.rst (right):
https://codereview.appspot.com/5541054/diff/15003/docs/source/charm.rst#newcode89
docs/source/charm.rst:89: :doc:`subordinate-services`.
On 2012/02/06 19:46:19, niemeyer wrote:
> Very nice, thanks.
thanks
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit-relations.rst
File docs/source/drafts/implicit-relations.rst (right):
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit-relations.rst#newcode3
docs/source/drafts/implicit-relations.rst:3: ====================
On 2012/02/06 19:46:19, niemeyer wrote:
> Let's please try to use a single header convention for our specifications: ===
> for top headers, --- for inner ones. Please see the other files to make sure.
Done.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit-relations.rst#newcode42
docs/source/drafts/implicit-relations.rst:42: specified (it must not be) would look like ::
On 2012/02/06 19:46:19, niemeyer wrote:
> That's wrong. You've got the wrong context for the comment. This juju-info may
> actually be specified.
Oh, I see what I did, fixed above.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordinate-internals.rst#newcode3
docs/source/drafts/subordinate-internals.rst:3: ============================================
On 2012/02/06 19:46:19, niemeyer wrote:
> Header needs updating too.
Done.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordinate-internals.rst#newcode33
docs/source/drafts/subordinate-internals.rst:33: subordinate: subordinate_id-000000002
This has been updated based on the G+ talk we had.
On 2012/02/06 19:46:19, niemeyer wrote:
> That's the settings node, and is charm-modifiable. We can't depend on that data
> to implement internal relation establishment logic. Let's talk about this live.
https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordinate-internals.rst#newcode61
docs/source/drafts/subordinate-internals.rst:61: relations are not deployed until those relationships are satisfied.
On 2012/02/06 19:46:19, niemeyer wrote:
> If a service has multiple container relationships, it doesn't need to satisfy
> all of them for the charm to be deployed.
Done.
Message from unknown
2012-02-09T03:40:04+00:00bcsallerurn:md5:276587afefd478c9020f85dd2adad6ce
Message from bcsaller@gmail.com
2012-02-09T03:40:06+00:00bcsallerurn:md5:bdcf03a592e08def06706dca0f058dbe
Please take a look.
Message from n13m3y3r@gmail.com
2012-02-10T00:20:39+00:00niemeyerurn:md5:cb951ed798ecd4556cf51ac379865024
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/implicit-relations.rst
File docs/source/drafts/implicit-relations.rst (right):
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/implicit-relations.rst#newcode59
docs/source/drafts/implicit-relations.rst:59: provides less information) `juju-info` interface.
This is the opposite of what we agreed, I think. Doing that would mean no charm containing a juju-info relation will have auto-resolution working properly. It must resolve to logging-directory, unless juju-info is explicitly requested in the command line.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst#newcode30
docs/source/drafts/subordinate-internals.rst:30: container_relations/
s/_/-/
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst#newcode43
docs/source/drafts/subordinate-internals.rst:43: indented under it.. Elements preceded by **|** represent a YAML
s/.././
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst#newcode44
docs/source/drafts/subordinate-internals.rst:44: encoded structure under the preceding ZK node. `container_relations`
s/under/within/
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst#newcode61
docs/source/drafts/subordinate-internals.rst:61: service-00000002: {name: name, role: role}
Let's take the chance to update this, as we discussed online:
relations
relation-00000001:
interface: <name>
scope: <name>
services:
service-00000001: {name: name, role: role}
service-00000002: {name: name, role: role}
Please mention that this change to the topology had bumped the VERSION to 2.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst#newcode80
docs/source/drafts/subordinate-internals.rst:80: have been satisfied.
As we discussed online, adding a "scope: container" relation isn't necessarily going to deploy a charm, since it may be established between two subordinate charms.
What causes the deployment of the subordinate charm is adding a "scope: container" relation with a "subordinate: false" charm.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-services.rst#newcode99
docs/source/drafts/subordinate-services.rst:99: result in a subordinate deployment. Subordinate services may still
As we discussed, a subordinate charm must necessarily have a:
subordinate: true
Principal charms may also have "scope: container" relations, which should only be established with a subordinate charm. These relations shouldn't turn the principal into a subordinate.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-services.rst#newcode128
docs/source/drafts/subordinate-services.rst:128: logging:
s/logging/rsyslog/, to make the example more clear. Then please review all the relation information below. There are mismatches and typos.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-services.rst#newcode163
docs/source/drafts/subordinate-services.rst:163: juju add-relation logging wordpress
s/rsyslog/logging/ above and below as well.
Message from bcsaller@gmail.com
2012-02-10T01:32:25+00:00bcsallerurn:md5:9c25dbedba43a47aad9733f993131321
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/implicit-relations.rst
File docs/source/drafts/implicit-relations.rst (right):
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/implicit-relations.rst#newcode59
docs/source/drafts/implicit-relations.rst:59: provides less information) `juju-info` interface.
There was some confusion around my wording but not the intention here. Some wording was changed to be more explicit.
On 2012/02/10 00:20:39, niemeyer wrote:
> This is the opposite of what we agreed, I think. Doing that would mean no charm
> containing a juju-info relation will have auto-resolution working properly. It
> must resolve to logging-directory, unless juju-info is explicitly requested in
> the command line.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst#newcode61
docs/source/drafts/subordinate-internals.rst:61: service-00000002: {name: name, role: role}
On 2012/02/10 00:20:39, niemeyer wrote:
> Let's take the chance to update this, as we discussed online:
>
> relations
> relation-00000001:
> interface: <name>
> scope: <name>
> services:
> service-00000001: {name: name, role: role}
> service-00000002: {name: name, role: role}
>
> Please mention that this change to the topology had bumped the VERSION to 2.
Done.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst#newcode80
docs/source/drafts/subordinate-internals.rst:80: have been satisfied.
On 2012/02/10 00:20:39, niemeyer wrote:
> As we discussed online, adding a "scope: container" relation isn't necessarily
> going to deploy a charm, since it may be established between two subordinate
> charms.
>
> What causes the deployment of the subordinate charm is adding a "scope:
> container" relation with a "subordinate: false" charm.
Done.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-services.rst#newcode128
docs/source/drafts/subordinate-services.rst:128: logging:
I always have trouble writing these by hand. Hopefully I got it right this time.
On 2012/02/10 00:20:39, niemeyer wrote:
> s/logging/rsyslog/, to make the example more clear. Then please review all the
> relation information below. There are mismatches and typos.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-services.rst#newcode163
docs/source/drafts/subordinate-services.rst:163: juju add-relation logging wordpress
On 2012/02/10 00:20:39, niemeyer wrote:
> s/rsyslog/logging/ above and below as well.
Done.
Message from unknown
2012-02-10T01:33:18+00:00bcsallerurn:md5:9ae91dc3c9e68067d5e313858211a1cf
Message from bcsaller@gmail.com
2012-02-10T01:33:19+00:00bcsallerurn:md5:a3b6615fdfef6e8872726b80480c37b4
Please take a look.
Message from n13m3y3r@gmail.com
2012-02-10T18:28:30+00:00niemeyerurn:md5:b0c2615414093b9dc29351f35215c8cf
Thanks a lot for the extensive spec, and for keeping up with it Ben. Just a few last minute trivials.
LGTM!
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordinate-internals.rst#newcode44
docs/source/drafts/subordinate-internals.rst:44: encoded structure under the preceding ZK node. `container_relations`
On 2012/02/10 00:20:39, niemeyer wrote:
> s/under/within/
Missing this fix. The YAML content goes within the preceding ZK node. "under" was already used above to give the idea of a sub-node.
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordinate-internals.rst#newcode68
docs/source/drafts/subordinate-internals.rst:68: handled automatically.
Please include in the same paragraph:
The version of the topology is also changed from 1 to 2.
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordinate-services.rst#newcode108
docs/source/drafts/subordinate-services.rst:108: `subordinate: true` charms.
Nice wording on the new text. Thanks.
Message from bcsaller@gmail.com
2012-02-10T18:32:49+00:00bcsallerurn:md5:36933214c7f6b02faa8707ebe93053a8
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordinate-internals.rst#newcode68
docs/source/drafts/subordinate-internals.rst:68: handled automatically.
On 2012/02/10 18:28:30, niemeyer wrote:
> Please include in the same paragraph:
>
> The version of the topology is also changed from 1 to 2.
I hadn't because of the issue of time sensitivity in the document, but I will now.
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordinate-services.rst
File docs/source/drafts/subordinate-services.rst (right):
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordinate-services.rst#newcode108
docs/source/drafts/subordinate-services.rst:108: `subordinate: true` charms.
On 2012/02/10 18:28:30, niemeyer wrote:
> Nice wording on the new text. Thanks.
Done.
Message from unknown
2012-02-27T19:31:40+00:00bcsallerurn:md5:4f6aab77a9d770824671a8717bec7f2a
Message from bcsaller@gmail.com
2012-02-27T19:31:42+00:00bcsallerurn:md5:d6527a0f4a6064f7ede1e48b82787ec2
Please take a look.
Message from n13m3y3r@gmail.com
2012-02-27T20:24:19+00:00niemeyerurn:md5:162180f3974ab311aa05aeb52dbce22f
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst#newcode30
docs/source/drafts/subordinate-internals.rst:30: container/
As discussed online:
1) Drop container/
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst#newcode41
docs/source/drafts/subordinate-internals.rst:41: settings/
2) Move this to within each of the respective units above.
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst#newcode50
docs/source/drafts/subordinate-internals.rst:50: global/
3) Drop both global/ and global/global/
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst#newcode51
docs/source/drafts/subordinate-internals.rst:51: client/
4) Move this back to under relation-000000002.
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst#newcode81
docs/source/drafts/subordinate-internals.rst:81: codebase for dealing with path construction and node access.
The text above needs fixing to accommodate the discussed changes.
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst#newcode92
docs/source/drafts/subordinate-internals.rst:92: interface: name
s/name/<name>/, same below.
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst#newcode95
docs/source/drafts/subordinate-internals.rst:95: service-00000001: {name: name, role: role}
s/name: name/name: <name>/, same for role.
One is a variable, the other is not. Same below.
Message from unknown
2012-02-27T23:13:57+00:00bcsallerurn:md5:882c788f36f348f4f79e8e810bf16478
Message from bcsaller@gmail.com
2012-02-27T23:13:59+00:00bcsallerurn:md5:7e02b73b4f7070c7e04f715b43552467
Please take a look.
Message from kapilt@gmail.com
2012-02-28T00:04:54+00:00hazmaturn:md5:d97ef69630b3d4744018057bc4f15a5f
lgtm
Message from n13m3y3r@gmail.com
2012-02-28T00:45:20+00:00niemeyerurn:md5:49eafa4e0a6f4270112d8be272af88e0
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordinate-internals.rst#newcode92
docs/source/drafts/subordinate-internals.rst:92: interface: name
On 2012/02/27 20:24:19, niemeyer wrote:
> s/name/<name>/, same below.
Still needs addressing.
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode30
docs/source/drafts/subordinate-internals.rst:30: unit_id-00000001/
All the unit ids need fixing in this example. It's something along the lines of "unit-0000000001".
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode32
docs/source/drafts/subordinate-internals.rst:32: - unit_id-000000002
The style is also inconsistent. This is a node, just like "unit-id-00000001" below, so both should end in /, or both should start with -.
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode64
docs/source/drafts/subordinate-internals.rst:64: relation-id/
relation-<id>
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode65
docs/source/drafts/subordinate-internals.rst:65: container_id/ # omitted for global scoped relations
unit-<principal id>
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode66
docs/source/drafts/subordinate-internals.rst:66: <role>/ {name: name, role: role}
{name: <name>, role: <role>}
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode73
docs/source/drafts/subordinate-internals.rst:73: store an container information and the normal child nodes are present
This needs some fixing ("relations do not store an container information"?). Something like:
Globally scoped relations have the "client", "server" and "settings" nodes directly under the "relation-<id>" node.
Message from unknown
2012-02-28T01:13:56+00:00bcsallerurn:md5:fd697eb987b094938b2f7937db930f11
Message from bcsaller@gmail.com
2012-02-28T01:13:59+00:00bcsallerurn:md5:8c88429aa9b4a1fc7409e69ab1deffa3
Please take a look.
Message from bcsaller@gmail.com
2012-02-28T01:14:08+00:00bcsallerurn:md5:1ae19b8f9915daa2351348e73d93c5c2
Thanks for looking this over again, believe I got it all.
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst
File docs/source/drafts/subordinate-internals.rst (right):
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode30
docs/source/drafts/subordinate-internals.rst:30: unit_id-00000001/
On 2012/02/28 00:45:20, niemeyer wrote:
> All the unit ids need fixing in this example. It's something along the lines of
> "unit-0000000001".
Done.
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode32
docs/source/drafts/subordinate-internals.rst:32: - unit_id-000000002
On 2012/02/28 00:45:20, niemeyer wrote:
> The style is also inconsistent. This is a node, just like "unit-id-00000001"
> below, so both should end in /, or both should start with -.
I think I understand what you mean here. Hopefully the changes address what you expect.
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode64
docs/source/drafts/subordinate-internals.rst:64: relation-id/
On 2012/02/28 00:45:20, niemeyer wrote:
> relation-<id>
Done.
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode65
docs/source/drafts/subordinate-internals.rst:65: container_id/ # omitted for global scoped relations
On 2012/02/28 00:45:20, niemeyer wrote:
> unit-<principal id>
Done.
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode66
docs/source/drafts/subordinate-internals.rst:66: <role>/ {name: name, role: role}
On 2012/02/28 00:45:20, niemeyer wrote:
> {name: <name>, role: <role>}
Done.
https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordinate-internals.rst#newcode73
docs/source/drafts/subordinate-internals.rst:73: store an container information and the normal child nodes are present
On 2012/02/28 00:45:20, niemeyer wrote:
> This needs some fixing ("relations do not store an container information"?).
> Something like:
>
> Globally scoped relations have the "client", "server" and "settings" nodes
> directly under the "relation-<id>" node.
Done.
Message from n13m3y3r@gmail.com
2012-02-28T01:24:19+00:00niemeyerurn:md5:59b6b3f3fb78b42eb31772136e0709a8
LGTM, thanks.