Merge lp:~free.ekanayaka/storm/schema-commit-once into lp:storm

[Free Ekanayaka]

This branch cleans up a bit ZStormResourceManager.make() and makes it a tad more efficient by commit all schema changes only once across all stores.

[Thomas Herve (therve)]

Looks great, +1!

review: Approve

[lp:~free.ekanayaka/storm/schema-commit-once]

423. By Free Ekanayaka on 2012-01-11

Merge from trunk, fix broken import

[Björn Tillenius (bjornt)]

[1]

+ class NoopCommitter(object):
+ commit = lambda _: None
+ rollback = lambda _: None
+
+ committer = self._committer if self._autocommit else NoopCommitter()
+ patch_applier = PatchApplier(store, self._patch_package, committer)

What happens if PatchApplier tries to roll back the transaction?

[2]

+ store1 = zstorm.get("test2")
+ store2 = zstorm.get("test2")
+ self.assertEqual([], list(store1.execute("SELECT foo FROM test")))
+ self.assertEqual([], list(store2.execute("SELECT foo FROM test")))

I assume the first line should read "test1"?

review: Needs Information

[Free Ekanayaka (free.ekanayaka)]

Thanks for the careful review, Bjorn.

[1]

In that case the exception will be simply propagated without actually rolling back anything, see patch.py line 123. Issuing autocommit(False) means that it's up to the calling code to drive commits and rollbacks. In the specific case of the ZStormResourceManager, the exception will not be caught and will make the process exit (and all open PG connections/transactions will be rolled back automatically).

I've changed the autocommit() docstring to:

        """Control whether to automatically commit/rollback schema changes.

        The default is C{True}, if set to C{False} it's up to the calling code
        to handle commits and rollbacks.

        @note: In case of rollback the exception will just be propagated, and
            no rollback on the store will be performed.
        """

to make it more clear.

[2]

Yeah, good catch, fixed.

[lp:~free.ekanayaka/storm/schema-commit-once]

424. By Free Ekanayaka on 2012-01-13

Better docstring and test fix

[Björn Tillenius (bjornt)]

On Fri, Jan 13, 2012 at 11:52:25AM -0000, Free Ekanayaka wrote:
> Thanks for the careful review, Bjorn.
>
> [1]
>
> In that case the exception will be simply propagated without actually
> rolling back anything, see patch.py line 123. Issuing autocommit(False)
> means that it's up to the calling code to drive commits and rollbacks.
> In the specific case of the ZStormResourceManager, the exception will
> not be caught and will make the process exit (and all open PG
> connections/transactions will be rolled back automatically).
>
> I've changed the autocommit() docstring to:
>
> """Control whether to automatically commit/rollback schema changes.
>
> The default is C{True}, if set to C{False} it's up to the calling code
> to handle commits and rollbacks.
>
> @note: In case of rollback the exception will just be propagated, and
> no rollback on the store will be performed.
> """
>
> to make it more clear.

Ok. I suspect that behavior is a bit confusing, people will forget to
handle the rollbacks. But I guess it's good enough, especially since
autocommit is True by default.

> [2]
>
> Yeah, good catch, fixed.

+1

--
Björn Tillenius | https://launchpad.net/~bjornt

[Björn Tillenius (bjornt)]

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Thanks!

[1]

Yeah, but it's really a feature meant only for the testing machinery. As you point, the default behavior is want you want for actual deployments. If you change it the docstring explains that you should know what you're doing, so I guess it's fine.

[lp:~free.ekanayaka/storm/schema-commit-once]

425. By Free Ekanayaka on 2012-01-13

Fix typo