Storm

Merge lp://qastaging/~stub/storm/bug-391996 into lp://qastaging/storm

  1. bug-391996
  2. Merge into trunk
Proposed by Stuart Bishop
Status: Rejected
Rejected by: Stuart Bishop
Proposed branch: lp://qastaging/~stub/storm/bug-391996
Merge into: lp://qastaging/storm
Diff against target: 141 lines (has conflicts) 
Text conflict in storm/zope/zstorm.py
To merge this branch: bzr merge lp://qastaging/~stub/storm/bug-391996
Reviewer Review Type Date Requested Status
Jamu Kakar (community) Abstain
James Henstridge Needs Fixing
Review via email: mp+7924@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

Addresses Bug #392011 by allowing a Store factory to be passed to ZStorm.get and ZStorm.create.

For Launchpad, we currently have to patch the Store returned by ZStorm.get to switch Cache implementation. The factory allows us to instantiate the Store correctly in the first place.

Revision history for this message
James Henstridge (jamesh) wrote :

I don't think this is the right approach to solving this problem. The usual pattern for using named stores is to configure them once on startup (either by set_defaul_uri() calls or with ZCML that does the same), and then just call IZStorm.get(name) after that (i.e. separate the connection configuration from connection use).

With the proposed patch, you'd need to pass a consistent store_factory to each get() call or risk having different behaviour in different threads.

Secondly: do we really want to be passing in a generic store_factory here? Would a cache factory be enough?

review: Needs Fixing
Revision history for this message
Stuart Bishop (stub) wrote :

> Addresses Bug #392011 by allowing a Store factory to be passed to ZStorm.get
> and ZStorm.create.

I of course mean Bug #391996, as indicated by the branch nickname.

Revision history for this message
Stuart Bishop (stub) wrote :

> I don't think this is the right approach to solving this problem. The usual
> pattern for using named stores is to configure them once on startup (either by
> set_defaul_uri() calls or with ZCML that does the same), and then just call
> IZStorm.get(name) after that (i.e. separate the connection configuration from
> connection use).
>
> With the proposed patch, you'd need to pass a consistent store_factory to each
> get() call or risk having different behaviour in different threads.

Just like you should be consistent passing through the correct uri too if you use that existing option. If that is actually a problem - Launchpad provides its own mechanism for looking up Stores, which is the single callsite for ZStorm.get (this abstraction layer is how we implement read-only mode and database load balancing).

A mechanism such as set_default_uri would also work (set_default_store_factory?), but just makes my mode more verbose with unnecessary function calls.

> Secondly: do we really want to be passing in a generic store_factory here?
> Would a cache factory be enough?

Not quite for Launchpad's needs. After getting a Store, I'm also attaching marker interfaces to it for the Zope adaption machinery. A cache factory would be enough if if I had some way of knowing if ZStore.get retrieved an existing Store or called ZStore.create, but I don't have a way of knowing that. Using a factory to generate the Store is future proof, as well as allowing more flexibility (such as using an entirely different Store implementation for example).

Revision history for this message
James Henstridge (jamesh) wrote :

> > I don't think this is the right approach to solving this problem. The usual
> > pattern for using named stores is to configure them once on startup (either
> by
> > set_defaul_uri() calls or with ZCML that does the same), and then just call
> > IZStorm.get(name) after that (i.e. separate the connection configuration
> from
> > connection use).
> >
> > With the proposed patch, you'd need to pass a consistent store_factory to
> each
> > get() call or risk having different behaviour in different threads.
>
> Just like you should be consistent passing through the correct uri too if you
> use that existing option. If that is actually a problem - Launchpad provides
> its own mechanism for looking up Stores, which is the single callsite for
> ZStorm.get (this abstraction layer is how we implement read-only mode and
> database load balancing).

I'd forgotten about that change in Launchpad, but I think the point still stands: would you consider it appropriate to have a store_factory argument on IStoreSelector.get()?

> A mechanism such as set_default_uri would also work
> (set_default_store_factory?), but just makes my mode more verbose with
> unnecessary function calls.

I would consider the API broken if it doesn't allow configuring a named store once ahead of time (rather than on each call to get the store). Whether it is possible to configure this through IZStorm.get() as well should be in addition to the "ahead of time" API rather than a replacement.

> > Secondly: do we really want to be passing in a generic store_factory here?
> > Would a cache factory be enough?
>
> Not quite for Launchpad's needs. After getting a Store, I'm also attaching
> marker interfaces to it for the Zope adaption machinery. A cache factory would
> be enough if if I had some way of knowing if ZStore.get retrieved an existing
> Store or called ZStore.create, but I don't have a way of knowing that. Using a
> factory to generate the Store is future proof, as well as allowing more
> flexibility (such as using an entirely different Store implementation for
> example).

That sounds like a reasonable use case. Perhaps it'd be worth bringing this bug/branch up on the Storm mailing list for added visibility?

