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