accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubb...@apache.org>
Subject Re: Review Request 21043: ACCUMULO-1691 Update Thrift to 0.9.1
Date Sun, 11 May 2014 20:12:51 GMT


> On May 9, 2014, 5:23 p.m., Sean Busbey wrote:
> > server/base/src/main/java/org/apache/accumulo/server/util/CustomNonBlockingServer.java,
lines 36-38
> > <https://reviews.apache.org/r/21043/diff/1/?file=573991#file573991line36>
> >
> >     nit: whitespace
> 
> Christopher Tubbs wrote:
>     This is introduced by a known issue with the Eclipse formatter (https://bugs.eclipse.org/bugs/show_bug.cgi?id=270745).
I'm not too concerned.
> 
> Sean Busbey wrote:
>     Provided you can remove it manually, please do so.

You're asking me to diverge away from the implementation of an established formatting standard
for the project. While I agree these trailing whitespace should not be there, I'd rather not
spend time making manual modifications to formatting that we've agreed on as a standard. I
prefer them gone as well, but it's not worth the time and effort to make manual modifications.
If it's a blocker for you, I'll remove it (I assume it's not, since you described it as a
"nit"), otherwise, I'd prefer to spend my time elsewhere.

I've accepted patches with these removed and with them present. I ask that you be tolerant
to both as well, until there is a formatting standard that we've agreed on that fixes this
issue once and for all, and focus the review effort on more important bits. Thanks.


> On May 9, 2014, 5:23 p.m., Sean Busbey wrote:
> > server/base/src/main/java/org/apache/accumulo/server/util/CustomNonBlockingServer.java,
lines 38-41
> > <https://reviews.apache.org/r/21043/diff/1/?file=573991#file573991line38>
> >
> >     is there an upstream ticket in Thrift that would allow us to skip having our
own implementation?
> 
> Christopher Tubbs wrote:
>     Yes, see the THRIFT issue linked in the JIRA, and the additional issues linked from
there. This issue is a regression that has occurred at least twice in THRIFT, and other projects
(Cassandra, for example) have also had to reimplement these classes as a workaround. The reported
THRIFT issue would essentially fix it permanently.
>     
>     However, even if that fix is addressed in a future version of Thrift, it will not
help us today, and since Thrift 0.9.1 is the latest release, we need this patch today, to
integrate with downstream packaging which use Thrift 0.9.1.
> 
> Sean Busbey wrote:
>     please either
>     
>     1) link to the THIRFT jira in the javadoc
>     
>     or
>     
>     2) change the kind of relationship on ACCUMULO-1691. "Depends on" reads to me like
we won'd do the upgrade until the upstream ticket is fixed. I'd be fine with that, but it
would presumably obviate this class. "broken by" maybe?

Sure, I can certainly do both of those things. In any case, I need to update this javadoc
regardless, because it describes replacing only the AsyncFrameBuffer, and I replace both.


- Christopher


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


On May 2, 2014, 7:15 p.m., Christopher Tubbs wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21043/
> -----------------------------------------------------------
> 
> (Updated May 2, 2014, 7:15 p.m.)
> 
> 
> Review request for accumulo, Sean Busbey and kturner.
> 
> 
> Bugs: ACCUMULO-1691
>     https://issues.apache.org/jira/browse/ACCUMULO-1691
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> Updates Thrift dependency to 0.9.1 with a hack to access a needed protected field.
> 
> 
> Diffs
> -----
> 
>   pom.xml 43aa5fb 
>   server/base/src/main/java/org/apache/accumulo/server/util/CustomNonBlockingServer.java
PRE-CREATION 
>   server/base/src/main/java/org/apache/accumulo/server/util/TServerUtils.java 6d9e4c7

> 
> Diff: https://reviews.apache.org/r/21043/diff/
> 
> 
> Testing
> -------
> 
> mvn clean verify -Psunny
> 
> 
> Thanks,
> 
> Christopher Tubbs
> 
>


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