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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-41844] Make hasDuplicateHistory efficient #79

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

[JENKINS-41844] Make hasDuplicateHistory efficient #79

wants to merge 19 commits into from

Conversation

fbelzunc
Copy link
Contributor

@fbelzunc fbelzunc commented Nov 29, 2018 •
edited
Loading

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

@fbelzunc fbelzunc closed this Nov 29, 2018
@fbelzunc fbelzunc reopened this Nov 29, 2018
@fbelzunc
Copy link
Contributor Author

@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.

@fbelzunc fbelzunc changed the title [JENKINS-41844] Default values for maxHistoryEntries, maxEntriesPerPage and maxDaysToKeepEntries [WiP][JENKINS-41844] Default values for maxHistoryEntries, maxEntriesPerPage and maxDaysToKeepEntries Dec 3, 2018
@fbelzunc fbelzunc changed the title [WiP][JENKINS-41844] Default values for maxHistoryEntries, maxEntriesPerPage and maxDaysToKeepEntries [WiP][JENKINS-41844] Make hasDuplicateHistory efficient Dec 3, 2018
@Jochen-A-Fuerbacher
Copy link
Member

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.

@fbelzunc fbelzunc closed this Mar 27, 2019
@fbelzunc fbelzunc reopened this Mar 27, 2019
@fbelzunc fbelzunc closed this Mar 27, 2019
@fbelzunc fbelzunc reopened this Mar 27, 2019
@fbelzunc
[JENKINS-41844] Modify testcreateNewHistoryDir() to incorporate the s…
e524b21 
…ymlinks

[JENKINS-41844] Major symblink implementation
@Jochen-A-Fuerbacher
Copy link
Member

Hello @fbelzunc,

please comment here, if you are finish with your changes. Thanks.

@Jochen-A-Fuerbacher Jochen-A-Fuerbacher mentioned this pull request Apr 3, 2019
Merged
@fbelzunc
Copy link
Contributor Author

fbelzunc commented Apr 3, 2019

@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!

@fbelzunc fbelzunc changed the title [WiP][JENKINS-41844] Make hasDuplicateHistory efficient [JENKINS-41844] Make hasDuplicateHistory efficient Apr 30, 2019
@fbelzunc
Copy link
Contributor Author

@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?

@Jochen-A-Fuerbacher
Copy link
Member

@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.

@darxriggs
Copy link
Contributor

@fbelzunc did you already have the time to look at this again?

@fbelzunc
Copy link
Contributor Author

I will try to get back to this next week.

@fbelzunc
Copy link
Contributor Author

fbelzunc commented Aug 6, 2019

@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.

@fbelzunc
Copy link
Contributor Author

fbelzunc commented Aug 6, 2019

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.

@gulyaev13
Copy link

@fbelzunc , @Jochen-A-Fuerbacher , hello!
Any updates on this issue?

@Jochen-A-Fuerbacher
Copy link
Member

@fbelzunc Are you working on this, yet?

@fbelzunc fbelzunc requested a review from a team as a code owner February 28, 2022 17:04
NotMyFault
NotMyFault reviewed Feb 28, 2022
View reviewed changes
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Heyo,

it would help if you can rebase this PR if you request my review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NotMyFault NotMyFault NotMyFault left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fbelzunc @Jochen-A-Fuerbacher @darxriggs @gulyaev13 @NotMyFault