> 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. > > Josh Elser wrote: > 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.
You are right in that I could do a separate method for this. But I feel that further enables an issue I noticed while working on this of refactoring things while not renaming/cleaning things up to what their actual purpose is. This method was called bulkImport, but all it specifically did was assign files. It did no moving of files, etc., it just validated they were in the right path. So I changed the name to reflect it's actual behavior to better indicate what it was doing. I just added a flag to make an ingrained side-affect less ingrained. This really comes across as a case supporting copying a method, which I have fundamental issues with. Maybe I'm not really seeing the value in strictly maintaining a server-only API since we're not supporting wire compatibility yet. > 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. > > Josh Elser wrote: > So, root table is only precluded as the source table because it's backed > by ZK and not HDFS files? I precluded it because that's what clone table was doing. We can see about opening up to that though. > 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. > > Josh Elser wrote: > 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. the ke is determined solely by the existance of the prev row column, which any default tablet will have. Having no files just generates an empty map of files to import, so it's effectively a noop. - John ----------------------------------------------------------- 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 > >
