1565: Only poll for updated MRs from GitLab #1369

erikj79 · 2022-09-02T22:08:08Z

This is another rather big patch, but again, with some explanation, it shouldn't be too scary. The most important part is the new PullRequestPoller class. This class implements a polling mechanism for PullRequests that tries to only return new or updated pull requests to a bot and guaranteeing that none are missed. The goal is to minimize unnecessary evaluations that currently take place, especially when polling Gitlab. I have added quite a few tests to verify (almost) all the functionality of the poller. For a more detailed explanation of all the challenges this poller faces, see the bug description.

In order to reduce impact, I'm not actually making any bot use the new poller yet. I'm filing followup issues to convert them one by one, at least to start. The retry and quarantine features in the poller should cover the current needs for the bots I have inspected.

Most of the rest of the changes are just adding equals and hashCode to a bunch of classes that we now need to compare (to know if a PR has been updated or not). This includes the GitHubPullrequest, GitLabMergeRequest and all the JSON types. For test code this also includes the TestPullRequest and TestIssue and their related data classes.

To test this properly, I needed to refactor some test classes. This work was moved to a separate change in SKARA-1589.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace

Issue

SKARA-1565: Only poll for updated MRs from GitLab

Reviewers

Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to 8aa4d8c9

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/skara pull/1369/head:pull/1369
$ git checkout pull/1369

Update a local copy of the PR:
$ git checkout pull/1369
$ git pull https://git.openjdk.org/skara pull/1369/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1369

View PR using the GUI difftool:
$ git pr show -t 1369

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/skara/pull/1369.diff

bridgekeeper · 2022-09-02T22:08:18Z

👋 Welcome back erikj! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2022-09-02T22:12:42Z

Webrevs

erikj79 · 2022-09-02T22:30:40Z

Please disregard this for now. I discovered a testing issue with any bot using the poller that I need to address properly first.

openjdk · 2022-09-08T20:02:42Z

⚠️ @erikj79 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

erikj79 · 2022-09-08T21:06:20Z

Removing draft. This is now ready for review, but depends on SKARA-1589.

openjdk-notifier · 2022-09-12T12:33:48Z

The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout SKARA-1565-pr-poll
git fetch https://git.openjdk.org/skara master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2022-09-12T12:34:33Z

@erikj79 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout SKARA-1565-pr-poll
git fetch https://git.openjdk.org/skara master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

lgxbslgx · 2022-09-12T12:40:46Z

@erikj79 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:
git checkout SKARA-1565-pr-poll
git fetch https://git.openjdk.org/skara master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

This message was sent to the mail list. Is it a intent? Or should the bot openjdk be added to the ignored list?

erikj79 · 2022-09-12T13:41:09Z

This message was sent to the mail list. Is it a intent? Or should the bot openjdk be added to the ignored list?

Yes, it should. The openjdk-notifier[bot] is on the exclude list for all other repos, but not for the configuration used for the Skara repo itself. I've added it now.

erikj79 · 2022-09-12T13:52:50Z

I'm reducing the scope of this change to just add the new poller, without actually putting it to use yet. There are two followup issues filed already for converting specific bots.

magicus · 2022-09-20T19:28:44Z

forge/src/main/java/org/openjdk/skara/forge/PullRequestPoller.java

+        } else {
+            reviewsMap = Map.of();
+        }
+        return reviewsMap;


Just an aesthetic remark. It looks a bit odd that this method has a local variable instead of returning directly, when it is almost the very same as the method above.

Oh, I see what you mean now. Will fix.

magicus

I think this looks good. You have commented the PollRequestPoller very well, and it is easy to follow (or at least as easy as it can get). Still, this is tricky logic and I can't say with 100% certainty that you are not missing any edge case etc. I think we can go with it, and if there are some issues we will hopefully spot that later.

openjdk · 2022-09-20T20:46:30Z

@erikj79 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

After integration, the commit message for the final commit will be:

1565: Only poll for updated MRs from GitLab

Reviewed-by: ihse

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

erikj79 · 2022-09-20T23:39:14Z

/integrate

openjdk · 2022-09-20T23:39:41Z

Going to push as commit ca8d09a.

openjdk · 2022-09-20T23:39:47Z

@erikj79 Pushed as commit ca8d09a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

SKARA-1565

c6e66de

