Skip to content

Remove multiple paths from NodeEnvironment#72599

Merged
rjernst merged 4 commits intoelastic:masterfrom
rjernst:mdp24
May 3, 2021
Merged

Remove multiple paths from NodeEnvironment#72599
rjernst merged 4 commits intoelastic:masterfrom
rjernst:mdp24

Conversation

@rjernst
Copy link
Copy Markdown
Member

@rjernst rjernst commented May 2, 2021

This commit converts the multiple paths and locks internal to
NodeEnvironment into a singular data path.

relates #71205

This commit converts the multiple paths and locks internal to
NodeEnvironment into a singular data path.

relates elastic#71205
@rjernst rjernst added :Core/Infra/Core Core issues without another label >refactoring v8.0.0 labels May 2, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label May 2, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst rjernst requested a review from jasontedor May 2, 2021 20:35
Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left a comment about the change to the log messages.

totFSPath.add(fsPath);
}
}
if (logger.isInfoEnabled()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should change the log messages here. Can you put it back to the debug/info differentiation, it's only that there's one path to log in each case now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The difference between the two cases was now only a slight difference in format, and the debug case had free space as well. This didn't seem worth a differentiation. Can you elaborate on why we should keep two slightly different log messages for the exact same info?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry I misspoke, one other difference was whether the actual path was printed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mainly because changing the log messages is orthogonal to removing support for multiple data paths in NodeEnvironment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fair enough. I pushed 81ba52c

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I also pushed d914d6a because the plural language of the message no longer makes sense.

Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@rjernst rjernst merged commit b6436c5 into elastic:master May 3, 2021
@rjernst rjernst deleted the mdp24 branch May 3, 2021 18:11
@rjernst rjernst mentioned this pull request Sep 30, 2021
17 tasks
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Oct 4, 2021
This reverts commit b6436c5.

The revert had no merge conflicts.

relates elastic#78525
relates elastic#71205
rjernst added a commit that referenced this pull request Oct 5, 2021
This reverts commit b6436c5.

The revert had no merge conflicts.

relates #78525
relates #71205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants