accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Vines" <vi...@apache.org>
Subject Re: Review Request 27198: ACCUMULO-3236 introducing cloneInto feature
Date Mon, 27 Oct 2014 23:15:30 GMT


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

This isn't public api, this is only used for master->tserver.


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

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.


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

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.


> On Oct. 27, 2014, 8:29 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/CloneIntoIT.java, line 42
> > <https://reviews.apache.org/r/27198/diff/1/?file=733417#file733417line42>
> >
> >     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.

That's fine. I originally had one case and I creeped it up a bit.


> On Oct. 27, 2014, 8:29 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/CloneIntoIT.java, line 45
> > <https://reviews.apache.org/r/27198/diff/1/?file=733417#file733417line45>
> >
> >     Thank you for adding this :)

I just copied the CloneIT... or BulkIT which had it.


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message