accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Drob <md...@mdrob.com>
Subject Re: ACCUMULO-958 - Pluggable encryption in walogs
Date Wed, 30 Jan 2013 22:42:07 GMT
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, <dlmarion@comcast.net> 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" <keith@deenlo.com>
> To: dev@accumulo.apache.org
> 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 <afuchs@apache.org> 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 <eric.newton@gmail.com>
> 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 <josh.elser@gmail.com>
> 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>
> >> >
> >>
>

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