RFR: 1912: Show priority for bugs in pull request body [v2]

Erik Joelsson erikj at openjdk.org
Tue May 23 22:15:15 UTC 2023

On Tue, 23 May 2023 21:08:55 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> The initial purpose of this issue is to show priority for bugs in the issue list in the pull request body. However, with current pr bot, if the user changes the type of the issue or the priority of the issue in the JBS, the changes will not be reflected in the PR body unless there is something could trigger the CheckWorkItem(like user edits the PR title and user issues some commands).
>> As Erik.J suggested, we could maintain a map(from JBS issue to the PRs related with this issue) in memory, so if the issue gets updated, we could know which pr needs to be updated.
>> To query updated issues and handle them, a new bot called `IssueBot` is introduced. This bot would query for updated issues(exclude CSR and JEP) in JBS, and it will read the in memory map to know whether there is any pr needs to be updated. If so, it will generate `CheckWorkItem` for that pr.
>> So currently, there are two ways to generate `CheckWorkItem`. Updated issues and updated pull requests. Previously, in the `CheckWorkItem`, we would check the metadata of the pull request and if the metadata is up to date, `jcheck` would not be triggered.  Now, we could check the metadata of the pull request and the issues related to this pr, but it would be too expensive because if only the pull request is updated, we also need to fetch all the issues from JBS to just generate the metadata. Therefore, in this patch, metadata is split to two parts, one part for pull request and one part for issues. If the `CheckWorkItem` is spawned from an updated pull request, we will only check pr metadata. On the other hand, if the `CheckWorkItem` is spawned from an updated issue, we will only check issues metadata.
>> Besides, since the map is in-memory, so if the bot restarts, the map needs to be initialized. When bot restarts, a `CheckWorkItem` would be generated for each pr, so the initialization is in `CheckWorkItem`.
> Zhao Song has updated the pull request incrementally with seven additional commits since the last revision:
>  - fix some problems
>  - fix a problem
>  - delete IssueWorkItem
>  - change initializedPR to set
>  - fix synchronization
>  - print priority for all issues except CSR and JEP
>  - introduce PRRecord

bots/common/src/main/java/org/openjdk/skara/bots/common/BotUtils.java line 44:

> 42:     }
> 43: 
> 44:     public static Set<String> parseIssues(String body) {

If this method is made public in the common module, then it needs a bit of documentation about what it does (parses issues from PR body and filters out JEP and CSR) and what it returns (list of issue IDs).

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 756:

> 754:                 }
> 755:             }
> 756:             log.info("Map after updated: " + workItem.bot.issuePRMap());

Logging the whole map here is probably not a good idea in production.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 331:

> 329:             }
> 330:             bot.initializedPRs().add(prId);
> 331:             log.info("Map after initialization with pr " + pr.id() + " : " + bot.issuePRMap());

Same here regarding logging. The map will get huge in production.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PRRecord.java line 65:

> 63:         return repoName + "#" + prId;
> 64:     }
> 65: }

With record I meant actually using the new record feature. It's pretty neat.

public record PRRecord(String repoName, String prId);

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java line 339:

> 337:         issuePRMap.putIfAbsent(issueId, new LinkedList<>());
> 338:         synchronized (issuePRMap.get(issueId)) {
> 339:             List<PRRecord> prRecords = issuePRMap.get(issueId);

I would suggest only fetching the list once and then synchronizing on it.


PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1203063932
PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1203076801
PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1203079797
PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1203066945
PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1203068522

More information about the skara-dev mailing list