You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2019/09/26 05:14:42 UTC
[kudu-CR] [thread] Fix the comment of Thread
lingbinlb@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14308
Change subject: [thread] Fix the comment of Thread
......................................................................
[thread] Fix the comment of Thread
In the current implementation(i.e. from commit 9fa9cf181d),
the handshake is no longer needed between the parent thread
and the child thread, but the comments are not updated
in time, which can lead to misunderstanding
Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
---
M src/kudu/util/thread.h
1 file changed, 16 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/14308/1
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14308 )
Change subject: [thread] Fix the comment of Thread
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14308/3/src/kudu/util/thread.h
File src/kudu/util/thread.h:
http://gerrit.cloudera.org:8080/#/c/14308/3/src/kudu/util/thread.h@354
PS3, Line 354: ven the
> Should it be "...even after the..."?
Ah, my mistake. Yes, "even after the..."
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 27 Sep 2019 05:17:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14308
to look at the new patch set (#3).
Change subject: [thread] Fix the comment of Thread
......................................................................
[thread] Fix the comment of Thread
In the current implementation(i.e. from commit 9fa9cf181d),
the handshake is no longer needed between the parent thread
and the child thread, but the comments are not updated
in time, which can lead to misunderstanding
Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
---
M src/kudu/util/thread.h
1 file changed, 13 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/14308/3
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14308 )
Change subject: [thread] Fix the comment of Thread
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14308/3/src/kudu/util/thread.h
File src/kudu/util/thread.h:
http://gerrit.cloudera.org:8080/#/c/14308/3/src/kudu/util/thread.h@354
PS3, Line 354: ven the
> Ah, my mistake. Yes, "even after the..."
Done
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 27 Sep 2019 06:37:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14308 )
Change subject: [thread] Fix the comment of Thread
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 27 Sep 2019 22:25:38 +0000
Gerrit-HasComments: No
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14308
to look at the new patch set (#4).
Change subject: [thread] Fix the comment of Thread
......................................................................
[thread] Fix the comment of Thread
In the current implementation(i.e. from commit 9fa9cf181d),
the handshake is no longer needed between the parent thread
and the child thread, but the comments are not updated
in time, which can lead to misunderstanding
Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
---
M src/kudu/util/thread.h
1 file changed, 13 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/14308/4
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14308
to look at the new patch set (#2).
Change subject: [thread] Fix the comment of Thread
......................................................................
[thread] Fix the comment of Thread
In the current implementation(i.e. from commit 9fa9cf181d),
the handshake is no longer needed between the parent thread
and the child thread, but the comments are not updated
in time, which can lead to misunderstanding
Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
---
M src/kudu/util/thread.h
1 file changed, 13 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/14308/2
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14308 )
Change subject: [thread] Fix the comment of Thread
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h
File src/kudu/util/thread.h:
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h@111
PS1, Line 111: // Each Thread object knows its operating system thread ID (TID) by caching it in
> This change seems unnecessary as tid_ is an implementation detail. What add
Done
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h@208
PS1, Line 208: Howerve
> However
Done
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h@209
PS1, Line 209: the
> Drop 'the'
Done
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h@353
PS1, Line 353: // The reference count of the Thread object has already been incremented
: // in StartThread(), so it is safe to refer to it.
> Having a hard time seeing what this is trying to say. When is it safe to re
Done
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 26 Sep 2019 06:54:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14308 )
Change subject: [thread] Fix the comment of Thread
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14308/3/src/kudu/util/thread.h
File src/kudu/util/thread.h:
http://gerrit.cloudera.org:8080/#/c/14308/3/src/kudu/util/thread.h@354
PS3, Line 354: ven the
> "...even before the..."
Should it be "...even after the..."?
Because the Thread object is newed in the parent thread(StartThread()), and StartThread() may finish before SuperviseThread().
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 27 Sep 2019 02:24:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14308 )
Change subject: [thread] Fix the comment of Thread
......................................................................
[thread] Fix the comment of Thread
In the current implementation(i.e. from commit 9fa9cf181d),
the handshake is no longer needed between the parent thread
and the child thread, but the comments are not updated
in time, which can lead to misunderstanding
Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Reviewed-on: http://gerrit.cloudera.org:8080/14308
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/util/thread.h
1 file changed, 13 insertions(+), 18 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14308 )
Change subject: [thread] Fix the comment of Thread
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h
File src/kudu/util/thread.h:
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h@111
PS1, Line 111: // Each Thread object knows its operating system thread ID (TID) by caching it in
This change seems unnecessary as tid_ is an implementation detail. What additional information does it add?
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h@208
PS1, Line 208: Howerve
However
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h@209
PS1, Line 209: the
Drop 'the'
http://gerrit.cloudera.org:8080/#/c/14308/1/src/kudu/util/thread.h@353
PS1, Line 353: // The reference count of the Thread object has already been incremented
: // in StartThread(), so it is safe to refer to it.
Having a hard time seeing what this is trying to say. When is it safe to refer to the Thread object? Why is it not obvious that this is safe?
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 26 Sep 2019 05:49:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [thread] Fix the comment of Thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14308 )
Change subject: [thread] Fix the comment of Thread
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14308/3/src/kudu/util/thread.h
File src/kudu/util/thread.h:
http://gerrit.cloudera.org:8080/#/c/14308/3/src/kudu/util/thread.h@354
PS3, Line 354: ven the
"...even before the..."
--
To view, visit http://gerrit.cloudera.org:8080/14308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib493fa0e33bb55cf436d7894cc87854337919854
Gerrit-Change-Number: 14308
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 26 Sep 2019 17:13:23 +0000
Gerrit-HasComments: Yes