-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-72107] Introduce Loadable
interface
#8544
Conversation
CloudBees CI uses `load()` method to reload selectively parts of the Jenkins object model, without reloading everything. This formalizes the interface that gets used for this purpose, even if most of the Saveable implementations were using a `load()` method by convention.
@@ -265,7 +266,8 @@ public void doDynamic(StaplerRequest req, StaplerResponse rsp) throws IOExceptio | |||
* | |||
* @since 1.245 | |||
*/ | |||
protected void load() throws IOException { | |||
@Override | |||
public void load() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as this is opening up the load
could it be the case that a plugin has made a previous assumption that this would only be called once at Jenkins startup. Also that it may not be thread safe?
Should this be synchronized
or have you checked for classes that overload this, or made those synchronized where they need to be (there may be none, given this is deprecated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could find the following OSS overloads.
- https://github.com/jenkinsci/global-build-stats-plugin/blob/9867e364d6719cc1e01fe974d3499e29733cf138/src/main/java/hudson/plugins/global_build_stats/GlobalBuildStatsPlugin.java#L47 makes it public
- https://github.com/jenkinsci/docker-traceability-plugin/blob/9d197cc6da0e641e78944441f14ce022f244f550/docker-traceability-plugin/src/main/java/org/jenkinsci/plugins/docker/traceability/DockerTraceabilityPlugin.java#L84-L90 does seem like useless
- https://github.com/jenkinsci/jira-issues-versioning-plugin/blob/73d4a878fcb0bf62c1fd7f75a59dd64f23432be8/hudson-plugin/src/main/java/org/jvnet/hudson/plugins/jira/issueversioning/plugin/hudson/PluginImpl.java#L14 useless
There doesn't seem to be an intensive usage of it (thankfully). Make it synchronized
as well as save()
could be considered (same as Descriptor
) though there seems to be a general point to synchronizing configuration change whether in configure(..)
load()
or save()
/label ready-for-merge This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback. |
CloudBees CI uses
load()
method to reload selectively parts of the Jenkins object model from disk, without reloading everything.This formalizes the interface that gets used for this purpose, even if most of the
Saveable
implementations already had aload()
method by convention.I could not find any use case in the community that could rely on this mecanism. For example, configuration as code uses data binding from casc configuration to live objects, but doesn't need to interact with disk.
See JENKINS-72107.
Testing done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist