Default Project

Steve Foley

finished reviewing CR-10

07 Jul
Steve Foley

commented on CR-10

07 Jul

Shorten up some of these long lines as per PEP-8, please

Steve Foley

commented on CR-10

07 Jul

What does an empty one of these look like? Maybe {} or None or ""? If not defining something empty (your _str_ covers up an empty DataObject), maybe an exception or return type to indicate if a decode or encode worked or not?

Steve Foley

commented on CR-10

07 Jul

How about testing a case where an actual class gets created? For example, I get an encoded string of something like ['lifecycle', 'LCState\x00new', 'name', 'str\x00testResource'] when I do a lifecycle adjustment.

Steve Foley

commented on CR-10

07 Jul

How about showing a proper failure so that we can see how this is handled? Maybe something bad comes through AMQP?

Steve Foley

commented on CR-10

07 Jul

Is it possible for bad data to ever make it into the attributes? If so, can you test the failure case?

Steve Foley

commented on CR-10

07 Jul

Im working on some code for the ResourceAgent that has some generic handling of a resource registry client. Its not perfect, but its a start. Ill get it checked in sometime soon, I hope. I keep getting sidetracked.

Dorian Raymer

replied to Paul Hubbard in CR-8

29 Jun

Nope, part of the implementation. Needs better documentation, of course.

Dorian Raymer

commented on CR-5

29 Jun

Although the following tests do test the implementation of IStore using Cassandra as a backend store, these are more like integration tests than unit tests; a failure in any test might reflect an pure coding/implementation error, an problem/incorrect usage of the cassandra api, or a network problem. It would be ideal to test the implementation separate from actually calling out to cassandra, although I'm not sure of how to do that, or if it is possible. The casssndra python library presumably has a set of unit tests to ensure the cassandra protocol is properly implemented without having to call a running cassandra server.

Dorian Raymer

commented on CR-5

29 Jun

It might be organizationally more appropriate to only test the store service in the ion.services.coi area.

Dorian Raymer

commented on CR-5

29 Jun

The intended behavior and implementation of query might deserve a little more thought. After using it in real practice (the registry list function) I made some modifications that seemed to make it more generally useful. Very implementation specific and re specific. See the commit history, if interested.

Dorian Raymer

commented on CR-5

29 Jun

It should be specified in the IStore interface that self.kvs is an attribute provided by the object. 'kvs' is an OK name; I suggest 'backend_store', or maybe 'backend' but not strongly. Whatever the name, it should be a general word that represents the 'backend key value store' concept, and it should be specified in the interface so that all implementations use the same name.

Dorian Raymer

commented on CR-5

29 Jun

It is convenient but questionable to build in "factory" functions (as classmethods) for a class such as this; it is likely the backend of any store will require an asynchronous start-up procedure before that needs to occur before the actual store can be either instantiated, or considered ready to provide its behavior.
This classmethod is mixing the concepts of object life-cycle/object creation with object behavior, in this case. Defining a separate "Store Factory" that eventually produces a running store instance, could prove to be more flexible/useful.

Dorian Raymer

commented on CR-5

29 Jun

This should be renamed 'test_get_nonexistent' or 'test_get_fail'. '404' is something that comes from communication protocol; this is a programming interface.

Dorian Raymer

commented on CR-5

29 Jun

This might be better named StoreTest, since this particular test is testing the store.Store implementation.

Dorian Raymer

commented on CR-5

29 Jun

Declaring _init_ as part of the interface is [usually] not appropriate; the interface is a place for implementers to find out what functions they can implement and what behavior the functions should have. _init_ is a Python-Language-defined instance method that is always an option for implementers to implement; and the implementation of _init_ can vary between any two implementations of the same interface. An objects _init_ isn't something that provides [specific] behavior and therefore should not be used in an interface to specify an objects behavior.

Dorian Raymer

commented on CR-5

29 Jun

After developing cas.py and objstore.py using zope.interface, I recommend updating the IStore interface to use and comply with the zope.interface style.
This will separate the idea of defining the actual interface from the provision of an Abstract Base Class. The meaning of the term Abstract Base Class probably needs clarification, as Python 2.6 actually includes a module that provides such a concept/thing, with its own specific (Pythonic) meaning.

Dorian Raymer

It is a necessity to return a Deferred for all implementations of IStore (even if it utilizes a synchronous backend), because the IStore functions declare a Deferred as their return value.
There were (at least) two choices in how to return deferreds here: one is wrapping the backend call in maybeDeferred; the other is calling the backend normally, and returning an already fired deferred object (defer.succed(result) or defer.fail(reason)). maybeDeferred turns out to be a better choice because it will automatically return a failure (errback) if something goes wrong during the backend call, relieving the implementer from thinking about different exceptions to catch and subsequently manually creating case conditions where defer.fail is returned.

Matt Rodriguez

commented on CR-11

29 Jun

Add doc strings to all of the classes. The doc strings should explain the functionality of a class and any interactions
it has with other classes. The doc string should be one or two paragraphs.

Matt Rodriguez

commented on CR-11

29 Jun

Good job.

Please add concise and descriptive docstrings to the classes. This provides the necessary summary view for people to understand this module.
Think of it as a marketing task.

I think a few of the methods should be refactored into smaller methods.

I'm not sure why you use classmethod for load and reference. I know you might want to use it for create as this follows
a factory pattern. There could be a good reason to use it for load and reference, but I don't know it.

Nice use of asserts. This is a good programming practice.

Dorian Raymer

commented on CR-5

29 Jun

In reply to the objective:
The return value of a function can be specified in the doc-string of the functions definition in the interface (the Python language does not support declaring return type).
The Twisted developers rely heavily on interface & implementation doc-strings (documentation string of a function) to specify return value types; they make it very clear (via the doc-string) when a function returns a Deferred.
When the return type is not a Deferred or a built-in Python type/structure, then it is good to be able to specify the qualified object class (i.e. module.Class) or, if provided, the interface of the return object (e.g ion.data.datastore.cas.ICAStoreObject).

Matt Rodriguez

commented on CR-11

29 Jun

This method body is too long. Refactor it into smaller methods. The routines that add properties and add associations can be made their own methods.
A Pythonic way of doing this is to use a leading underscore to indicate that they are supposed to be private methods.

Matt Rodriguez

commented on CR-11

29 Jun

Small nitpick: make this 4 spaces not 8

Paul Hubbard

replied to Brian Fox in CR-6

29 Jun

Will do.

Paul Hubbard

replied to Brian Fox in CR-6

29 Jun

Not easy to do, might be possible.

Paul Hubbard

commented on CR-5

29 Jun

Worth a docstring to note that this is the base class for the tests further down the file.

Paul Hubbard

commented on CR-5

29 Jun

With this commented out, cruft key/value pairs will accumulate in cassandra. Need a way to cleanup after a test.

Paul Hubbard

commented on CR-10

29 Jun

How does this differ from get_service_instance?

Paul Hubbard

commented on CR-10

29 Jun

Is this deprecated code then?

Paul Hubbard

commented on CR-10

29 Jun

Should this be self.failUnlessEqual? Not sure what assertFalse does with two arguments.