Dorian Raymer

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.

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).

Dorian Raymer

commented on CR-6

28 Jun

Probably shouldn't use dict as a content container. Even when the lcaarch + magnet integration message layer is fixed to support sending arbitrary binary data, sending application content as a nested Python dictionary with binary data as one of the values will not ('just') work. This might be an indication that there needs to be a common understanding among developers and designers on what data-structures/data-types are acceptable as application content.

Dorian Raymer
Started project
added README