Return-Path: X-Original-To: apmail-accumulo-dev-archive@www.apache.org Delivered-To: apmail-accumulo-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id AE65D17FDE for ; Mon, 27 Oct 2014 20:29:32 +0000 (UTC) Received: (qmail 82915 invoked by uid 500); 27 Oct 2014 20:29:32 -0000 Delivered-To: apmail-accumulo-dev-archive@accumulo.apache.org Received: (qmail 82868 invoked by uid 500); 27 Oct 2014 20:29:32 -0000 Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list dev@accumulo.apache.org Received: (qmail 82847 invoked by uid 99); 27 Oct 2014 20:29:31 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 27 Oct 2014 20:29:31 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 401641DF793; Mon, 27 Oct 2014 20:29:36 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============9026987051965448929==" MIME-Version: 1.0 Subject: Re: Review Request 27198: ACCUMULO-3236 introducing cloneInto feature From: "Josh Elser" To: "John Vines" , "accumulo" , "Josh Elser" Date: Mon, 27 Oct 2014 20:29:36 -0000 Message-ID: <20141027202936.7137.71664@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Josh Elser" X-ReviewGroup: accumulo X-ReviewRequest-URL: https://reviews.apache.org/r/27198/ X-Sender: "Josh Elser" References: <20141025193155.9508.71027@reviews.apache.org> In-Reply-To: <20141025193155.9508.71027@reviews.apache.org> Reply-To: "Josh Elser" X-ReviewRequest-Repository: accumulo --===============9026987051965448929== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27198/#review58675 ----------------------------------------------------------- Reviewed what I can; reviewboard seems to have eaten some of the diffs (notably Tablet.java). core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 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. core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java Boolean.toString(boolean) instead of creating a new String for the cast, please. core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperationsImpl.java nit: whitespace core/src/main/thrift/tabletserver.thrift 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. server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java 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? server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java This could be incorrect. The underlying exception might be for srcTableId or destTableId. I think you'd need to split up the security checks by hand or make sure that canCloneIntoTable method is throwing you an exception that you know how to unwrap and give back to the client. server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java Would be nice to lift the inner classes out of CloneIntoTable into their own classes inside of the this file. That way we could test the individual steps without having to have an instance of CloneIntoTable when we don't want it. Makes it a bit more consistent with the other fate ops too (e.g. CreateTable) server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java Use MetadataSchema.TabletsSection.getRange(String) instead. server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneIntoTable.java Won't this fail if you try cloneInto with an empty table as the source? test/src/test/java/org/apache/accumulo/test/functional/CloneIntoIT.java Some more tests here that enumerate the basic edge cases would be good: srcTable missing, destTable missing, read perms denied on source, write perms denied on dest. Breaking up testCloneInto into a few test cases instead of one big one would be much easier when trying to debug things later. test/src/test/java/org/apache/accumulo/test/functional/CloneIntoIT.java Thank you for adding this :) - Josh Elser 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 > > --===============9026987051965448929==--