accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benson Margulies <bimargul...@gmail.com>
Subject Re: ACCUMULO-958 - Pluggable encryption in walogs
Date Wed, 30 Jan 2013 20:13:34 GMT
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 <keith@deenlo.com> wrote:
> On Wed, Jan 30, 2013 at 2:02 PM, William Slacum
> <wilhelm.von.cloud@accumulo.net> 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 <scubafuchs@gmail.com> 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 <
>>> wilhelm.von.cloud@accumulo.net> 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 <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
>>> > > 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 <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
View raw message