Edgewall Software
Modify

Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#10602 closed defect (fixed)

GitPlugin: Fix cache

Reported by: Peter Suter Owned by: Jun Omae
Priority: normal Milestone: 1.0.2
Component: plugin/git Version: 1.0dev
Severity: normal Keywords: performance cache
Cc: Jun Omae Branch:
Release Notes:

Avoid missing changeset after resetting and removing branch on cached git repository.

API Changes:

Added remove_cache() and save_metadata() to CachedRepository for its subclasses.

Description

Created as part of the move of GitPlugin. Tickets originally reported for th:GitPlugin: th:#8016, th:#4505

Quoting xarkam:

If files in git repositories as renamed, the git plugin does not read the change.

Change on trac.ini in [git] section cached_repository = true to false resolve this bug.

Quoting anonymous:

Once a commit is reset, GitPlugin keeps complaining about the missing changeset even when newer changesets modify the same files. Browsing the git repo is not possible anymore after this. Disabling the cache in trac.ini worked around the errors, but browsing changes is now very slow.

Attachments (1)

post-receive (2.3 KB ) - added by Jun Omae 5 years ago.
git post-receive hook

Download all attachments as: .zip

Change History (24)

comment:1 by Peter Suter, 8 years ago

Type: enhancementdefect

comment:2 by Christian Boos, 7 years ago

Keywords: performance cache added
Milestone: next-major-releases
Version: 1.0dev

We may still need a cache even after #10606, but what exactly remains to be determined (quite likely some kind of tree → commit mapping).

in reply to:  description ; comment:3 by Jun Omae, 6 years ago

Milestone: next-major-releases1.0.2

Replying to psuter:

Quoting anonymous:

Once a commit is reset, GitPlugin keeps complaining about the missing changeset even when newer changesets modify the same files. Browsing the git repo is not possible anymore after this. Disabling the cache in trac.ini worked around the errors, but browsing changes is now very slow.

Proposed change are in log:jomae.git@t10602.

  • Added GitCachedRepository.sync instead of CachedRepository.sync
  • The GitCachedRepository.sync creates revision and node_change records from older revision to newer for each branch.
    • CachedRepository.sync creates from older to newer in date order.
  • If HEAD is reset, the sync updates metadata[CACHE_YOUNGEST_REV] to avoid a NoSuchChangeset

Thoughts?

comment:4 by Jun Omae, 6 years ago

Cc: Jun Omae added

in reply to:  3 comment:5 by Jun Omae, 6 years ago

Proposed change are in log:jomae.git@t10602.

Rebased branch on 1.0-stable r12533: log:jomae.git@t10602.1.

comment:6 by Jun Omae, 6 years ago

Owner: set to Jun Omae
Status: newassigned

comment:7 by anonymous, 5 years ago

Any news on this? It's an important issue and having git cache disabled slows down everything.

comment:8 by Ryan J Ollos, 5 years ago

Yes, there is a proposed change in comment:3. You could test it out and report back your findings.

comment:9 by anonymous, 5 years ago

Unfortunately, the patch conflicts. Also are there any plans to finally cut a new release?

comment:10 by anonymous, 5 years ago

Hm, there seems like there is fixes for git cache in the upstream 1.0-stable already, how does that integrate with this patch?

Like this for example: [12479/branches/1.0-stable/tracopt/versioncontrol/git/git_fs.py]

Could you rebase it so it would be easier to test it out?

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:11 by Jun Omae, 5 years ago

Sorry for the delay. I rebased the changes, jomae.git@t10602.2. Could you please try the new branch?

comment:12 by anonymous, 5 years ago

Thanks alot, will try it out.

comment:13 by anonymous, 5 years ago

Hmm.. having some troubles, but im doing a resync currently.. noticed in the log that for each commit it does:

                SELECT DISTINCT s.sid, n.value, e.value
                FROM session AS s
                 LEFT JOIN session_attribute AS n ON (n.sid=s.sid
                  and n.authenticated=1 AND n.name = 'name')
                 LEFT JOIN session_attribute AS e ON (e.sid=s.sid
                  AND e.authenticated=1 AND e.name = 'email')
                WHERE s.authenticated=1 ORDER BY s.sid

We have the trac_user_rlookup = true, which is probably why it does it, but does it really need to do it for EVERY commit?

And if it does need to do that… why don't search for the email(?) directly instead of fetching all users?

Also, what is the recommended settings now for cached_repository and persistent_cache ?

in reply to:  13 ; comment:14 by Jun Omae, 5 years ago

Replying to anonymous:

Hmm.. having some troubles, but im doing a resync currently..

Please post your testing procedure and details of the troubles.

noticed in the log that for each commit it does: […] We have the trac_user_rlookup = true, which is probably why it does it, but does it really need to do it for EVERY commit?

And if it does need to do that… why don't search for the email(?) directly instead of fetching all users?

Currently, users list is not cached.

in reply to:  14 ; comment:15 by Ryan J Ollos, 5 years ago

Replying to jomae:

Currently, users list is not cached.

Is the query from Environment.get_known_users: trunk/trac/env.py@12843:687-695#L679?

If I understand correctly, caching of the user list is proposed to be added in #7339 (though the patch needs more work).

in reply to:  15 comment:16 by anonymous, 5 years ago

Replying to rjollos:

Replying to jomae:

Currently, users list is not cached.

Is the query from Environment.get_known_users: trunk/trac/env.py@12843:687-695#L679?

If I understand correctly, caching of the user list is proposed to be added in #7339 (though the patch needs more work).

Yes thats the function it uses. Adding a get_user_by_email would be one solution, if there is no global cache.

in reply to:  14 comment:17 by anonymous, 5 years ago

Replying to jomae:

Replying to anonymous:

Hmm.. having some troubles, but im doing a resync currently..

Please post your testing procedure and details of the troubles.

Having random problems with some commits not wanting to trigger the ticket update thing.. sometimes they work, sometimes not, don't think its related to this branch tho.

comment:18 by Jun Omae, 5 years ago

Ah, at least, repository sync should be run via post-receive hook when branches are removed or renamed. Otherwise, Trac wouldn't know git repository is changed.

trac-admin $ENV repository sync $reponame

comment:19 by anonymous, 5 years ago

I used changeset added "(default)" *SHA1* and sometimes it works and sometimes not, nothing in the log when it dosent work, it never calls the hook.

What is more strange tho is doing a resync:

With 1.0.1: 9254 revisions cached

With jomae.git@t10602.2: 8932 revisions cached

Did a comparison and it seems like its mostly Merge-commits missing

by Jun Omae, 5 years ago

Attachment: post-receive added

git post-receive hook

in reply to:  19 comment:20 by Jun Omae, 5 years ago

Replying to anonymous:

I used changeset added "(default)" *SHA1* and sometimes it works and sometimes not, nothing in the log when it dosent work, it never calls the hook.

Hmm, it works well except merge-commits.

Did a comparison and it seems like its mostly Merge-commits missing

Oh, sorry. I'm wrong. Revised log:jomae.git@t10602.2. I tested with uwsgi (4 processes) and post-receive hook for adding, updating, force updating and removing branch.

Last edited 5 years ago by Jun Omae (previous) (diff)

comment:21 by anonymous, 5 years ago

Hi

Just tried your commit and now it finds the same number of commits as 1.0.1!

We're using the https://github.com/aaugustin/trac-github to take care of the hooks, and it works fine 99% of the times (think the cache is the blame when its not working)

Looking forward to see this branch merged into 1.0-stable

comment:22 by Jun Omae, 5 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Thanks for the feedback. Committed in [12972] and merged to trunk in [12973].

comment:23 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.