----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27198/#review59201 -----------------------------------------------------------
server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java <https://reviews.apache.org/r/27198/#comment100481> reserve table, tries to reserve the table. If it succeeds it returns 0, otherwise it returns 100. It should be called in isReady(), and isReady() should return what it returns. server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java <https://reviews.apache.org/r/27198/#comment100482> Fate ops should try to get locks in isReady(). If they can not, they should return >0 and let the fate thread try to execute another operation. Calling lock() in call, instead of tryLock() in is ready holds up this fate thread. In the worst case all fate threads are held and no progress can be made. Also deadlock can occur if someone calls cloneInto(A,B) and someone else calls cloneInto(B,A). Should lock tables in alpha order. server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java <https://reviews.apache.org/r/27198/#comment100483> Are the tables offline? If not, then the splits could change during this operation. server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java <https://reviews.apache.org/r/27198/#comment100484> This is user unfriendly. We could possibly add the needed splits to the dest table. server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java <https://reviews.apache.org/r/27198/#comment100490> AFAICT nothing is done w/ the logical time of the src table? Should take the max logical time for a src tablet and dest tablet and set that as the dest tablets logical time. Also logical times should be of same type. server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java <https://reviews.apache.org/r/27198/#comment100488> This will result in the fileToExtents map being stored in ZK, which could cause problems. Could store the data in HDFS and store a file pointer in HDFS. Fate could create and clean up the file. I have only reviewed the FATE ops so far. How will this work w/ replication? I am thinking another possible approach may be to make a clone operation that accepts multiple input tables and creates a new table. The reason I am thinking about this is that it avoids having to deal w/ issues related to the dest table changing while the clone is happening. Something like clone([tableA, tableB], tableC) However, this is stil still tricky. The existing clone handles cloning of online table that may be splitting. It makes multiple passes over the src table metadata entries updating the dest until it stabilizes. In order to avoid this for multiple tables could move from a cloneInto that supports multiple online inputs to adding a merge that supports multiple offline tables as input. ``` clone(tableA, tmpA) offline(tmpA) clone(tableB, tmpB) offline(tmpB) mergeTables([tmpA, tmpB], tmpC) ``` After this tmpA and tmpB would be deleted and tmpC would have the files and splits of both. tmpC would also have the correct logical time. However one thing I am not sure about is about per table props. When clone(tableA, tableB) is done, it will create tableB w/ tableA's per table props. - kturner On Oct. 29, 2014, 8:52 p.m., John Vines wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27198/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2014, 8:52 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-3236 > https://issues.apache.org/jira/browse/ACCUMULO-3236 > > > Repository: accumulo > > > Description > ------- > > Includes all code to support feature, including thrift changes > Includes minor code cleanup to TableLocator and items in the Bulk path to > remove signature items that are unused (arguments & exceptions) > Includes renaming of some bulk import functions to clarify their purpose > (because they're now multi-purpose) > > Patch is based on 1.6, but we can choose to make it target only 1.7 if we > choose (this conversation should be taken up on jira, not in RB) > > > Diffs > ----- > > > core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java > 97f538d > > core/src/main/java/org/apache/accumulo/core/client/impl/RootTabletLocator.java > 97d476b > > core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java > 2792bcc > core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocator.java > e396d82 > > core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocatorImpl.java > c550f15 > > core/src/main/java/org/apache/accumulo/core/client/impl/TimeoutTabletLocator.java > bcbe561 > > core/src/main/java/org/apache/accumulo/core/client/impl/thrift/TableOperation.java > 7716823 > > core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperationsImpl.java > de19137 > > core/src/main/java/org/apache/accumulo/core/client/mock/impl/MockTabletLocator.java > 35f160f > > core/src/main/java/org/apache/accumulo/core/master/thrift/FateOperation.java > f65f552 > > core/src/main/java/org/apache/accumulo/core/tabletserver/thrift/TabletClientService.java > 2ba7674 > core/src/main/thrift/client.thrift 38a8076 > core/src/main/thrift/master.thrift 38e9227 > core/src/main/thrift/tabletserver.thrift 25e0b10 > > core/src/test/java/org/apache/accumulo/core/client/admin/TableOperationsHelperTest.java > 1d91574 > > core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java > 02838ed > > server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java > 27ab078 > > server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java > ebea064 > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java > d0e6aea > > server/base/src/test/java/org/apache/accumulo/server/client/BulkImporterTest.java > 3680341 > > server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java > 5818da3 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java > PRE-CREATION > server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java > 0778f5b > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 03fe069 > > test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java > 0591b19 > test/src/test/java/org/apache/accumulo/test/functional/CloneIntoIT.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27198/diff/ > > > Testing > ------- > > Includes CloneIntoIT, which exercises all permutations of the flags. Existing > BulkIT still functions as intended for validation of no feature loss in > refactoring exiting code for multi-purposing. > > > Thanks, > > John Vines > >
