Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.

8289183: jdk.jfr.consumer.RecordedThread.getId references Thread::getId, should be Thread::threadId #93

Closed
wants to merge 4 commits into from
Closed

8289183: jdk.jfr.consumer.RecordedThread.getId references Thread::getId, should be Thread::threadId #93

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5a7802
Initial
egahlin Jun 29, 2022
3819c94
Change from 'The' to 'the'
egahlin Jun 29, 2022
49b33e6
Add link to java.lang.Thread#threadId() in getJavaThreadId()
egahlin Jul 1, 2022
89a4bd0
Add link to getOSThreadId() to RecordedThread::getId()
egahlin Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/jdk.jfr/share/classes/jdk/jfr/consumer/RecordedThread.java
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@ public String getOSName() {
/**
* Returns the thread ID used by the operating system.
*
* @return The Java thread ID, or {@code -1} if doesn't exist
* @return the OS thread ID, or {@code -1} if doesn't exist
*/
public long getOSThreadId() {
Long l = getTyped("osThreadId", Long.class, -1L);
Expand Down Expand Up @@ -86,6 +86,8 @@ public String getJavaName() {
* Returns the Java thread ID, or {@code -1} if it's not a Java thread.
*
* @return the Java thread ID, or {@code -1} if it's not a Java thread
*
* @see java.lang.Thread#threadId()
*/
public long getJavaThreadId() {
Long l = getTyped("javaThreadId", Long.class, -1L);
Expand All @@ -97,7 +99,7 @@ public long getJavaThreadId() {
* reused within the lifespan of the JVM.
* <p>
* See {@link #getJavaThreadId()} for the ID that is returned by
* {@code java.lang.Thread.getId()}
* {@code java.lang.Thread.threadId()}
Copy link

Choose a reason for hiding this comment

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

Looks okay but maybe it should be a link?

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered it, but the purpose of the comment is to help users not mix up getId() with getJavaThreadId() in the RecordedThread class. If users follows a link to java.lang.Thread and read about the two IDs there, it might confuse them. The difference between getId() and threadId() in j.l.Thread is not the same as in RecordedThread.

That said, if you think a link would be helpful I can add it.

Copy link

Choose a reason for hiding this comment

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

It might help to have "Java thread ID" in the RecordedThread::getThreadId descriptoin link to Thread::threadId. For RecordedThread::getId then maybe it just needs a @see #getThreadId() with no reference or link to j.l.Thread methods. But up to you, I don't have a strong opinion on this except that developers using this API understand that consuming a recording means dealing with non-Java threads and they have an ID too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I added a link Thread::threadId() to RecordedThread::getJavaThreadId(), but didn't link RecordedThread::getId(). I also added a link to getOSThreadId() from getId().

*
* @return a unique ID for the thread
*/
Expand Down