Bug #11803

repositories#get_all_permissions API should not be so slow

Added by Tom Clegg over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
06/13/2017
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

Subtasks

Task #11848: fix slow inner loop in RepositoriesController#get_all_permissionsResolvedTom Clegg

Task #11835: Review 11803-repo-permsResolvedPeter Amstutz

Associated revisions

Revision 3c44c82a
Added by Tom Clegg over 4 years ago

Merge branch '11803-repo-perms'

refs #11803

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Tom Morris over 4 years ago

  • Target version set to 2017-06-21 sprint
  • Story points set to 1.0

#2 Updated by Tom Clegg over 4 years ago

  • Assigned To set to Tom Clegg

#3 Updated by Tom Clegg over 4 years ago

  • Status changed from New to In Progress

#5 Updated by Tom Clegg over 4 years ago

It looks like user.group_permissions() is the thing that is either cached or slow. Instead of calling/retrieving this for each user individually for each link giving permission to a repo, we can get one big hash up front with permissions for all users, so the inner loop needs to do 2 hash lookups.

There's certainly more room for improvement in get_all_permissions, but this easy change should bring the slowest queries down from ~60s to ~1s.

11803-repo-perms @ ec7510c680ee2065d6372fef6a340ef754dbe724

#6 Updated by Peter Amstutz over 4 years ago

Tom Clegg wrote:

It looks like user.group_permissions() is the thing that is either cached or slow. Instead of calling/retrieving this for each user individually for each link giving permission to a repo, we can get one big hash up front with permissions for all users, so the inner loop needs to do 2 hash lookups.

There's certainly more room for improvement in get_all_permissions, but this easy change should bring the slowest queries down from ~60s to ~1s.

11803-repo-perms @ ec7510c680ee2065d6372fef6a340ef754dbe724

LGTM.

But I got these test failures:

  1) Failure:
ContainerTest#test_find_reusable_with_logging_disabled [/usr/src/arvados/services/api/lib/log_reuse_info.rb:13]:
unexpected invocation: #<ActiveSupport::Logger:0x564654464998>.info('find_reusable: starting with 12 container records in database')
unsatisfied expectations:
- expected never, invoked once: #<ActiveSupport::Logger:0x564654464998>.info(any_parameters)

  2) Failure:
JobTest#test_find_reusable_without_logging [/usr/src/arvados/services/api/lib/log_reuse_info.rb:13]:
unexpected invocation: #<ActiveSupport::Logger:0x564654464998>.info('find_reusable: starting with 35 jobs readable by current user zzzzz-tpzed-xurymjxw79nv3jz')
unsatisfied expectations:
- expected never, invoked once: #<ActiveSupport::Logger:0x564654464998>.info(any_parameters)

Running in jenkins now: https://ci.curoverse.com/job/developer-run-tests-services-api/328/

#7 Updated by Peter Amstutz over 4 years ago

Turns out I have a test configuration override of "log_reuse_decisions: true" so false alarm. Please merge.

#8 Updated by Tom Clegg over 4 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF