[ckan-dev] test performance and sqlalchemy complete

Fri Jan 7 01:14:17 UTC 2011

I have looked at the error and now think the monkeypatch is not the right
choice as the errors are correct.

The errors are due to us deleting the same row in the database twice.  i.e
doing  "delete from package where package.id = 1"  twice.  We should not be
doing this.  In postgres, this does not cause an error because it does not
report accurately how many rows it deleted to sqlalchemy. So sqlalchemy has
no way of knowing if a delete actually did not work (i.e it already
happened).  Sqlite does report this accurately and it shows that the tests
and code are doing a lot of deleting the same row twice. The monkeypatch
just turns this checking off.

Anyway in ticket 868 <http://knowledgeforge.net/ckan/trac/ticket/868> I have
attached patches for the vdm and ckan so the tests pass without the
monkeypatch.  The issue were mainly due to autoflushes deleting from a
cascade and then later we did the delete again.  This patch has the benefit
of working on sqlalchemy 0.5, it also means we do less flushing/work so
things are (very slightly) faster.

Also....

I have attached another new patch to
868<http://knowledgeforge.net/ckan/trac/ticket/868>
that brings the postgres times down from 14 mins to 9 mins on my laptop that
works off Sebs branch.

David

On Thu, Jan 6, 2011 at 5:17 PM, James Gardner <james at 3aims.com> wrote:

> I'm personally fine with this particular monkey patch and in this
> specific case think it is less confusing than installing a forked
> dependency.
>
> James
>
>
> On Thu, 2011-01-06 at 16:56 +0000, Seb Bacon wrote:
> > On 6 January 2011 16:12, William Waites <ww at eris.okfn.org> wrote:
> > > * [2011-01-06 14:28:03 +0000] Seb Bacon <seb.bacon at gmail.com> écrit:
> > >
> > > ] I've commited a monkeypatch now.
> > >
> > > please roll that back and just put a patched sqlalchemy up and refer to
> it in the pip requirements.
> > >
> > > third party apps monkey patching libraries wreak havoc with testing
> local changes, etc, to those libraries.
> > >
> > > please do not do this.
> >
> > While I agree with the general point about monkeypatches:
> >
> > (a) this patch, which only involves changing a flag from True to
> > False, is only applied when running tests against a sqlite backend.  I
> > can't think of a scenario whereby this would wreak any havoc?
> >
> > (b) putting up a patched tarball of sqlalchemy isn't painless either.
> > It creates a bit more cognitive-maintenance overhead for when we do
> > sqlalchemy updates (e.g. how will whoever does an update in a year
> > know why we have our own version? should we maintain the patch next to
> > it somewhere for that? etc)
> >
> > The point in (b) isn't a big deal, but I maintain neither is the
> > potential issue with a monkeypatch, *in this specific case*.  Plus, I
> > haven't got time to think about it again until next week ;-)
> >
> > Have a look at the patch
> > (https://bitbucket.org/sebbacon/ckan/changeset/ad9f51642166), and if
> > you're still convinced it might screw things up I'll happily change it
> > next week.
> >
> > Thanks,
> >
> > Seb
> >
> > _______________________________________________
> > ckan-dev mailing list
> > ckan-dev at lists.okfn.org
> > http://lists.okfn.org/mailman/listinfo/ckan-dev
>
>
>
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20110107/b50c5aa1/attachment-0001.html>