-
Notifications
You must be signed in to change notification settings - Fork 227
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
Do not refresh if no repo changes #4085
Do not refresh if no repo changes #4085
Conversation
Signed-off-by: John Belamaric <[email protected]>
ffaa6d3
to
c9cd488
Compare
Signed-off-by: John Belamaric <[email protected]>
60d4bd2
to
eeefc71
Compare
Signed-off-by: John Belamaric <[email protected]>
@@ -186,6 +189,35 @@ func (r *gitRepository) Close() error { | |||
return nil | |||
} | |||
|
|||
func (r *gitRepository) Version(ctx context.Context) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we think about the version for the git repository calculated here? If I understand the code correctly it essentially hashes all the refs and uses that as the version. Would this handle commits that happens to existing refs (which I think primarily branches and tags) in git? In particular, would this version change if someone adds an additional commit on the main branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This checks the existence of refs, plus the commit hash to which they point. A push to main updates the main head ref to point to that new commit.
So this will change for any new or deleted tag, for a commit to any branch, for a new or deleted branch, or for a forced push, or for a tag move. AFAIK, there is no change you could make to a remote that this would not pick up. Maybe some sort of magic where you add a commit but somehow have no ref to it. But we would not care about such a change anyway (if it is even possible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, by using both the refs and the commit hash it should cover all scenarios I can think of.
* Do not refresh if no repo changes Signed-off-by: John Belamaric <[email protected]> * Need to hold the lock during Version Signed-off-by: John Belamaric <[email protected]> * Fix fake repo Signed-off-by: John Belamaric <[email protected]> --------- Signed-off-by: John Belamaric <[email protected]>
This is a partial fix to the issue of Porch repository polling taking a long time when polling a repo, and holding the lock. It avoids any refresh at all during a steady state.
This helps but is insufficient to resolve the problem completely. Future PR will do partial refreshes, where we hold the lock only long enough to load new or changed revisions.