Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-62755] Introduce Fingerprint Cleanup in External Storage API #4817

Merged
merged 38 commits into from
Jul 21, 2020

Conversation

stellargo
Copy link
Contributor

@stellargo stellargo commented Jun 22, 2020

See JENKINS-62755.
See JEP-226.

Proposed changelog entries

  • Developer: Introduce new FingerprintStorage methods: iterateAndCleanupFingerprints, cleanFingerprint to allow external fingerprint storages to configure fingerprint cleanup.
  • User: Creates a Fingerprint cleanup configuration page which allows enabling/disabling fingerprint cleanup.

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). 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
  • 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 correct
  • 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).

@stellargo
Copy link
Contributor Author

This PR is WIP currently.

@stellargo stellargo changed the title [JENKINS-62755] Introduce Fingerprint cleanup API in Jenkins core [JENKINS-62755] Introduce External Fingerprint cleanup API in Jenkins core Jun 22, 2020
@stellargo stellargo changed the title [JENKINS-62755] Introduce External Fingerprint cleanup API in Jenkins core [JENKINS-62755] Introduce External Fingerprint cleanup API Jun 22, 2020
@daniel-beck daniel-beck added the work-in-progress The PR is under active development, not ready to the final review label Jun 23, 2020
@stellargo
Copy link
Contributor Author

Requesting reviews @oleg-nenashev @afalko @mikecirioli

@stellargo
Copy link
Contributor Author

In this PR, we expose the fingerprint cleanup API for external plugins to implement in the case fingerprint storage plugins want to implement their own fingerprint cleanup. Downstream PR: jenkinsci/redis-fingerprint-storage-plugin#23. We refactor the current fingerprint cleanup into FileFingerprintCleanupThread which can perform the fingerprint cleanup for file system based storage.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current implementation I am not sure why it has to be a separate extension point. Could we just integrate the logic into the FingerprintStorage and invoke it from FingerprintCleanupThread which is universal for all extension points?

cleanFingerprint and execute implementations could be moved to a storage.

@stellargo stellargo requested a review from oleg-nenashev June 24, 2020 22:09
@stellargo stellargo changed the title [JENKINS-62755] Introduce External Fingerprint cleanup API [JENKINS-62755] Introduce Fingerprint Cleanup in External Storage API Jun 24, 2020
@oleg-nenashev
Copy link
Member

@stellargo is it still in progress?

@stellargo
Copy link
Contributor Author

stellargo commented Jul 12, 2020

@oleg-nenashev So it isn't in progress, the work is complete. But it will create merge conflicts with #4834. I think it is better to merge that before this.

@oleg-nenashev oleg-nenashev added on-hold This pull request depends on another event/release, and it cannot be merged right now developer Changes which impact plugin developers and removed work-in-progress The PR is under active development, not ready to the final review on-hold This pull request depends on another event/release, and it cannot be merged right now labels Jul 12, 2020
@oleg-nenashev
Copy link
Member

Unblocking, because #4834 is merged. Merge conflict needs to be resolved tho

@stellargo
Copy link
Contributor Author

Yep currently working on resolving them :)

@oleg-nenashev oleg-nenashev requested a review from afalko July 17, 2020 15:16
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Fingerprint cleanup will not work as written, I suggest inverting it to "Disable Fingerprint cleanup"

The API iteself looks OK for a beta implementation. I would rather create an API method for iterator with handler parameter, but it can be done later. API is Beta anyway

taskListener.getLogger().println("Cleaned up "+numFiles+" records");
}

private boolean cleanFingerprint(File fingerprintFile, TaskListener listener) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is private, it does not override the implementation in upper class. Looks like a 🐛 for API users

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this isn't supposed to override the method of the upper class(signature is different). The method in the upper class isn't called by itself, it is -used- by plugin implementations.

@stellargo
Copy link
Contributor Author

@oleg-nenashev

I would rather create an API method for iterator with handler parameter

So basically asking the fingerprint implementations to provide an iterator, and do the cleanup ourself, is that what you meant?

@oleg-nenashev
Copy link
Member

So basically asking the fingerprint implementations to provide an iterator, and do the cleanup ourself, is that what you meant?

Might be. Anyway, it can be refactored later. I would not block the pull request by it

@stellargo stellargo marked this pull request as draft July 17, 2020 15:54
@stellargo stellargo marked this pull request as ready for review July 17, 2020 17:23
@stellargo stellargo requested a review from oleg-nenashev July 17, 2020 17:26
@stellargo stellargo requested a review from oleg-nenashev July 17, 2020 18:00
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to go. Thanks @stellargo !

<f:section title="${%Fingerprints}">
<f:section title="${%Fingerprints}">
<f:entry title="Disable Fingerprint Cleanup">
<f:checkbox field="fingerprintCleanupDisabled" default="${it.fingerprintCleanupDisabled}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to disable this if there isn’t an external plugin for fingerprints?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good feature to have in case the disk storage is cheaper for a user than having a periodically running process. Or say Jenkins is configured on a distributed disk storage. I don't have any strong opinions though :)

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 20, 2020
@oleg-nenashev
Copy link
Member

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

@oleg-nenashev oleg-nenashev merged commit be28d58 into jenkinsci:master Jul 21, 2020
@daniel-beck daniel-beck mentioned this pull request Jul 27, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants