accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Keith Turner <ke...@deenlo.com>
Subject Re: ACCUMULO-958 - Pluggable encryption in walogs
Date Wed, 30 Jan 2013 19:26:01 GMT
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