-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
bugfix: use system time and current folder to jstack #3931
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3931 +/- ##
=============================================
- Coverage 40.76% 40.74% -0.02%
+ Complexity 3067 3066 -1
=============================================
Files 682 682
Lines 22948 22950 +2
Branches 2856 2856
=============================================
- Hits 9355 9352 -3
- Misses 12722 12726 +4
- Partials 871 872 +1
|
try { | ||
Runtime.getRuntime().exec("jstack " + pid + " >d:/" + idx + ".log"); | ||
Runtime.getRuntime().exec("jstack " + pid + " > " + idx + ".log"); |
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.
may be no permission, use current execute user home dir?
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.
and log.info the command?
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.
may be no permission, use current execute user home dir?
I think this operation will dump the file to the folder of the running path?
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.
recommended to add logging record commands. view the stack information path from the log information.
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.
done
rename pr title |
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.
@leizhiyuan a small problem about code style, please fix it.
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.
LGTM
Ⅰ. Describe what this PR did
use System.currentTimeMillis() as id, and jstack dump to current folder.
Ⅱ. Does this pull request fix one issue?
fixes #3922
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews