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 Wed, 29 Oct 2014 20:51:42 GMT


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


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