Excerpts from Gustavo Niemeyer's message of Tue Apr 26 18:04:27 UTC 2011:
> Review: Needs Fixing
> Some comments:
> 
> 
> [1]
> 
> +class ServiceUnitRelationResolvedAlreadyEnabled(StateError):
> +    """The unit has already been marked resolved.
> 
> Documentation needs updating.

how so?

> 
> [2]
> 
> +    def get_resolved(self):
> +        """Get the value of the resolved flag if any.
> 
> The result of this method isn't a flag.
> 

fixed.

> [3]
> 
> +    def get_resolved(self):
> (...)
> +        except zookeeper.NoNodeException:
> +            # We get to the same end state.
> +            returnValue(None)
> 
> This is a bit misleading.  We're just reading, so there's no "end state" to be
> achieved.  The existence of the file is actually what determines if resolved
> was requested or not.
> 

fixed.

> [4]
> 
> +        unit_resolve_path = "/units/%s/resolved" % self.internal_id
> 
> Given that this was used 5 times, it would be handy to have something like:
> 
>     @property
>     def _unit_resolved_path
>         return "/units/%s/resolved" % self.internal_id
> 
> Then you can use self._unit_resolved_path everywhere.
> 

done.

> [5]
> 
> +        rel_resolved_path = "/units/%s/relation_resolved" % self._internal_id
> 
> Our file paths so far are all dashed.  It'd be good to keep the convention
> inside ZooKeeper too.
> 
> May also be factored out into a property as per the point above.

done
> 
> [6]
> 
> +    def set_relation_resolved(self, relation_map):
> +        """Mark a unit's relations as in need of being resolved.
> 
> Doesn't this actually mean something like:
> 
>     "Mark the problem in the unit's relation as being resolved."
> 
> IOW, it's not requesting for the problem to be resolved, but stating
> that it has been.
> 

It is actually making a request for the problem to be solved, not solving the 
problem directly, as there are behavioral aspects to transitions that need be 
done inside of the unit agent.


> [7]
> 
> +        :param relation_map: A map of internal relation ids, to retry booleans.
> 
> This feels like a non-ideal API for a couple of reasons:
> 
> a) It's exposing internal ids rather than using the relation names as we have
>    elsewhere.
> 
> b) set_relation_resolved({"db": True, "cache": False}) feels like marking db as
>    resolved, and cache as unresolved.
> 
> It's a bit hard to suggest something else without knowing exactly how you
> intend to use.  Let's talk about it.
> 

As per irc discussion, we've deferred a) to a latter time, there's a commit in the branch
which does switch the syntax to

set_relation_resolved(db, retry_hooks=True)

but its then inconsistent with its get_resolved api which retrieves a dictionary of 
relation ids : retry hook boolean



> [8]
> 
> +    d1_keys = set(d1.keys())
> +    d2_keys = set(d2.keys())
> +
> +    must_match = d1_keys.intersection(d2_keys)
> 
> As a hint, this could be spelled as:
> 
> must_match = set(d1).intersection(d2):

fixed.

> 
> [9]
> 
> +    running = workflow_state in ("up",)
> 
> FWIW, this pattern feels slightly weird when there's a single value.
> workflow_state == "up" would be more straightforward.
> 
> [10]
> 
> +        self.assertIdentical(results.pop(0).type_name, "created")
> 
> assertIdentical is being overused a bit.  There's no reason why these should be
> allocated in the same memory location.
> 

agreed, fixed.