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