-
Notifications
You must be signed in to change notification settings - Fork 392
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
Print informational message when kernel.core_pattern is piped #6838
Conversation
Local test results: When
|
Not sure who to follow-up on this with. Perhaps @babsingh, can you please review? |
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.
Highlighted places in the modified code where the coding standard is not satisfied, and minor nitpicks.
The commit message and PR description do not follow the Commit Guidelines: https://github.com/eclipse/omr/blob/master/CONTRIBUTING.md#commit-guidelines.
|
re #6838 (comment):
|
Preliminary testing to verify the functionality of the changes. jenkins build all |
I also recommend to run an OpenJ9 personal build since some OpenJ9 tests look at the output of core file generation. The following |
It's convention to use two spaces between sentences, so I think the "extra space" should stay. |
Then, we should add two spaces in the below message:
|
That is a matter for the OpenJ9 project (the text you quote is not in this repository). Did you mean to suggest an extra space on line 240? If so, I agree. |
The first message, However, I think If
As per the other comments, I'll update to double-spaces.
These messages are in the
I agree in general, although the underlying issue is that end users are misunderstanding these messages and thinking core dumps aren't being produced in these cases, so I'd rather lean towards some redundancy. For example, I see support cases where just one of the messages related to producing the dump is extracted out of the logs and put in the case instead of all the messages. So I'd rather keep the redundancy, but if others feel strongly otherwise, I can remove it (although these messages are in the |
Thanks, it looks like you ran this test. I see one test failed (
Looks like this is a known issue: #6704 |
I did a preliminary test with the additional proposed change and it works as I guessed:
|
LGTM, all of my suggested changes have been addressed. @keithc-ca Does this PR look good to you? re #6838 (comment) and #6838 (comment):
re #6838 (comment):
|
@kgibm Thanks for addressing the feedback. Please squash the commits once @keithc-ca approves the PR. Afterwards, I will launch the PR builds and merge the PR. |
I think this change is good, but the description and the (squashed) commit message should be updated, in particular the phrase "no longer treated as an error" which is now inaccurate. |
On Linux, when a core dump is requested, if kernel.core_pattern is a piped pattern, then print a new informational message JVMPORT049I to clarify the subsequent JVMDUMP012E message that although the JVM cannot find the core dump, the user should review the manual for the program specified in core_pattern to potentially find the core. Signed-off-by: Kevin Grigorenko <kevin.grigorenko@us.ibm.com>
Squashed and updated commit message and issue description |
jenkins build all |
On Linux, when a core dump is requested, if kernel.core_pattern is
a piped pattern, then print a new informational message JVMPORT049I
to clarify the subsequent JVMDUMP012E message that although the JVM
cannot find the core dump, the user should review the manual for
the program specified in core_pattern to potentially find the core.
Fixes #6837
CC @pshipton @keithc-ca