job-config-history-plugin
job-config-history-plugin copied to clipboard
jenkinsci
[JENKINS-41844] Make hasDuplicateHistory efficient
This is the easiest part to fix JENKINS-41844
With the default values on this PR it seems hasDuplicateHistory continue working properly even on slow filesystems
More PRs will come to fix JENKINS-41844
@Jochen-A-Fuerbacher hellom are you sure the failure tests are related to this PR? I think there might be environment related - Those tests corretly run on my local environment.
Hello @fbelzunc,
sorry for taking so long to answer. I took a look into the test failures. The tests also fail on my environment. Reason is, that you create a symlink in the history dir and three of the tests check the file count in the history dir. So they fail because there's one more file, now: the symlink. Please fix the three tests. Thanks.
Hello @fbelzunc,
please comment here, if you are finish with your changes. Thanks.
@Jochen-A-Fuerbacher Not for the moment, I am using symlinks and I will need to go away from this approach. I will ping you once I am done. Thanks for the follow-up!
@Jochen-A-Fuerbacher I think I am done, however, I think this is an important change in the plugin and I am a bit worried about cases I might not be contemplating.
Basically, I am
- Introducing/updating a symbolic link each time a new history is created.
- When checking if there are duplicated histories, I fall-back to the previous behavior in case there is not still a symlink or in case there was an issue reading the symlink
One of the reasons why I am worried is because in Windows environments, it seems the symlinks do not work - see https://issues.jenkins-ci.org/browse/JENKINS-37862
I short, I would like to know if in your opinion this PR is using - or not the right approach. I know this is a BIG issue on big organizations, so that is the reason why I am trying to address the issues.
What do you think?
@fbelzunc There's a bug in your PR: If you have the purger enabled (global Jenkins configuration -> Job Config History -> Advanced -> Max number of days to keep history entries set to some value) and all matching history entries get deleted, the symlink doesn't get updated to the last available history entry (which is older than the previous existing history entries). You need to modify the JobConfigHistoryPurger class as well.
The rest of your PR looks good. As I am not a Windows user, I cannot simply verify the impact of your PR on Windows environments.
@Jochen-A-Fuerbacher I am deleting now the symblink in case it is needed, but I am not updating it. Why? Because...
- when there are still entries without being deleted, the symblink should be pointing to the most recent one already.
- In any case, if the symblink is deleted it should be recreated after adding a new entry, or after passing through hasDuplicated.
I will re-review everything tomorrow, but I still don't understand why this change does not have an impact on the maximum number of configuration history entries to keep. I did not see on the code where we are taking care of this.
