> On Oct. 27, 2014, 8:29 p.m., Josh Elser wrote: > > core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java, > > line 346 > > <https://reviews.apache.org/r/27198/diff/1/?file=733392#file733392line346> > > > > Maybe it would be clearer to actually say that no data is actually > > copied but the existing files will be referenced by the target table. It > > means the same thing that you said, but is a bit more straightforward IMO.
Changed > On Oct. 27, 2014, 8:29 p.m., Josh Elser wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java, > > line 724 > > <https://reviews.apache.org/r/27198/diff/1/?file=733394#file733394line724> > > > > Boolean.toString(boolean) instead of creating a new String for the > > cast, please. Went ahead and fixed that in importDirectory too (where I got that code from) > On Oct. 27, 2014, 8:29 p.m., Josh Elser wrote: > > core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperationsImpl.java, > > line 452 > > <https://reviews.apache.org/r/27198/diff/1/?file=733399#file733399line452> > > > > nit: whitespace Reformatted file > 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. > > John Vines wrote: > 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. > > Josh Elser wrote: > You hit exactly the reason I'm getting hung up on it. I'd like to start > focusing on wire compatibility more. While we don't have hard/fast rules yet, > I'd like to avoid making such changes only when there is no other option. It > would be a shame if this was the only change that kept a 1.6 master from > talking to a 1.7 tserver. Obviously, I doubt this is actually true, but I'd > rather not find out the hard way. > > Sean Busbey wrote: > Please do not break wire compatibility. I'd like to start testing rolling > upgrades and doing so will preclude it. Added back removed api, marked as deprecated so we can phase it out in a release we're comfortable breaking wire compatibility. > 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. > > John Vines wrote: > 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. Added a test > 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? > > John Vines wrote: > I precluded it because that's what clone table was doing. We can see > about opening up to that though. Can't clone the root files because they're managed by them existing, which means you can't link to them. - 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 > >
