Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9393 )

Change subject: KUDU-2290: Tool to re-create a tablet
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/CMakeLists.txt@101
PS7, Line 101: ADD_KUDU_TEST(replace_tablet-itest)
> B-but the test always fails and is disabled, because of KUDU-2376. Can I ad
Sorry, I missed that the disabled test was the only test in the file.

Add the TODO near the test itself so it'll be more obvious when it is reenabled.


http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/master/catalog_manager.cc@4376
PS7, Line 4376:   LOG(INFO) << "ReplaceTablet: tablet " << old_tablet->id()
              :             << " deleted and replaced by tablet " << 
new_tablet->id();
              :   resp->set_replaceme
> Does the following order work?
1. l_table doesn't actually need to be in WRITE mode; READ is sufficient. That 
should simplify this a bit by eliminating step #4.
2. We should be able to follow what ProcessPendingAssignments does, right? It 
also replaces tablets. Looks like it first commits the tablets lock, then 
adds/removes the tablets to the table, then finally updates the tablet map.

In general I'm not sure there's a deadlock free way to do this without exposing 
a window where GetTableLocations and  GetTabletLocations aren't totally 
consistent with one another. From the looks of it, ProcessPendingAssignments is 
vulnerable to the window you're describing.  Maybe it's able to get away with 
it because the table is expected to be generally unusable during this time 
(it's just after a CreateTable after all). Though, maybe the same code path is 
followed during an ALTER TABLE ADD PARTITION too?



--
To view, visit http://gerrit.cloudera.org:8080/9393
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbefbde68e3ca724f04efe0426a3906e5c33ed3c
Gerrit-Change-Number: 9393
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wdberke...@gmail.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Tue, 03 Apr 2018 04:09:11 +0000
Gerrit-HasComments: Yes

Reply via email to