I don't have any concerns with releasing features in 1.5.1, but please be cognizant that the intent of ACCUMULO-751<https://issues.apache.org/jira/browse/ACCUMULO-751>was for wire compatibility across the 1.5.X series.
I'll strongly agree with Keith regarding a stable API - confusing and constantly changing APIs are the most frustrating aspect of working with certain projects (mapred/mapreduce, anyone?) and I think that's a path to avoid. Mike On Wed, Jan 30, 2013 at 12:49 PM, <[email protected]> wrote: > > > > > - Josh has brought up some technical concerns with the patch > > > > - Eric said that he would be fine adding it in 1.5.1 after testing > > > > - Keith suggests that we don't change the public API for this if it is > likely to change > > > > +1 to all of the above. I think it's ok to add portions of a feature as > they mature, but it doesn't make sense to add something that we know is > going to drastically change later. Feature branches are great for > experiments. Regardless of whether somebody wants it, if we deliver it now, > then they will use and be dependent on a new, non-complete, deprecated > feature. > > > > Dave > > > > ----- Original Message ----- > > > From: "Keith Turner" <[email protected]> > To: [email protected] > Sent: Wednesday, January 30, 2013 11:10:38 AM > Subject: Re: ACCUMULO-958 - Pluggable encryption in walogs > > 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 > > I do experiments all of the time w/o including half done things in a > release. > > Should I include the following in 1.5.0 just so I can experiment with > it? I was working on it got sidetracked and never got back to it. At > this point I am uncertain of its utility. It needs further > experimentation. > > https://github.com/keith-turner/accumulo/tree/ACCUMULO-551 > > > 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. > > Back to ACCUMULO-551 I did experiements with that with not problem w/o > including it in a release. I just created a version of accumulo > called accumulo-551-snapshot so no one would be confused if they > encountered it. What is wrong with the approach? > > > 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). > > Forking over this seems illogical. > > If we leave it in and hide it, then should all of the configuration > properties be removed? > > I would consider the config props to be part of the public API and not > easily modified in the future. Since the props may change as the full > implementation evolves, I think it would make sense to remove them > from the public API. If left, we should modify the config to support > marking the config props as experimental and also modify the code that > generates config documentation. I just want to avoid boxing ourselves > in or having to make things confusing for users. > > > > > 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> > >> > > >> >
