New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Submitted
Owner: ----
Closed: Feb 4
Cc:
Components:



Sign in to add a comment
link

Issue 10401: Excessive resource consumptions when deleting project

Reported by fabio.po...@gmail.com, Jan 29 Project Member

Issue description

*****************************************************************
*****                                                       *****
***** !!!! THIS BUG TRACKER IS FOR GERRIT CODE REVIEW !!!!  *****
*****                                                       *****
***** DO NOT SUBMIT BUGS FOR CHROME, ANDROID, CYANOGENMOD,  *****
***** INTERNAL ISSUES WITH YOUR COMPANY'S GERRIT SETUP, ETC.*****
*****                                                       *****
*****   THOSE ISSUES BELONG IN DIFFERENT ISSUE TRACKERS     *****
*****                                                       *****
*****************************************************************

Affected Version: 2.16, 2.15

What steps will reproduce the problem?
1. Delete a project

What is the expected output?

The project is deleted without excessive resource consumption

What do you see instead?

Memory usage skyrockets

Please provide any additional information below.

The delete-project plugin checks if any project is the parent of another one. The Parent-child relationship is stored in the refs/meta/config, but only child-to-parent and NOT parent-to-child :(

See the attached screenshot for an example of memory increase happened on gerrithub.io
 
delete-project.png
144 KB View Download

Comment 1 by david.os...@gmail.com, Jan 29

Project Member
So, this seems to be the line in delete-project plugin responsible for the spike: [1]

  List<ProjectInfo> children = listChildProjectsProvider.get().apply(rsrc);

and this is the logic in ListChildProjects to retrieve the projects,
who are the children of the project-to-delete:

  for (Project.NameKey name : projectCache.all()) {
      ProjectState c = projectCache.get(name);
      if (c != null
          && parent.equals(c.getProject().getParent(allProjects))
          && c.statePermitsRead()) {
        children.put(c.getNameKey(), c.getProject());
      }
  }

Instead of scanning over all projects again and again we could benefit from 
maintaining projectChildrenCache, that could return all children projects
for a specific project or empty list when input project doesn't have any
children.

[1] https://github.com/GerritCodeReview/plugins_delete-project/blob/master/src/main/java/com/googlesource/gerrit/plugins/deleteproject/projectconfig/ProjectConfigDeleteHandler.java#L51
[2] https://github.com/GerritCodeReview/gerrit/blob/master/java/com/google/gerrit/server/restapi/project/ListChildProjects.java#L81-L88

Comment 2 by luca.mil...@gmail.com, Jan 29

Project Member
Summary: Excessive resource consumptions (was: Excessive resource consumption)
This is not a new issue, the code has been like that for years. However, the v2.16 uses a Lucene index for projects and thus the cache is not kept warm all the times.

In the short term we need to relax the verification for parent-child and make it optional and in master we should introduce a “parent” field in the projects index so that we can avoid a full scan for checkinrg parent/child relationship.

Comment 3 by luca.mil...@gmail.com, Jan 29

Project Member
Labels: -Priority-1 Priority-2

Comment 4 by david.os...@gmail.com, Jan 29

Project Member
Status: Accepted (was: New)
> In the short term we need to relax the verification for parent-child and make it optional

This is problamtic, because you can end up with site corruption, when projects are deleted,
that have child projects.

> in master we should introduce a “parent” field in the projects index so that we can avoid a full scan for checkinrg parent/child relationship.

Great idea. I can add the field and the query to use it. Moreover, we already amended project index
in 2.16.4 fix release, so that we could also amend it again in 2.16.5 and add parent field and force
reindex of project index, by bumping the project index schema version.

Comment 5 by luca.mil...@gmail.com, Jan 29

Project Member
>> In the short term we need to relax the verification for parent-child and make it optional
>This is problamtic, because you can end up with site corruption, when projects are deleted, that have child projects.

Point taken, let's drop this option then :-)

> in master we should introduce a “parent” field in the projects index so that we can avoid a full scan for checkinrg parent/child relationship.

>> Great idea. I can add the field and the query to use it. Moreover, we already amended project index in 2.16.4 fix release, so that we could also amend it again in 2.16.5 and add parent field and force reindex of project index, by bumping the project index schema version.

I'm not a big fan of projects reindexing, it is super-slow and painful.
However, you can start working on this on the master branch and I can do the in-memory cache on stable-2.16 :-)

Then, we can always ask the community and, if they are happy with it, cherry-pick the master commit into the stable-2.16 later on.

Comment 6 by luca.mil...@gmail.com, Jan 29

Project Member
Components: -plugins>delete-project Backend
This isn't actually a problem of the delete-project plugin but of the "children" REST-API that causes the full scan of all projects to collect the children.

Comment 7 by luca.mil...@gmail.com, Jan 29

Project Member
Status: ChangeUnderReview (was: Accepted)
Fix for Gerrit v2.16:
https://gerrit-review.googlesource.com/c/gerrit/+/212085

Comment 8 by david.os...@gmail.com, Jan 29

Project Member
Alternative fix for v2.16 that makes use of project index to find parent project: [1].

[1] https://gerrit-review.googlesource.com/c/plugins/delete-project/+/212101

Comment 9 by luca.mil...@gmail.com, Jan 29

Project Member
@DavidO I like your fix a lot more :-) shall we use that one and re-implement the children API on top of it in Gerrit core?

Delete-project plugin just uses the API, which is the right way to integrate. The bug must be fixed in the Gerrit API IMHO.

Comment 10 by david.pu...@gmail.com, Feb 4

Components: plugins>delete-project
Labels: FixedIn-2.16.5
Status: Submitted (was: ChangeUnderReview)
Summary: Excessive resource consumptions when deleting project (was: Excessive resource consumptions )

Sign in to add a comment