accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Busbey" <s...@manvsbeard.com>
Subject Re: Review Request 20636: ACCUMULO-2726 restore binary compatibility 1.5 -> 1.6
Date Fri, 25 Apr 2014 07:10:35 GMT


> On April 24, 2014, 9:18 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/security/Credentials.java, lines 94-100
> > <https://reviews.apache.org/r/20636/diff/4/?file=568016#file568016line94>
> >
> >     I'm not a fan of baking in more thrift/RPC stuff into the rest of the code.
> >     
> >     This isn't serious, because Credentials is not public API (yet), but I'm opposed
to it. I'd rather just inline these than modify Credentials.
> 
> Sean Busbey wrote:
>     are you -1 or -0?
> 
> Christopher Tubbs wrote:
>     -1, especially because the other issue, with the use Credentials version. It makes
no sense to switch from TCredentials to Credentials if Credentials just has TCredentials in
its API.
> 
> Sean Busbey wrote:
>     I've fixed the other issue with the reference to "use the Credentials version". It
no longer says that.
>     
>     Fixing the coupling of Credentials to TCredentials will touch 60+ files. Could we
do it in a follow on?
> 
> Christopher Tubbs wrote:
>     How would inlining this brand new method, affect 60+ classes?
> 
> Sean Busbey wrote:
>     I am -1 to inlining. We should have paired serialization methods. toThrift will have
to go somewhere that fromThrift can also go. ATM I have a partial implementation that moves
them out of Credentials to a helper class.
> 
> Christopher Tubbs wrote:
>     Actually, nevermind... I see now. The toThrift method already exists. That's what
you mean. I wasn't thinking that much... I just didn't want to add any *new* couplings here.
Decoupling it entirely (Dropping toThrift) is something that I plan to address in the future.
> 
> Christopher Tubbs wrote:
>     I concede. I'll change my objection to -0. This new method's introduction, it's utilization,
and its new tests could easily have been done in a follow-on issue, as I consider it out of
scope for the API issue, but I will not block the RC4 with my objection.

ACCUMULO-2733 consolidates manual deserializations into a fromThrift call per discussion on
IRC. If that ticket gets +1ed before this one does, I'll refactor this patch to be based on
that one.


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20636/#review41382
-----------------------------------------------------------


On April 24, 2014, 8:54 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20636/
> -----------------------------------------------------------
> 
> (Updated April 24, 2014, 8:54 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-2726
>     https://issues.apache.org/jira/browse/ACCUMULO-2726
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> restores things found missing by japi compliance checker
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/admin/ActiveCompaction.java 9c39ea6

>   core/src/main/java/org/apache/accumulo/core/client/admin/ActiveScan.java 30e47af 
>   core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperationsImpl.java
f80eee5 
>   core/src/main/java/org/apache/accumulo/core/client/admin/NamespaceOperationsHelper.java
b9a7791 
>   core/src/main/java/org/apache/accumulo/core/client/admin/NamespaceOperationsImpl.java
569a3b6 
>   core/src/main/java/org/apache/accumulo/core/client/admin/SecurityOperationsImpl.java
9d662f4 
>   core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsHelper.java
843f572 
>   core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsImpl.java 3d69cc1

>   core/src/main/java/org/apache/accumulo/core/client/impl/ActiveCompactionImpl.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/client/impl/ActiveScanImpl.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 7d9d3ab

>   core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsHelper.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/SecurityOperationsImpl.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsHelper.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/client/mock/MockConnector.java 996198c

>   core/src/main/java/org/apache/accumulo/core/client/mock/MockInstanceOperations.java
15379af 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockInstanceOperationsImpl.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockNamespaceOperations.java
9f0594a 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockSecurityOperations.java
16a8e02 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockSecurityOperationsImpl.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockShell.java 2bc9436 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java d3b1571

>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperationsImpl.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTabletLocator.java 6bd01a9

>   core/src/main/java/org/apache/accumulo/core/security/Credentials.java 5afc6e8 
>   core/src/test/java/org/apache/accumulo/core/client/admin/TableOperationsHelperTest.java
32136a8 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java
PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/security/CredentialsTest.java 4f8079e 
>   server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java
c2a7001 
>   server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java d63a63e

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/ImportTable.java cd59b78

>   test/compat/japi-compliance/japi-accumulo-1.5.xml 9e6f47f 
>   test/compat/japi-compliance/japi-accumulo-1.6.xml 36553b8 
> 
> Diff: https://reviews.apache.org/r/20636/diff/
> 
> 
> Testing
> -------
> 
> unit tests pass. ITs passed.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>


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