lp://qastaging/~stub/storm/bug-391996 updated
309. By Stuart Bishop

Add ZStorm.set_default_store_factory to mirror ZStorm.set_default_uri, as requested by review

Revision history for this message
Stuart Bishop (stub) wrote :

On Wed, Jul 1, 2009 at 3:53 PM, James
Henstridge<email address hidden> wrote:

>> Just like you should be consistent passing through the correct uri too if you
>> use that existing option. If that is actually a problem - Launchpad provides
>> its own mechanism for looking up Stores, which is the single callsite for
>> ZStorm.get (this abstraction layer is how we implement read-only mode and
>> database load balancing).
>
> I'd forgotten about that change in Launchpad, but I think the point still stands: would you consider it appropriate to have a store_factory argument on IStoreSelector.get()?

Of course not - the call site doesn't know the factory to use, not
other ZStorm specific implementation details like the name or uri.
IStoreSelector exists to calculate this sort of thing based on the
current runtime state.

>> A mechanism such as set_default_uri would also work
>> (set_default_store_factory?), but just makes my mode more verbose with
>> unnecessary function calls.
>
> I would consider the API broken if it doesn't allow configuring a named store once ahead of time (rather than on each call to get the store).  Whether it is possible to configure this through IZStorm.get() as well should be in addition to the "ahead of time" API rather than a replacement.

The 'ahead of time' model doesn't fit Launchpad, where we calculate
this stuff at runtime.

I've added ZStorm.set_default_store_factory for people able to use the
'set once ahead of time'.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

lp://qastaging/~stub/storm/bug-391996 updated
310. By Stuart Bishop

Fix docstring ordering

Revision history for this message
James Henstridge (jamesh) wrote :

> > I'd forgotten about that change in Launchpad, but I think the point still
> stands: would you consider it appropriate to have a store_factory argument on
> IStoreSelector.get()?
>
> Of course not - the call site doesn't know the factory to use, not
> other ZStorm specific implementation details like the name or uri.
> IStoreSelector exists to calculate this sort of thing based on the
> current runtime state.

So my point is that in other applications, IZStorm.get() is used in the same way as Launchpad's IStoreSelector.get(). So that needs to be taken into account when changing the interface, rather than treating it as an internal plumbing API.

> >> A mechanism such as set_default_uri would also work
> >> (set_default_store_factory?), but just makes my mode more verbose with
> >> unnecessary function calls.
> >
> > I would consider the API broken if it doesn't allow configuring a named
> store once ahead of time (rather than on each call to get the store).  Whether
> it is possible to configure this through IZStorm.get() as well should be in
> addition to the "ahead of time" API rather than a replacement.
>
> The 'ahead of time' model doesn't fit Launchpad, where we calculate
> this stuff at runtime.

It is certainly true that the Launchpad code isn't passing the configuration of stores to the IZStorm utility until first use, but you do make the policy decisions about how stores will be configured ahead of time (through IStoreSelector.push(dbpolicy)).

The main issue is to be able to preserve a split between configuration of named stores and code that uses those named stores. That's all I mean by "ahead of time configuration".

> I've added ZStorm.set_default_store_factory for people able to use the
> 'set once ahead of time'.

Good. I'd still like to see this brought up on the mailing list, since I'm not convinced the change is getting enough exposure through the code review system.

Revision history for this message
Jamu Kakar (jkakar) wrote :

The store_factory keyword argument on ZStorm.get is a bit
suspicious. Partly because of the things James has mentioned, but
also because it isn't used if a store is already registered with the
specified name. If I pass in a store_factory I expect to get a
store that was created by it.

I think I might be a bit confused about the use case you have. Are
you trying to use the same store name to access master (read/write)
and slave (read-only) databases? If so, is this a good idea?
Presumably you know somewhere outside ZStorm.get that you want
either a read/write or a read-only store--would it make sense to use
different names for each of these things to avoid needing all of
this special casing?

review: Abstain

Unmerged revisions

310. By Stuart Bishop

Fix docstring ordering

309. By Stuart Bishop

Add ZStorm.set_default_store_factory to mirror ZStorm.set_default_uri, as requested by review

308. By Stuart Bishop

