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

Should throttledPipelinesByCategory be persisted to disk? #71

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

basil
Copy link
Member

@basil basil commented Jan 1, 2020

I am the new maintainer of this plugin and I am trying to understand the intended usage of ThrottleJobProperty.DescriptorImpl#throttledPipelinesByCategory. Should throttledPipelinesByCategory be persisted to disk or not?

The current logic does not persist the descriptor to disk when throttledPipelinesByCategory is changed, but does persist the descriptor to disk (including throttledPipelinesByCategory) whenever one happens to save the descriptor for some other reason, including hitting "Save" from the "Configure System" page. This behavior strikes me as inconsistent and possibly accidental. I can imagine two possible consistent implementations:

  • If throttledPipelinesByCategory should be persisted to disk, then I think the current behavior [of persisting it only when other changes are made to the descriptor] is not sufficient. Rather, if this is the intention then I would expect us to persist the descriptor to disk whenever we modify throttledPipelinesByCategory. This draft PR is an example of such a change.
  • If, on the other hand, throttledPipelinesByCategory should not be persisted to disk, then I think the current behavior [of serializing the field to the XML file] is also incorrect. Rather, if this is the intention then I would expect throttledPipelinesByCategory to be transient so that it is not serialized by XStream, perhaps with some migration logic to remove accidentally persisted entries.

@abayer (since you originally wrote this code) and @oleg-nenashev (since you originally reviewed this code), could either of you shed some light into the intended behavior of throttledPipelinesByCategory? Should it be persisted to disk or not?

@basil basil changed the title Persist throttledPipelinesByCategory to disk whenever it is modified. Should throttledPipelinesByCategory be persisted to disk? Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant