RFR: 1598: GitRepository.isHealthy takes a long time in ArchiveWorkItem [v3]

[Erik Joelsson]

On Wed, 14 Dec 2022 12:03:41 GMT, Magnus Ihse Bursie <ihse at openjdk.org> wrote:

>> Erik Joelsson has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review comments
>
> bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java line 252:
> 
>> 250:         var sentMails = new ArrayList<Email>();
>> 251:         var archiveContents = bot.archiveRepo().fileContents(mboxFile(), bot.archiveRef());
>> 252:         archiveContents.ifPresent(s -> sentMails.addAll(Mbox.splitMbox(s, bot.emailAddress())));
> 
> So this adds the old contents to sentMails, and if there is none, we just ignore adding it? I needed some time to figure out why it was safe doing nothing if archiveContents was empty. Perhaps a comment about this?

I didn't reflect on that as it's the same logic as before, only before it was handled with the ignored `IOException`, but I can add a comment.

> forge/src/main/java/org/openjdk/skara/forge/HostedRepository.java line 100:
> 
>> 98:      * @param authorEmail Email of author and committer for commit
>> 99:      */
>> 100:     void writeFileContents(String content, String filename, Branch branch, String message, String authorName, String authorEmail);
> 
> Maybe not a big deal, but I thought the symmetry with `fileContents` was a bit lost when `String filename` was not the first argument to this method.

Sure, makes sense.

> forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java line 292:
> 
>> 290:                         .put("name", authorName)
>> 291:                         .put("email", authorEmail))
>> 292:                 .put("content", new String(Base64.getEncoder().encode(content.getBytes(StandardCharsets.UTF_8)), StandardCharsets.UTF_8));
> 
> Are we talking about 10's of MB here as well? We don't risk running into limits?

Hm, I just realized the comment on fileContents is wrong, with raw we get up to 100MB, so I will fix that. I can't find any documentation regarding a limit when writing file contents. The manual test I have tests up to 10MB, which was fine. The biggest file in our current archive is just over 1MB. The current archive is however hosted on GitLab and not GitHub, so the GitHub limits aren't really relevant at this time.

> forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java line 334:
> 
>> 332:                         return Optional.of(request.post("repository/files/" + encodedFileName)
>> 333:                                 .body(body)
>> 334:                                 .execute());
> 
> You don't need an `onError` to return `Optional.empty()` for the POST case? Or is it the default if you don't specify an error handler?

The default for `execute()` is to throw `UncheckedRestException` if we get any kind of error response from the server. Here I made the assumption that replacing files is more common than creating new files, and simply first try PUT to replace a file and if that fails with the documented error code 400, we retry with POST to create a new file. If the POST fails, then we just let it fail.

Another way of doing it would have been to first call GET to see if the file exists and then pick either PUT or POST depending on the result. I just tried to keep the number of calls down for the current common case.

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

PR: https://git.openjdk.org/skara/pull/1442