Given the discussion that's taken place today on this thread, I'd like
to bring back around the discussion and refocus my concerns, given
everyone else's comments, and perhaps generate some sort of summary,
boiling down to "what next".
First, my own opinions, despite how my actions may/may not have been
interpreted by others on this list, my reason for reviewing 958, and
furthermore starting this thread, is purely from a QA standpoint. I
would have not hesitated to start a similar discussion if the
contributor was a more-seasoned Accumulo dev than I (of which I consider
most to be), a coworker, former coworker, friend, stranger. I want to
strongly bring focus back to the point: I reviewed a patch from a new
contributor, I saw issues in the patch, gave (in my opinion) positive
feedback to the contributor and received no further comments after what
I consider a fair amount of time (5 days). My overarching reason for
escalating this issue is that I am concerned that the longer we wait to
*actually* finalize the first 1.5.0 release candidate, the less we will
actually test.
In less words, I saw *code* (not an individual) which I had issues with,
received no response to code review after a very quick patch
application, and wanted to test the waters to see if others also had
complaints.
(putting on my PMC hat to try to be unbiased, but *please* read for the
thread for yourself)
Trying to summarize what has been discussed, there is concern from
developers with the changes in regards to public API changes, lack of
unit/functional tests, the lack of an actual encryption policy (one that
encrypts) in the provided changes and the lack of a complete encryption
solution. On the other side, issues were raised about stifling
contributions due to a high barrier to entry to Accumulo which could
cause project division and the provided code is disabled by default
which shouldn't cause any harm to users/testers.
(putting my hat back on)
To voice my final opinion, I have *no* problem with including code for
encrypting WALs if someone will publicly sign up to bring the quality of
that code up to what I consider Accumulo-par. Meaning, someone signs up
to actively participate in review with the devs and adhere to future
deadlines of completion (so as to not delay a 1.5.0 release). I also
expect the creation of some basic functional tests to give "us" some
warm-fuzzies that the code doesn't create any performance issues. I
would *like* to see an implementation of encryption using these changes
(instead of just the promise), but I can understand if this is not
feasible given the time constraints of a 1.5.0 release (but please
explicitly tell the rest of us this if it is infeasible). I'm personally
not super-concerned about new config values being added across a major
release (public API).
Outside of the scope of just this issue, I'm going to 1) start thinking
about ways that we can quantify/automate the issue of QA contributions
to alleviate any future issues stemming from similar situations. 2) the
next time we have a feature-freeze, we need to be as explicit as
possible to avoid conflict (how can we make this process better).
- Josh
On 01/30/2013 03:13 PM, Benson Margulies wrote:
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 <[email protected]> wrote:
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>
