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

Use DirectoryStream paths in java 7 to traverse the users. #2251

Closed
wants to merge 3 commits into from

Conversation

christ66
Copy link
Member

With the move to Java 7 the nio2 library is made available for usage. The DirectoryStream api allows for a more effeicient method of walking the directory tree structure. This fixes a potential performance issue where there are several thousand users in the $JENKINS_HOME/users directory which are still on the legacy format for user ids. This uses the NIO2 methods for doing the tree walk and moving the directories into the correct locations.

@reviewbybees

With the move to Java 7 the nio2 library is made available for usage. The DirectoryStream api allows for a more effeicient method of walking the directory tree structure. This fixes a potential performance issue where there are several thousand users in the $JENKINS_HOME/users directory which are still on the legacy format for user ids. This uses the NIO2 methods for doing the tree walk and moving the directories into the correct locations.
@ghost
Copy link

ghost commented Apr 11, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@christ66 christ66 added the needs-more-reviews Complex change, which would benefit from more eyes label Apr 11, 2016
new Object[]{legacyUserDir, configFile.getParentFile()});

try {
DirectoryStream<Path> legacy = getLegacyConfigFilesFor(id);
Copy link
Member

Choose a reason for hiding this comment

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

🐛 legacy is not closed. Use try-with-resources.

@christ66 christ66 added work-in-progress The PR is under active development, not ready to the final review and removed needs-more-reviews Complex change, which would benefit from more eyes labels Apr 18, 2016
@christ66
Copy link
Member Author

I think the failing unit test is part of these code changes. Marking as WIP.

@jglick
Copy link
Member

jglick commented May 9, 2016

What is the motivation behind this change? The code in question is only ever run once, during an upgrade from an old Jenkins version. Do we really care about optimizing it?

@christ66
Copy link
Member Author

@jglick I've seen this triggered via the api (with the git plugin).

@oleg-nenashev oleg-nenashev added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 11, 2016
@jglick
Copy link
Member

jglick commented Nov 2, 2017

JENKINS-47429 seems to track this issue.

@jglick
Copy link
Member

jglick commented Nov 28, 2017

Obsolete as of #3150.

@jglick jglick closed this Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unresolved-merge-conflict There is a merge conflict with the target branch. work-in-progress The PR is under active development, not ready to the final review
Projects
None yet
4 participants