Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============4743874958020342639=="
MIME-Version: 1.0
Subject: Re: Review Request 20636: ACCUMULO-2726 restore binary compatibility
 1.5 ->
 1.6
From: "Sean Busbey" <sean@manvsbeard.com>
To: "Christopher Tubbs" <ctubbsii@apache.org>,
 "Sean Busbey" <sean@manvsbeard.com>, "accumulo" <dev@accumulo.apache.org>
Date: Fri, 25 Apr 2014 07:10:35 -0000
Message-ID: <20140425071035.25434.78673@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Sean Busbey" <noreply@reviews.apache.org>
References: <20140424211803.2045.88421@reviews.apache.org>
In-Reply-To: <20140424211803.2045.88421@reviews.apache.org>
Reply-To: "Sean Busbey" <sean@manvsbeard.com>

--===============4743874958020342639==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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


--===============4743874958020342639==--