Optional Store factory to ZStorm.get and ZStorm.create to allow customization of the Store

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storm/zope/zstorm.py'
2--- storm/zope/zstorm.py 2009-06-26 08:59:26 +0000
3+++ storm/zope/zstorm.py 2009-08-13 20:20:37 +0000
4@@ -68,6 +68,7 @@
5 self._local = threading.local()
6 self._default_databases = {}
7 self._default_uris = {}
8+ self._default_store_factories = {}
9
10 def _reset(self):
11 for name, store in list(self.iterstores()):
12@@ -112,10 +113,22 @@
13 self._default_databases[name] = self._get_database(default_uri)
14 self._default_uris[name] = default_uri
15
16- def create(self, name, uri=None):
17+ def set_default_store_factory(self, name, default_store_factory):
18+ """Set C{default_store_factory} as the default factory to use
19+ when creating the C{name} Store.
20+
21+ @param default_store_factory: The factory used to create the
22+ Store. It must be a callable accepting the database as its
23+ single parameter.
24+ """
25+ self._default_store_factories[name] = default_store_factory
26+
27+ def create(self, name, uri=None, store_factory=None):
28 """Create a new store called C{name}.
29
30 @param uri: Optionally, the URI to use.
31+ @param store_factory: Optionally, the factory used to create the
32+ store. Factory is passed the database as its single parameter.
33 @raises ZStormError: Raised if C{uri} is None and no default
34 URI exists for C{name}. Also raised if a store with
35 C{name} already exists.
36@@ -130,7 +143,11 @@
37 if name is not None and self._named.get(name) is not None:
38 raise ZStormError("Store named '%s' already exists" % name)
39
40- store = Store(database)
41+ if store_factory is not None:
42+ store = store_factory(database)
43+ else:
44+ store = self._default_store_factories.get(name, Store)(database)
45+
46 store._register_for_txn = True
47 store._event.hook(
48 "register-transaction", register_store_with_transaction,
49@@ -143,17 +160,28 @@
50
51 return store
52
53+<<<<<<< TREE
54 def get(self, name, default_uri=None):
55 """Get the store called C{name}, creating it first if necessary.
56+=======
57+ def get(self, name, default_uri=None, store_factory=None):
58+ """Get the store called C{name} or None if one isn't available.
59+>>>>>>> MERGE-SOURCE
60
61 @param default_uri: Optionally, the URI to use to create a
62+<<<<<<< TREE
63 store called C{name} when one doesn't already exist.
64 @raises ZStormError: Raised if C{uri} is None and no default
65 URI exists for C{name}.
66+=======
67+ store called C{name} when one doesn't already exist.
68+ @param store_factory: Optionally, the factory used to create the
69+ store. Factory is passed the database as its single parameter.
70+>>>>>>> MERGE-SOURCE
71 """
72 store = self._named.get(name)
73 if not store:
74- return self.create(name, default_uri)
75+ return self.create(name, default_uri, store_factory)
76 return store
77
78 def remove(self, store):
79
80=== modified file 'tests/zope/zstorm.py'
81--- tests/zope/zstorm.py 2009-03-05 21:53:12 +0000
82+++ tests/zope/zstorm.py 2009-08-13 20:20:37 +0000
83@@ -1,5 +1,5 @@
84 #
85-# Copyright (c) 2006, 2007 Canonical
86+# Copyright (c) 2006-2009 Canonical
87 #
88 # Written by Gustavo Niemeyer <gustavo@niemeyer.net>
89 #
90@@ -31,6 +31,7 @@
91 if has_zope_component:
92 from zope.component import provideUtility, getUtility
93
94+from storm.cache import GenerationalCache
95 from storm.exceptions import OperationalError
96 from storm.locals import Store
97
98@@ -92,6 +93,16 @@
99
100 self.assertTrue(raised)
101
102+ def test_create_with_store_factory(self):
103+ def store_factory(database):
104+ "Store factory creating Stores with a non-default Cache"
105+ return Store(database, cache=GenerationalCache(123456))
106+
107+ default_store = self.zstorm.create("def", "sqlite:")
108+ factory_store = self.zstorm.create("fac", "sqlite:", store_factory)
109+ self.failIfEqual(
110+ default_store._cache._size, factory_store._cache._size)
111+
112 def test_get_unexistent(self):
113 self.assertRaises(ZStormError, self.zstorm.get, "name")
114
115@@ -138,6 +149,26 @@
116 self.zstorm.remove(store)
117 self.assertEquals(self.zstorm.get_name(store), None)
118
119+ def test_get_with_store_factory(self):
120+ def store_factory(database):
121+ "Store factory creating Stores with a non-default Cache"
122+ return Store(database, cache=GenerationalCache(123456))
123+
124+ default_store = self.zstorm.get("def", "sqlite:")
125+ factory_store = self.zstorm.get("fac", "sqlite:", store_factory)
126+ self.failIfEqual(
127+ default_store._cache._size, factory_store._cache._size)
128+
129+ def test_set_default_store_factory(self):
130+ def store_factory(database):
131+ store = Store(database)
132+ store._test_marker = True
133+ return store
134+
135+ self.zstorm.set_default_store_factory('foo', store_factory)
136+ store = self.zstorm.get('foo', 'sqlite:')
137+ self.failUnless(getattr(store, '_test_marker', False))
138+
139 def test_default_databases(self):
140 self.zstorm.set_default_uri("name1", "sqlite:1")
141 self.zstorm.set_default_uri("name2", "sqlite:2")

Subscribers

People subscribed via source and target branches

to status/vote changes: