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
Do not swallow exceptions thrown while parsing settings #13039
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,10 +114,9 @@ public static Tuple<Settings, Environment> prepareSettings(Settings pSettings, b | |
} | ||
if (loadFromEnv) { | ||
for (String allowedSuffix : ALLOWED_SUFFIXES) { | ||
try { | ||
settingsBuilder.loadFromPath(environment.configFile().resolve("elasticsearch" + allowedSuffix)); | ||
} catch (SettingsException e) { | ||
// ignore | ||
Path path = environment.configFile().resolve("elasticsearch" + allowedSuffix); | ||
if (Files.exists(path)) { | ||
settingsBuilder.loadFromPath(path); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we should fail if we find several times the config file with different extensions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're right, but I don't know the history of allowing multiple settings formats and whether or not it is used in practice. If we are going to make a breaking change on this, now (pre 2.0) is most definitely the time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I've used this in the past for production ES clusters to have a set of common settings (elasticsearch.yml) and node-specific settings (elasticsearch.json) to merge two files with settings. That said, I still think it's safer/better to remove this feature and fail if more than one config file is found. It reduces the complexity for reasoning where a setting came from. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
+1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll open a separate issue for it. |
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
SKDFLK@$#L%@KL#%L#@$#@L$ #L$@$ #L@K$#L $L $K#L#@L $#L | ||
!!@!@$(#%#)(@)% #(%) | ||
#(%#@)%@#)% (@#%() | ||
()#%@#% (@ )%@%(@#)% @( %)@ %(@) | ||
)(%)@()(%)()(#%)@# | ||
|
||
node.name: "Hiro Takachiho" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like Files.exists in general but personally I think this is the right decision for a good surgical fix!