You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yifan Zhang (Code Review)" <ge...@cloudera.org> on 2019/12/05 12:16:36 UTC
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Yifan Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14849
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
KUDU-2992: Avoid sending duplicated requests in catalog_manager
When the master sent a large number of rpc requests to a tserver,
and the tserver didn't response immediately, catalog manager may
continue to send duplicate requests in ProcessTabletReport(), that
would cause rpc timeouts and high load on both masters and tservers.
This patch use task description to identify a unique rpc task,
catalog manager would not send duplicate request if the same task
didn't complete.
This patch also fix HandleResponse() in AsyncDeleteReplica to avoid
unnecessary retries.
Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
4 files changed, 43 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/14849/1
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 1
Gerrit-Owner: Yifan Zhang <ch...@163.com>
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (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/14849
to look at the new patch set (#4).
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
KUDU-2992: Avoid sending duplicated requests in catalog_manager
When the master sent a large number of rpc requests to a tserver,
and the tserver didn't response immediately, catalog manager may
continue to send duplicate requests in ProcessTabletReport(), that
would cause rpc timeouts and high load on both masters and tservers.
This patch add a tablet_id index to pending_tasks, catalog manager
would not send duplicate requests to a tablet with the same alter type
when handling a tablet report from a tablet server. If the existing
RPC task failed, catalog manager would create it the next time the
tablet server heartbeats.
This patch also fix HandleResponse() in AsyncDeleteReplica to avoid
unnecessary retries.
Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
5 files changed, 193 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/14849/4
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 1:
(4 comments)
I'm a little nervous about using the description as a unique key. They look immutable and unique, but they're also just supposed to be descriptive.
Could you add a test showing that this works the way you expect?
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc@3507
PS1, Line 3507: MarkComplete();
Will a new deletion task be created the next time the tserver heartbeats? Or will that tablet be missing from the tablet report?
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc@4314
PS1, Line 4314: // This is a floating task (since the table does not exist) created in
: // response to a tablet report.
: rpc->AddRef();
Do we even have these anymore? I don't think we do. Could simplify this code a bit.
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc@5529
PS1, Line 5529: LOG(WARNING) << "Do not send duplicated request: " + task->description();
Seems like this will be noisy. Throttle or VLOG?
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc@5533
PS1, Line 5533: pending_tasks_.emplace(task->description(), task);
Didn't we just do this on L5528?
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 1
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 05 Dec 2019 21:12:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 3:
> Patch Set 2:
>
> (4 comments)
>
> ASAN tests show a memory leak.
fixed
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 10 Dec 2019 13:47:55 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14849/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14849/2//COMMIT_MSG@16
PS2, Line 16: existing
> existing
Done
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@4363
PS2, Line 4363:
> Add a space before 'c'
Done
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@5511
PS2, Line 5511:
> Old style was correct: "MonitoredTask* task", not "MonitoredTask *task"
Done
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@5519
PS2, Line 5519: break;
> Don't need std:: prefixes in .cc files.
Done
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 10 Dec 2019 13:47:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc@3490
PS4, Line 3490:
> There's another case below us, so maybe remove this.
Done
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc@3499
PS4, Line 3499:
: target_ts_desc_->ToString()
> Swap these two arguments
Done
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc@3569
PS4, Line 3569: return Substitute("AlterTable RPC for tablet $0 (table $1,
> Do you think it's worth including the schema version number? Or, because Al
I think maybe it's better to include the schema version, since the schema version number could distinguish different AlterTable requests.
If the master has already sent a AlterTable request to a tablet but it hasn't been done, and the table schema altered again, it's better to send another AlterTable request when processing the next TabletReports from tservers.
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc@5561
PS4, Line 5561:
> Unnecessary parentheses here?
Done
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/master-test.cc@1796
PS4, Line 1796: ReplicaManagementInfoPB::PREPARE_REPLACEMENT_BEFORE_EVICTION);
:
: {
> nit: could just use PREPARE_REPLACEMENT_BEFORE_EVICTION, since that's the m
Done
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 5
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Thu, 12 Dec 2019 13:13:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 3:
(2 comments)
Still hoping you can add a test that shows the fix in action.
http://gerrit.cloudera.org:8080/#/c/14849/3/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/14849/3/src/kudu/master/catalog_manager.h@373
PS3, Line 373: std::unordered_multimap<std::string, scoped_refptr<MonitoredTask> > pending_tasks_;
You can do >> in C++11 (previous versions of C++ required a space between the triangular brackets or they wouldn't parse).
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@5531
PS2, Line 5531: void TableInfo::WaitTasksCompletion() {
: int wait_time = 5;
: while (1) {
Isn't this still a valid concern? That is, if the last ref to the TableInfo is in 'task', and pending_tasks_.erase() deletes the last ref to 'task', won't that cause 'this' to be deleted and the destructor call of 'l' to access freed memory?
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 10 Dec 2019 19:15:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@5531
PS2, Line 5531: void TableInfo::WaitTasksCompletion() {
: int wait_time = 5;
: while (1) {
> It seems impossible that the last ref to the TableInfo is in 'task', at lea
That's not entirely true; VisitTablesAndTabletsUnlocked will clear out the TableInfo maps, which could cause them to be destroyed.
That said, it'll also abort and wait for all tasks before doing so, meaning ReleaseTask called from UnregisterAsyncTask can't be called concurrently.
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc@5561
PS4, Line 5561: tasks->push_back((task.second));
Unnecessary parentheses here?
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Wed, 11 Dec 2019 21:26:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc@3507
PS1, Line 3507: TabletServerErrorPB::Code_Name(code), status.ToString());
> Will a new deletion task be created the next time the tserver heartbeats? O
Here we just abondon retrying to send the request, if the tablet hasn't been deleted and it report to master, the deletion task will be created again.
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc@4314
PS1, Line 4314:
: return Status::OK();
: }
> Do we even have these anymore? I don't think we do. Could simplify this cod
Done
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc@5529
PS1, Line 5529: }
> Seems like this will be noisy. Throttle or VLOG?
Done
http://gerrit.cloudera.org:8080/#/c/14849/1/src/kudu/master/catalog_manager.cc@5533
PS1, Line 5533: task->Release();
> Didn't we just do this on L5528?
Done
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 13:24:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc@3490
PS4, Line 3490: , otherwise retry forever or until cancelled.
There's another case below us, so maybe remove this.
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc@3499
PS4, Line 3499: tablet_id_,
: target_ts_desc_->ToString()
Swap these two arguments
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/catalog_manager.cc@3569
PS4, Line 3569: return "AlterTable RPC for tablet " + tablet_->ToString();
Do you think it's worth including the schema version number? Or, because AlterSchema is supposed to be serialized, will the schema version number not matter, since the master will only sent a single alter request at once?
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/14849/4/src/kudu/master/master-test.cc@1796
PS4, Line 1796: FLAGS_raft_prepare_replacement_before_eviction
: ? ReplicaManagementInfoPB::PREPARE_REPLACEMENT_BEFORE_EVICTION
: : ReplicaManagementInfoPB::EVICT_FIRST
nit: could just use PREPARE_REPLACEMENT_BEFORE_EVICTION, since that's the mode we expect almost everyone to use.
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Thu, 12 Dec 2019 01:53:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 2:
(4 comments)
ASAN tests show a memory leak.
http://gerrit.cloudera.org:8080/#/c/14849/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14849/2//COMMIT_MSG@16
PS2, Line 16: exsiting
existing
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@4363
PS2, Line 4363: c
Add a space before 'c'
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@5511
PS2, Line 5511: MonitoredTask *task
Old style was correct: "MonitoredTask* task", not "MonitoredTask *task"
Elsewhere too.
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@5519
PS2, Line 5519: void TableInfo::RemoveTask(const std::string& tablet_id, MonitoredTask *task) {
Don't need std:: prefixes in .cc files.
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 20:04:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 6
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 16 Dec 2019 21:19:59 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14849
to look at the new patch set (#5).
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
KUDU-2992: Avoid sending duplicated requests in catalog_manager
When the master sent a large number of rpc requests to a tserver,
and the tserver didn't response immediately, catalog manager may
continue to send duplicate requests in ProcessTabletReport(), that
would cause rpc timeouts and high load on both masters and tservers.
This patch add a tablet_id index to pending_tasks, catalog manager
would not send duplicate requests to a tablet with the same alter type
when handling a tablet report from a tablet server. If the existing
RPC task failed, catalog manager would create it the next time the
tablet server heartbeats.
This patch also fix HandleResponse() in AsyncDeleteReplica to avoid
unnecessary retries.
Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
5 files changed, 198 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/14849/5
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 5
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14849
to look at the new patch set (#6).
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
KUDU-2992: Avoid sending duplicated requests in catalog_manager
When the master sent a large number of rpc requests to a tserver,
and the tserver didn't response immediately, catalog manager may
continue to send duplicate requests in ProcessTabletReport(), that
would cause rpc timeouts and high load on both masters and tservers.
This patch add a tablet_id index to pending_tasks, catalog manager
would not send duplicate requests to a tablet with the same alter type
when handling a tablet report from a tablet server. If the existing
RPC task failed, catalog manager would create it the next time the
tablet server heartbeats.
This patch also fix HandleResponse() in AsyncDeleteReplica to avoid
unnecessary retries.
Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
5 files changed, 198 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/14849/6
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 6
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (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/14849
to look at the new patch set (#3).
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
KUDU-2992: Avoid sending duplicated requests in catalog_manager
When the master sent a large number of rpc requests to a tserver,
and the tserver didn't response immediately, catalog manager may
continue to send duplicate requests in ProcessTabletReport(), that
would cause rpc timeouts and high load on both masters and tservers.
This patch add a tablet_id index to pending_tasks, catalog manager
would not send duplicate requests to a tablet with the same alter type
when handling a tablet report from a tablet server. If the existing
RPC task failed, catalog manager would create it the next time the
tablet server heartbeats.
This patch also fix HandleResponse() in AsyncDeleteReplica to avoid
unnecessary retries.
Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
4 files changed, 68 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/14849/3
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (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/14849
to look at the new patch set (#2).
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
KUDU-2992: Avoid sending duplicated requests in catalog_manager
When the master sent a large number of rpc requests to a tserver,
and the tserver didn't response immediately, catalog manager may
continue to send duplicate requests in ProcessTabletReport(), that
would cause rpc timeouts and high load on both masters and tservers.
This patch add a tablet_id index to pending_tasks, catalog manager
would not send duplicate requests to a tablet with the same alter type
when handling a tablet report from a tablet server. If the exsiting
RPC task failed, catalog manager would create it the next time the
tablet server heartbeats.
This patch also fix HandleResponse() in AsyncDeleteReplica to avoid
unnecessary retries.
Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
4 files changed, 62 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/14849/2
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14849/3/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/14849/3/src/kudu/master/catalog_manager.h@373
PS3, Line 373: std::unordered_multimap<std::string, scoped_refptr<MonitoredTask>> pending_tasks_;
> You can do >> in C++11 (previous versions of C++ required a space between t
Done
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14849/2/src/kudu/master/catalog_manager.cc@5531
PS2, Line 5531: void TableInfo::WaitTasksCompletion() {
: int wait_time = 5;
: while (1) {
> Isn't this still a valid concern? That is, if the last ref to the TableInfo
It seems impossible that the last ref to the TableInfo is in 'task', at least catalog manager contains a ref count.
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Wed, 11 Dec 2019 13:49:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
KUDU-2992: Avoid sending duplicated requests in catalog_manager
When the master sent a large number of rpc requests to a tserver,
and the tserver didn't response immediately, catalog manager may
continue to send duplicate requests in ProcessTabletReport(), that
would cause rpc timeouts and high load on both masters and tservers.
This patch add a tablet_id index to pending_tasks, catalog manager
would not send duplicate requests to a tablet with the same alter type
when handling a tablet report from a tablet server. If the existing
RPC task failed, catalog manager would create it the next time the
tablet server heartbeats.
This patch also fix HandleResponse() in AsyncDeleteReplica to avoid
unnecessary retries.
Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Reviewed-on: http://gerrit.cloudera.org:8080/14849
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
4 files changed, 197 insertions(+), 57 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 7
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] KUDU-2992: Avoid sending duplicated requests in catalog manager
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14849 )
Change subject: KUDU-2992: Avoid sending duplicated requests in catalog_manager
......................................................................
Patch Set 2:
> Patch Set 1:
>
> (4 comments)
>
> I'm a little nervous about using the description as a unique key. They look immutable and unique, but they're also just supposed to be descriptive.
>
> Could you add a test showing that this works the way you expect?
Yes, using the description as a unique key is not suitable, since the description includes table_name in AlterTableRPC, and the table_name would be not consistent before and after the RPC call. I did some modifications and now we only handle duplicate requests in ProcessTabletReport.
--
To view, visit http://gerrit.cloudera.org:8080/14849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If090723821bd78e14a3c54a35cb5e471320002e9
Gerrit-Change-Number: 14849
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 13:24:44 +0000
Gerrit-HasComments: No