-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Don't close stderr under --quiet
#47208
Changes from 1 commit
49ea53b
ca2bc1f
fcdada1
5e74b75
f3973b3
950cfc0
fad0948
9513f7f
4091c55
de27e70
ca99dc3
41d71f2
1a255ae
52bb829
5fabb63
8850f14
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 |
---|---|---|
|
@@ -91,6 +91,12 @@ public void checkPermission(Permission perm) { | |
final Elasticsearch elasticsearch = new Elasticsearch(); | ||
int status = main(args, elasticsearch, Terminal.DEFAULT); | ||
if (status != ExitCodes.OK) { | ||
Terminal.DEFAULT.errorPrintln("ERROR: Elasticsearch did not exit normally - check the logs at " + | ||
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 it's possible that we can fail before these system properties have been set. 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. The problem here is that we can fail before we’ve configured logging, and so those system properties would not be set giving an empty error message to the user. The guarantee that we have here though is that if logging is not yet been configured, then no log messages will have been written (we have a check that ensures we don't touch logging before it's configured, and we fail startup if it is). That means that we should skip these message if the logging properties are not set, because we know from our guarantees that there won't be anything in the logs to look at. 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. @jasontedor I've added guards around both places where I'm printing the log file location. |
||
System.getProperty("es.logs.base_path") + | ||
System.getProperty("file.separator") + | ||
System.getProperty("es.logs.cluster_name") + | ||
".log" | ||
); | ||
exit(status); | ||
} | ||
} | ||
|
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.
It's puzzling to me that flush only flushes the error stream but not the output stream?