You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/08/13 00:08:22 UTC
[kudu-CR] Start a background thread to run ResultTracker GC
Todd Lipcon has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3961
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Start a background thread to run ResultTracker GC
Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/service_if.cc
M src/kudu/server/server_base.cc
4 files changed, 37 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/3961/1
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/2924/
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 2:
(1 comment)
How about a unit test to prove that it does what it says on the tin?
http://gerrit.cloudera.org:8080/#/c/3961/2//COMMIT_MSG
Commit Message:
Line 7: Start a background thread to run ResultTracker GC
I was expecting ResultTracker GC to be implemented as an MM op, like other kinds of GC. Why a dedicated thread? Is it because it performs no I/O and thus isn't expected to be long-lived?
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 5:
Build Started http://104.196.14.100/job/kudu-gerrit/2928/
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/2855/
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Start a background thread to run ResultTracker GC
Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Reviewed-on: http://gerrit.cloudera.org:8080/3961
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/service_if.cc
M src/kudu/server/server_base.cc
5 files changed, 54 insertions(+), 25 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3961
to look at the new patch set (#4).
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Start a background thread to run ResultTracker GC
Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
---
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/service_if.cc
M src/kudu/server/server_base.cc
5 files changed, 54 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/3961/4
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3961/3/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
Line 426: void ResultTracker::StartGCThread() {
> Can't we put this in an Init() style method that is assumed to be single-th
I suppose that's true. You OK with my just removing the lock then?
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2849/
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3961
to look at the new patch set (#3).
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Start a background thread to run ResultTracker GC
Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
---
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/service_if.cc
M src/kudu/server/server_base.cc
5 files changed, 53 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/3961/3
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 2:
(1 comment)
will try to write a test. fwiw I've been testing this on a cluster ~2 days now and seems stable. Across ~72 tablet servers none is showing a peak memory consumption of more than 5MB (and averaging around 2MB).
http://gerrit.cloudera.org:8080/#/c/3961/2//COMMIT_MSG
Commit Message:
Line 7: Start a background thread to run ResultTracker GC
> I was expecting ResultTracker GC to be implemented as an MM op, like other
yep, it's assumed that it's cheap enough (and doesnt do IO) so it doesn't need to be "prioritized" against other actions that do IO and thus might contend with each other.
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3961/3/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
Line 426: void ResultTracker::StartGCThread() {
> I suppose that's true. You OK with my just removing the lock then?
Yeah, that's fine with me.
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/2891/
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 2:
I looped exactly_once_writes-itest with this patch 1000 times (100% pass): http://dist-test.cloudera.org//job?job_id=todd.1471058312.30931
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3961/3/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
Line 426: void ResultTracker::StartGCThread() {
Can't we put this in an Init() style method that is assumed to be single-threaded, so that gc_thread_ needn't be protected by lock_?
Actually, a concurrent invocation would crash in the CHECK, so what's the point of the lock?
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Start a background thread to run ResultTracker GC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3961
to look at the new patch set (#5).
Change subject: Start a background thread to run ResultTracker GC
......................................................................
Start a background thread to run ResultTracker GC
Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
---
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/service_if.cc
M src/kudu/server/server_base.cc
5 files changed, 54 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/3961/5
--
To view, visit http://gerrit.cloudera.org:8080/3961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia34ce95e78920596eb8b9db53643845f637c8e6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>