jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

Allow Formchecker to check more than 1 thing at a time

Open mawinter69 opened this issue 2 years ago • 4 comments

The FormChecker is currently limiting the number of checks to one at a time. While for most forms this is fine for the matrix auth and the role-strategy this makes their config pages very slow to load. When 300 users are configured against an Active Directory it takes for me well over 10 minutes until for each sid it is decided if it is a user or group or doesn't exist. Changing this needs to handled with care by those plugins. Browsers limit the number of parallel connections one can open to the same host usually with http1.1 But with http2 one can see big performance improvements. Using a value of 50 and a role with 300 users the loading of user information dropped from over 10 min to about 1 min for me.

Proposed changelog entries

  • Allow Formchecker to check more than one thing at a time

Proposed upgrade guidelines

N/A

Submitter checklist

  • [ ] (If applicable) Jira issue is well described
  • [ ] Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • [ ] Appropriate autotests or explanation to why this change has no tests
  • [ ] New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • [ ] New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • [ ] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • [ ] There are at least 2 approvals for the pull request and no outstanding requests for change
  • [ ] Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • [ ] Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • [ ] Proper changelog labels are set so that the changelog can be generated automatically
  • [ ] If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

mawinter69 avatar Aug 01 '22 16:08 mawinter69

Downstream PR: https://github.com/jenkinsci/role-strategy-plugin/pull/228 This is just a PoC with the FormChecker copied over (with some not so nice code as one method called by FormChecker has changed between 2.303 as used by role-strategy and current weekly) The idea was to make it configurable how many parallel checks are allowed, setting it to 3 or 4 might also speed up things while on http1.1 though not so much, haven't checked that yet in detail

mawinter69 avatar Aug 01 '22 19:08 mawinter69

This is just a PoC with the FormChecker copied over

You can create linked PRs that actually use new API by changing the dependency to the upstream PR's incrementals deployment. See https://github.com/jenkinsci/jenkins/pull/6951/checks?check_run_id=7621275049 for the current one for this PR.

daniel-beck avatar Aug 02 '22 11:08 daniel-beck

I really like where this is going :)

daniel-beck avatar Aug 02 '22 22:08 daniel-beck

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Aug 25 '22 20:08 github-actions[bot]

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

basil avatar Oct 12 '22 17:10 basil