It might be preferable if the patch included an example plugin. Otherwise, it's a bit hard to see how an open process can evaluate the design decisions, perhaps test a bit of performance, etc. I write this without having read a line of code. If this is, in fact, very simple and straightforward, then it might be a tempest in a teapot.
On Wed, Jan 30, 2013 at 2:26 PM, Keith Turner <[email protected]> wrote: > On Wed, Jan 30, 2013 at 2:02 PM, William Slacum > <[email protected]> wrote: >> Adam, I think you need to invert your premise. You need to consider the >> benefit to the community of some piece of work before committing back to >> the community. A plug-in framework has no value if there are no plug-ins. >> Adding in untested, unreviewed layers of indirection for reading and > > We are CTR. Any committer should be willing to open discussion and > review of any change they make. I feel that very reasonable questions > about this change are not being answered or discussed. > >> writing data is a bad idea, plain and simple. Furthermore, you cannot say >> you are avoiding forking by supplying the patch and then openly state you >> are witholding portions that make said patch useful. >> >> On Wed, Jan 30, 2013 at 1:45 PM, Adam Fuchs <[email protected]> wrote: >> >>> Bill, >>> >>> The release date was not pushed back just for this ticket -- there were >>> several other changes that motivated that date change. We can discuss that >>> aspect separately from a discussion of ACCUMULO-958, and we need to start a >>> separate thread to talk about the remaining milestones before the 1.5.0 >>> release. >>> >>> I would also like to amend your statement to be "... the patch has no value >>> added [for] general users [without the addition of extensions that are not >>> included with the patch]." This is a more accurate yet much weaker premise, >>> and you should consider the implications on the broader ecosystem. >>> >>> It seems to me that the main points against this patch are that it is >>> imperfect. I don't think that feature freeze is the time at which we should >>> demand perfection. Several valid issues have been raised, which should be >>> fixed by code freeze (the date of which is not yet set). However, the >>> utility of this work is obvious to me. At the end of the day, what bar are >>> we trying to set for inclusion of a patch? >>> >>> Adam >>> >>> >>> >>> On Wed, Jan 30, 2013 at 11:05 AM, William Slacum < >>> [email protected]> wrote: >>> >>> > Bottom line, the patch has no value added to general users. The idea >>> behind >>> > pushing back a release date to stuff in unoperational code is very bad >>> > practice. It sets a precedent for not considering alternative approaches >>> > while simultaneously having no justification for choosing the approach we >>> > did. If a specific customer/group/person wants a feature, and that >>> feature >>> > does not exist yet, the code is freely available to be modified, >>> > distributed and open to public review. Adam, I strongly disagree that >>> > forking the code is bad, considering the progress that other projects >>> make >>> > specifically because they have experimental forks (HBase). >>> > >>> > On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <[email protected]> wrote: >>> > >>> > > Let me attempt to make another argument for why the 958 patch should be >>> > > included in 1.5.0. What this patch represents is not an encryption >>> > solution >>> > > for WAL, but an experimental extension point that will be used for >>> > building >>> > > an encryption solution as a pluggable module. We need to judge its >>> merit >>> > > based on whether it is a successful experimental extension point or >>> not. >>> > > There are three main reasons for including the patch in 1.5.0: >>> > > 1. Test the performance impact of the null cipher solution (default >>> > > configuration) in all the performance tests we will be running for the >>> > > 1.5.0 release. If it causes problems there then we can roll it back. >>> > > 2. Enable the use of this extension after 1.5 is released. External >>> > > experiments have dependencies on this extension point. Without the >>> > > extension point we will have to test with unreleased versions of >>> > Accumulo, >>> > > which would be less than ideal. >>> > > 3. It is not harmful and somebody wants it. The reason for wanting this >>> > > code in is well documented, so you need a very strong reason to throw >>> it >>> > > out. Otherwise you will encourage forking of the project (which would >>> be >>> > > bad). >>> > > >>> > > Adam >>> > > >>> > > >>> > > >>> > > >>> > > On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <[email protected]> >>> > > wrote: >>> > > >>> > > > Some comments about the comments in ACCUMULO-958: >>> > > > >>> > > > Josh writes: >>> > > > >>> > > > "We still have the ability to review this even after the feature >>> freeze >>> > > > happens, it's just frustrating from my point of view in generating >>> the >>> > > best >>> > > > 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty >>> > > darn >>> > > > quick)." >>> > > > >>> > > > John writes: >>> > > > >>> > > > "Yes, but we get stuck on x.y.* for a year or so, so it does become a >>> > > race >>> > > > to get all the features you want to see in the next year." >>> > > > >>> > > > As Accumulo matures, we will need to start thinking a little more >>> > > flexibly >>> > > > about what goes into minor releases. We have implemented new (small) >>> > > > features in minor releases before. >>> > > > >>> > > > I would have no problem including ACCUMULO-958 into 1.5.1 after a >>> test >>> > > > phase, and after some basic experience with the feature. However I'm >>> > > very >>> > > > uncomfortable including this in 1.5.0 because there is not a single >>> > test, >>> > > > and no real implementation behind the factory that anyone would use >>> In >>> > > Real >>> > > > Life. Is this an appropriate API? I have no idea. Comments in the >>> > code >>> > > > about the stability of the interface basically admit that the author >>> > > isn't >>> > > > completely comfortable with it, either. >>> > > > >>> > > > Let's not rush it, and when it is done right, I'm all for putting it >>> > into >>> > > > the next release. For now, I would hold back incorporating these >>> > changes >>> > > > until they are more fully implemented. After we branch 1.5, commit >>> this >>> > > to >>> > > > trunk, and back-port it to the 1.5 branch when experience and tests >>> > show >>> > > it >>> > > > is ready to be released. >>> > > > >>> > > > -Eric >>> > > > >>> > > > >>> > > > >>> > > > On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <[email protected]> >>> > > wrote: >>> > > > >>> > > > > All, >>> > > > > >>> > > > > It's been a few days and I haven't seen much chatter at all on >>> > > > > ACCUMULO-958 [1] since the patch was applied. There are a couple of >>> > > > > concerns I have that I definitely want to see addressed before a >>> > 1.5.0 >>> > > > > release. >>> > > > > >>> > > > > - It worries me that the provided patch is fail-open (when we can't >>> > > load >>> > > > > the configured encryption strategies/modules, we don't decrypt >>> > > anything. >>> > > > I >>> > > > > think for a security-minded database, we should probably be >>> > defaulting >>> > > to >>> > > > > fail-close; but, that brings up an issue, what happens when we >>> can't >>> > > > > encrypt a WAL? Do minor compactions fail gracefully? What does >>> > Accumulo >>> > > > do? >>> > > > > >>> > > > > - John said he had been reviewing the patch before he applied it; >>> it >>> > > > > bothers me that there was a version of this patch that had been >>> > > reviewed >>> > > > > privately for some amount of time when we had already pushed back >>> the >>> > > > > feature freeze date by a week waiting for features that weren't >>> done. >>> > > > > >>> > > > > - The author noted himself with the deprecation of the CryptoModule >>> > > > > interface that "we anticipate changing [this] in non-backwards >>> > > compatible >>> > > > > ways as we explore requirements for encryption in Accumulo...". >>> This >>> > > > tells >>> > > > > me that implementation of WAL encryption overall hasn't been >>> properly >>> > > > > thought out. >>> > > > > >>> > > > > Given all of this, it gives me great pause to knowingly include >>> this >>> > > > patch >>> > > > > into a 1.5.0 release. I see no signs that this has been truly >>> thought >>> > > > out, >>> > > > > there is no default provided encryption strategy for 1.5.0 with >>> this >>> > > > patch >>> > > > > for the WAL and there is still no support at all for RFile >>> encryption >>> > > (no >>> > > > > end-to-end Accumulo encryption for a user). All of these issues >>> > > > considered >>> > > > > make me believe that this is an incomplete feature that is not >>> ready >>> > > for >>> > > > an >>> > > > > Apache Accumulo release. >>> > > > > >>> > > > > Thoughts? >>> > > > > >>> > > > > - Josh >>> > > > > >>> > > > > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958< >>> > > > https://issues.apache.org/jira/browse/ACCUMULO-958> >>> > > > > >>> > > > >>> > > >>> > >>>