openjdk bot changed the title ~~SKARA-1565~~ 1565: Only poll for updated MRs from GitLab Sep 2, 2022

openjdk bot added the rfr label Sep 2, 2022

erikj79 changed the title ~~1565: Only poll for updated MRs from GitLab~~ DRAFT: 1565: Only poll for updated MRs from GitLab Sep 2, 2022

erikj79 added 4 commits September 8, 2022 08:55

Test refactor

d594b2f

Test refactor

e5547de

More test fixes

880b348

Merge branch 'SKARA-1589-test-refactor' into SKARA-1565-pr-poll

70c2e17

erikj79 changed the base branch from master to pr/1370 September 8, 2022 20:04

erikj79 added 4 commits September 8, 2022 13:07

Minor adjustments

a31eb56

Unused imports

4387c8c

Merge branch 'SKARA-1589-test-refactor' into SKARA-1565-pr-poll

4009f90

Adding log message

a7df397

erikj79 changed the title ~~DRAFT: 1565: Only poll for updated MRs from GitLab~~ 1565: Only poll for updated MRs from GitLab Sep 8, 2022

openjdk bot mentioned this pull request Sep 8, 2022

1590: Convert PullRequestBot to use new poller #1371

Closed

2 tasks

openjdk-notifier bot changed the base branch from pr/1370 to master September 12, 2022 12:33

openjdk bot added the merge-conflict label Sep 12, 2022

Merge branch 'master' into SKARA-1565-pr-poll

217e191

openjdk bot removed the merge-conflict label Sep 12, 2022

Revert CSRBot, moving it to separate change

c83197f

erikj79 added 4 commits September 12, 2022 07:00

Use new PullRequest API

0242f48

Fix tests

0639d75

Adjusted some comments

603efe2

Use Duration.ZERO

8aa4d8c

magicus reviewed Sep 20, 2022

View reviewed changes

magicus approved these changes Sep 20, 2022

View reviewed changes

openjdk bot added the merge-conflict label Sep 20, 2022

erikj79 added 2 commits September 20, 2022 13:14

Style fix

1dee8c1

Merge branch 'master' into SKARA-1565-pr-poll

f6a35b1

openjdk bot added ready and removed merge-conflict labels Sep 20, 2022

openjdk bot added the integrated label Sep 20, 2022

openjdk bot closed this Sep 20, 2022

openjdk bot removed ready rfr labels Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1565: Only poll for updated MRs from GitLab #1369

1565: Only poll for updated MRs from GitLab #1369

erikj79 commented Sep 2, 2022 •

edited by openjdk bot

bridgekeeper bot commented Sep 2, 2022

mlbridge bot commented Sep 2, 2022 •

edited

erikj79 commented Sep 2, 2022

openjdk bot commented Sep 8, 2022

erikj79 commented Sep 8, 2022

openjdk-notifier bot commented Sep 12, 2022

openjdk bot commented Sep 12, 2022

lgxbslgx commented Sep 12, 2022

erikj79 commented Sep 12, 2022

erikj79 commented Sep 12, 2022

magicus Sep 20, 2022

erikj79 Sep 20, 2022

magicus left a comment

openjdk bot commented Sep 20, 2022

erikj79 commented Sep 20, 2022

openjdk bot commented Sep 20, 2022

openjdk bot commented Sep 20, 2022

1565: Only poll for updated MRs from GitLab #1369

1565: Only poll for updated MRs from GitLab #1369

Conversation

erikj79 commented Sep 2, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 2, 2022

mlbridge bot commented Sep 2, 2022 • edited

Webrevs

erikj79 commented Sep 2, 2022

openjdk bot commented Sep 8, 2022

erikj79 commented Sep 8, 2022

openjdk-notifier bot commented Sep 12, 2022

openjdk bot commented Sep 12, 2022

lgxbslgx commented Sep 12, 2022

erikj79 commented Sep 12, 2022

erikj79 commented Sep 12, 2022

magicus Sep 20, 2022

Choose a reason for hiding this comment

erikj79 Sep 20, 2022

Choose a reason for hiding this comment

magicus left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 20, 2022

erikj79 commented Sep 20, 2022

openjdk bot commented Sep 20, 2022

openjdk bot commented Sep 20, 2022

erikj79 commented Sep 2, 2022 •

edited by openjdk bot

mlbridge bot commented Sep 2, 2022 •

edited