Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce db queries #454

Merged
merged 18 commits into from
Sep 27, 2015
Merged

Reduce db queries #454

merged 18 commits into from
Sep 27, 2015

Conversation

disko
Copy link
Member

@disko disko commented Sep 14, 2015

This supersedes #448 snd #449 (same implementation, cleaned up commits)

@dnouri
Copy link
Contributor

dnouri commented Sep 14, 2015

Looks good!

@disko
Copy link
Member Author

disko commented Sep 14, 2015

Except for the still failing test on PostgreSQL "caused" by that change: disko@1efb134#diff-090c6d6f97cd0028f14840e7b7f6aa94R227 :(

disko and others added 10 commits September 26, 2015 13:24
to possible fuzzy translations; see wichert/lingua#59
Additionally now we print file being currently compiled so that we know which statistic is
for which language.
Added '--no-wrap' option to invocation of `msgmerge` to avoid wrapping original
msgids. Wrapping needlessly "mangles" original msgids.
Please note that both options were lost during migration to lingua 3.6.1 in commit
Kotti@285bad5
@mgrbyte
Copy link
Contributor

mgrbyte commented Sep 26, 2015

@disko I'd like to help get this one passing, can you ping me instructions on how you're running the test suite against postgres pls?
I've just checkout this branch and run:

mkvirtualenv reduce-db-queries
python setup.py dev
pip install psycopg2
createdb -T template0 -E UTF8 -O mattr kotti
KOTTI_TEST_DB_STRING="postgresql+psycopg2://mattr@/kotti" py.test

fwiw, I get one failure with the above:
/home/mattr/git/kotti/kotti/tests/browser.txt:548: DocTestFailure

@disko
Copy link
Member Author

disko commented Sep 26, 2015

@mgrbyte this is exactly what I'm running and also the exact same failure. The failure occurs with index=True on Node.parent_id and vanishes without.

This makes absolutely no sense to me. I've removed that single assert in 66b285f and the test suite passes again on all environments. Tbh: I'm tempted to leave it removed and merge the branch…

@mgrbyte
Copy link
Contributor

mgrbyte commented Sep 26, 2015

@disko I think I solved it (maybe)

I think the test might be selecting the wrong control?

Line 548, insert:

import pdb;pdb.set_trace()
check the values of:
browser.getLink('Make Public', index=0)
browser.getLink('Make Public', index=1)
browser.getLink('Make Public', index=2)

The current test uses the control with index=2
I changed it to index=1 and the test passed.

@disko
Copy link
Member Author

disko commented Sep 26, 2015

On 26 Sep 2015, at 22:15, Matt Russell wrote:

@disko I think I solved it (maybe)

I think the test might be selecting the wrong control?

Line 548, insert:

import pdb;pdb.set_trace()
check the values of:
browser.getLink('Make Public', index=0)
browser.getLink('Make Public', index=1)
browser.getLink('Make Public', index=2)

The current test uses the control with index=2
I changed it to index=1 and the test passed.

I tried that, too. Unfortunately the test fails on MySQL and SQLite
then… :(

@mgrbyte
Copy link
Contributor

mgrbyte commented Sep 26, 2015

@disko I think its something do with the order of test execution not being the same.
Under SQLite:

(Pdb) browser.getLink('Make Public', index=0)
<Link text='Make Public' url='http://localhost:6543/second-child-1/@@workflow-change?new_state=public'>
(Pdb) browser.getLink('Make Public', index=1)
<Link text='Make Public' url='http://localhost:6543/second-child-1/third-child/@@workflow-change?new_state=public'>
(Pdb) browser.getLink('Make Public', index=2)
<Link text='Make Public' url='http://localhost:6543/second-child-1/grandchild-3/@@workflow-change?new_state=public'>

Under PostgreSQL:

<Link text='Make Public' url='http://localhost:6543/second-child-1/@@workflow-change?new_state=public'>
(Pdb) browser.getLink('Make Public', index=1)
<Link text='Make Public' url='http://localhost:6543/second-child-1/grandchild-2/@@workflow-change?new_state=public'>
(Pdb) browser.getLink('Make Public', index=2)
<Link text='Make Public' url='http://localhost:6543/second-child-1/grandchild-3/@@workflow-change?new_state=public'>

It would seem that either:

  • the order of links of the page are different due to some db ordering
  • the order of the tests executed by the test runner are different

@mgrbyte
Copy link
Contributor

mgrbyte commented Sep 26, 2015

@disko
I ran the test under both sqlite and pg backends again, writing out browser.contents to a file before the problematic call to browser.getLink('Make Public', index=2).

The order of the rows is different, so I conclude that the ordering @@contents view changes depending on db backend.

To test this theory out, I dug around the code a bit.
Change Line 548 in browser.txt to use index=3
and

diff --git a/kotti/resources.py b/kotti/resources.py
index 22f32fe..f9762e7 100644
--- a/kotti/resources.py
+++ b/kotti/resources.py
@@ -247,7 +247,7 @@ class Node(Base, ContainerMixin, PersistentACLMixin):
         remote_side=[id],
         backref=backref(
             '_children',
-            collection_class=ordering_list('position'),
+            collection_class=ordering_list('position', reorder_on_append=True),
             order_by=[position],
             cascade='all',
         )

That makes the test pass on both backends (and @@contents is order the same for both back-ends)

See the docstring for sqlalchemy.ext.orderinglist.

@mgrbyte
Copy link
Contributor

mgrbyte commented Sep 26, 2015

Here's 2 HTML files written using browser.contents before line 548 in browser.txt,
with reorder_on_append passed to _children's collection class (passing):

SQLite

browser-pass-sqlite

PostgeSQL

browser-pass-pg

and without (failures):

SQLite

browser-fail-sqlite

PostgeSQL

browser-fail-pg

@mgrbyte
Copy link
Contributor

mgrbyte commented Sep 26, 2015

fwiw, the whole test suite passes under both sqlite and pg with the reorder_on_append added.
👍 for merging it either way!

@disko
Copy link
Member Author

disko commented Sep 26, 2015

@mgrbyte you da man! thanks so much!

@codecov-io
Copy link

Current coverage is 94.62%

Merging #454 into master will increase coverage by +0.02% as of 185bcc3

@@            master    #454   diff @@
======================================
  Files           34      34       
  Stmts         3446    3458    +12
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3260    3272    +12
  Partial          0       0       
  Missed         186     186       

Review entire Coverage Diff as of 185bcc3


Uncovered Suggestions

  1. +0.55% via kotti/migrate.py#231...249
  2. +0.43% via kotti/init.py#1...15
  3. +0.23% via kotti/init.py#20...27
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@disko disko merged commit a3e799a into Kotti:master Sep 27, 2015
@disko disko deleted the feature/reduce_db_queries branch September 27, 2015 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants