> On Oct. 27, 2014, 8:29 p.m., Josh Elser wrote: > > core/src/main/thrift/tabletserver.thrift, line 180 > > <https://reviews.apache.org/r/27198/diff/1/?file=733405#file733405line180> > > > > I don't like removing the old bulkImport method. We should try to keep > > the server APIs as stable as we can. You can keep the old bulkImport method > > around and just call the new addFiles method in the implementation. This > > will help us stay closer to compatibility across versions. > > John Vines wrote: > This isn't public api, this is only used for master->tserver.
Ah, I didn't notice that the first time around, thanks for pointing it out. However, even though it is internal RPC, I still think that the BulkImport code shouldn't have to change to support this new feature. You can still keep the master->tserver RPC for bulk import the same and call the new `addFiles(..., false)`. This keeps the RPC methods that the server-side bulk import code calls the same while still letting you implement this new feature. > On Oct. 27, 2014, 8:29 p.m., Josh Elser wrote: > > server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java, > > line 235 > > <https://reviews.apache.org/r/27198/diff/1/?file=733412#file733412line235> > > > > We check that the source isn't the root table, but that the dest isn't > > in the system namespace. Don't we want both tables to just not be in the > > system namespace? > > John Vines wrote: > While I don't understand a use case for wanting metadata tablets to be > referenced in a standard table, I see no reason to prevent it. So, root table is only precluded as the source table because it's backed by ZK and not HDFS files? > On Oct. 27, 2014, 8:29 p.m., Josh Elser wrote: > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java, > > line 162 > > <https://reviews.apache.org/r/27198/diff/1/?file=733413#file733413line162> > > > > Won't this fail if you try cloneInto with an empty table as the source? > > John Vines wrote: > The table may be empty, but there will be the default tablet. I'll be > sure to add tests for this in the IT though to verify. Thanks, I know there will be the default tablet, but you wouldn't have a file column right away. I didn't look deep enough to figure out if that would actually be problematic. - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27198/#review58675 ----------------------------------------------------------- On Oct. 25, 2014, 7:31 p.m., John Vines wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27198/ > ----------------------------------------------------------- > > (Updated Oct. 25, 2014, 7:31 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 > >
