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

Check for default.path.data included in path.data #24285

Merged
merged 3 commits into from
Apr 24, 2017

Conversation

jasontedor
Copy link
Member

If the user explicitly configured path.data to include default.path.data, then we should not fail the node if we find indices in default.path.data. This commit addresses this.

Closes #24283

If the user explicitly configured path.data to include
default.path.data, then we should not fail the node if we find indices
in default.path.data. This commit addresses this.
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

left a suggestion but LGTM otherwise


boolean includedInPathData = false;
for (final NodeEnvironment.NodePath dataPath : nodeEnv.nodePaths()) {
includedInPathData |= Files.isSameFile(dataPath.path, defaultNodeDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe move the loop in a method then we can return early and don't need a label / boolean var in the outer loop

Copy link
Member Author

@jasontedor jasontedor Apr 24, 2017

Choose a reason for hiding this comment

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

I pushed 7d53317.

It is incredibly fortunate that you asked for this. As a result of asking for this, I made the necessary change and tried to run gradle :core:test -Dtests.class=org.elasticsearch.node.NodeTests -Dtests.iters=128 from the command-line to ensure that the change did not break anything. This failed, Gradle told me that no tests were executed. It turns out that org.elasticsearch.node.NodeTests was sitting in the test framework in src/main/java, so these tests were not executing. It appears the class has been like this since July of last year. Fortunately, nothing is broken. I moved this test class to the appropriate location in core.

@jasontedor jasontedor merged commit 1500bea into elastic:master Apr 24, 2017
jasontedor added a commit that referenced this pull request Apr 24, 2017
If the user explicitly configured path.data to include
default.path.data, then we should not fail the node if we find indices
in default.path.data. This commit addresses this.

Relates #24285
jasontedor added a commit that referenced this pull request Apr 24, 2017
If the user explicitly configured path.data to include
default.path.data, then we should not fail the node if we find indices
in default.path.data. This commit addresses this.

Relates #24285
jasontedor added a commit that referenced this pull request Apr 24, 2017
If the user explicitly configured path.data to include
default.path.data, then we should not fail the node if we find indices
in default.path.data. This commit addresses this.

Relates #24285
@jasontedor jasontedor deleted the default-path-data-in-path-data branch April 24, 2017 13:35
@jasontedor
Copy link
Member Author

Thanks @nik9000 and @s1monw.

@nik9000
Copy link
Member

nik9000 commented Apr 24, 2017

It turns out that org.elasticsearch.node.NodeTests was sitting in the test framework in src/main/java, so these tests were not executing.

😨

@nik9000
Copy link
Member

nik9000 commented Apr 24, 2017

I'll look at changing the gradle task that we have for naming conventions to detect that one....

@jasontedor
Copy link
Member Author

I'll look at changing the gradle task that we have for naming conventions to detect that one....

Good call @nik9000, thanks for picking this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants