RFR: 2063: Reduce the health check times of local repo instance

[Erik Joelsson]

On Thu, 12 Oct 2023 20:50:17 GMT, Zhao Song <zsong at openjdk.org> wrote:

> In the method HostedRepositoryPool#materializeClone, when the bot is trying to reuse a local repo instance, the bot will always check whether the repo is still good by processing command "git fsck --connectivity-only".
> 
> Sometimes this command would be slow and Erik said that we should believe the bots are doing right things, so we should assume the local repos are good, so the health check is not always needed. But shutdown of the bot could make a local repo instance unhealthy, so we should at least do the health check once for each local repo instance after the bot is restarted.
> 
> To solve this issue, Erik suggested to maintain a static map for keeping track of paths of known checked/good repositories.
> 
> To leverage this patch to mlbridge bot, I also made some changes to WebrevStorage.

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java line 334:

> 332:     WebrevGenerator generator(PullRequest pr, Repository localRepository, Path scratchPath, HostedRepositoryPool hostedRepositoryPool) throws IOException {
> 333:         var jsonLocalStorage = jsonStorage == null ? null : hostedRepositoryPool.checkout(jsonStorage, storageRef, scratchPath);
> 334:         var htmlLocalStorage = htmlStorage == null ? null : hostedRepositoryPool.checkout(htmlStorage, storageRef, scratchPath);

This will cause us to always materialize the repository, regardless of if webrevs will be generated. I still want it to be done lazily. You can add a method here that initializes the local storage fields and call it from the generate methods below.

forge/src/main/java/org/openjdk/skara/forge/HostedRepositoryPool.java line 48:

> 46:         private final HostedRepository hostedRepository;
> 47:         private final Path seed;
> 48:         private static Set<String> healthySet = new HashSet<>();

Should probably store `Path` in the set so we don't need to convert to string.

forge/src/main/java/org/openjdk/skara/forge/HostedRepositoryPool.java line 153:

> 151:                 if (!isHealthy(localRepoInstance, path)) {
> 152:                     removeOldClone(path, "unhealthy");
> 153:                     return cloneSeeded(path, allowStale, bare);

I think we should also add a path to the healthy set after a successful clone. Perhaps at the end of `cloneSeeded`.

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1569#discussion_r1357470918
PR Review Comment: https://git.openjdk.org/skara/pull/1569#discussion_r1357460471
PR Review Comment: https://git.openjdk.org/skara/pull/1569#discussion_r1357464372