> 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.
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. - 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 > >
