[JENKINS-34727] Disable cache and delay branch indexing #54
Conversation
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
|
🐝 for now |
| } | ||
| }); | ||
| }, 10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
@amuniz Could you add a comment with the Jira issue? It is easy to forget why we did this and difficult to understand for other.
There was a problem hiding this comment.
@amuniz I did not see your comment Delaying the indexing for some seconds to avoid GitHub cache
|
This hack LGTM 🐝 |
|
I think this introduced the issue: f0ae91d Removing and trying. |
|
@amuniz I bet for it |
|
Whether disabling client cache fixes the issue or not, the delay on indexing should be kept IMO, because that delay will group burst of commits into one single build (which seems ok to me). |
;) |
| if (config.getClientCacheSize() > 0) { | ||
| Cache cache = toCacheDir().apply(config); | ||
| client.setCache(cache); | ||
| } |
There was a problem hiding this comment.
🐜 Does this not just revert #30? Perhaps we can retain caching overall, but reset it when we receive a webhook?
There was a problem hiding this comment.
@jglick Do you think worth reset the cache each time Jenkins receives a webhook? This is something happens (or should happen) very often.
There was a problem hiding this comment.
I was searching for a way to invalidate the cache, but didn't find it :( Do you know how to do it?
There was a problem hiding this comment.
// connector...urlFactory.client().getCache().flush();
There was a problem hiding this comment.
@jglick We can find scenarios where they do not use webhooks but I hope you agree with me that one important feature of this plugin is just to build stuff when something happens in the source code. In this context, polling is not the most appropiate procedure.
I agree with remove the cache because the most important information from GitHub is needed in hot, without client cache.
There was a problem hiding this comment.
Yeah, I think my point 1) was just a bad test, sorry.
Given the second point and some indications that the cache is into play in another issues (returning branches even without network connectivity as @recena noted), I think it's ok to disable the cache for now.
As there are no potential compatibility issues we can activate it again later if needed and fine tune what requests need cache and which ones not.
There was a problem hiding this comment.
That's why github plugin configuration allows disabling it. But you are using your custom connector.
There was a problem hiding this comment.
note, if (config.getClientCacheSize() > 0) { so it would be used only with positive cachesize value.
There was a problem hiding this comment.
That's why github plugin configuration allows disabling it. But you are using your custom connector.
The configuration is shared, so I guess disabling cache at configuration would disable it here too (before this PR, now there is no cache).
| } | ||
| }); | ||
| },5, TimeUnit.SECONDS); |
|
As summary:
|
|
🐝 and 👍 |
amuniz
changed the title
JENKINS-34727
Just for review, do not merge until I get a answer from GitHub support to know what's the cache expiry time so I can set up the delay more accurately.
@reviewbybees