accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <ctubb...@apache.org>
Subject Re: [VOTE] ACCUMULO-3176
Date Tue, 25 Nov 2014 20:23:15 GMT
On Tue, Nov 25, 2014 at 2:57 PM, Sean Busbey <busbey@cloudera.com> wrote:

> > altering the public API (as a standalone change)
>
> I am very conservative about changes to our public API. I have had to write
> code that works across Accumulo versions and even when only working with
> the public API is it painful on almost every release. I have also seen
> plenty of cases where deployments of Accumulo lag behind version changes
> because of this same issue.
>
>
This seems to be a new objection, and independent of your previous veto. Is
that the case?

Would you be more comfortable accepting the changes if they simply
introduced the new API, and did not deprecate the old, multiple overloaded
create table methods?


> I fully support that major version changes are there for us to break APIs.
> That doesn't mean that such breakage doesn't have a high cost that needs to
> be weighed carefully.
>
>
Note: no breakage is included in this patch. This patch retains
backwards-compatibility.


> In general, for me to be positive on an API change there needs to be a
> compelling improvement or a correctness issue. For me, that correctness
> issue is the race condition on property updates you mention.
>
>
The use cases described are sufficiently compelling for me. The issue you
are concerned about for race condition on updates would be a compelling
change also, which would be worth consideration if somebody were to provide
a patch to address that. This is not that patch.


> While I acknowledge that a per-table configured load balancer can't be set
> prior to the initial tablet assignment without this change, I don't think
> that out weighs having to update all table creation code. (this will be the
> case even if we go through a deprecation cycle. the cycle just draws it
> out.) Granted, I have never had cause to use a per-table load balancer that
> was hampered by where the initial tablet was assigned. So I might just not
> know the level of pain that fix would address.
>
>
Again, this seems to be an argument for not removing (or not deprecating)
the existing methods for table creation, not an argument against this
patch's additions.


> >  implementing a partial fix for the larger problem
>
> This fix leaves those who need a consistent view of some set of properties
> with only options that involve table creation, either in this changed API
> or via Keith's clone table work around on an offline table. That means if
> you need to alter a constraint or an iterator (that needs to be used
> universally), you have the same negative consequence as some of the
> proposed solutions while also having to change the underlying table id.
> More
> importantly, I think it leaves people prone to not realizing that their
> change to these things won't be uniformly enforced during
> the propagation period.
>
> We could mitigate some of the gotchas around when property updates need to
> be strongly consistent with better docs, but that only helps when people
> are at the point of reading docs. I'd rather stop them from self inflicted
> pain directly because people often don't have the time to read docs. I see
> stopping this correctness problem as worth the behavior change something
> like "only update properties when tables are offline" would necessitate.
> But
> this vote isn't about the merits of that particular alternative.
>
> I'm willing to consider other options for solving the consistent property
> issue and have tried to work through them in the comments around
> ACCUMULO-3176. I just don't believe the one meets my standard for
> inflicting API pain.
>
>
Again, this issue you bring up is out of scope for the patch/JIRA. I don't
think it is reasonable to block this change because it doesn't satisfy that
(completely valid) issue, which we can still solve in the future.


> On Tue, Nov 25, 2014 at 1:16 PM, Josh Elser <josh.elser@gmail.com> wrote:
>
> > Can I pick your brain some more? (also, I'm sorry if this is already
> > addressed in the JIRA comments somewhere. I'm being lazy)
> >
> > As we know, there is a larger problem of ensuring consistent
> configuration
> > across all servers in the cluster. I don't think there is any contention
> > here. There are ways we can do this, but no one has done it yet. That's
> the
> > general problem, and, I believe, what you mean by the "underlying issue".
> >
> > A subset of that problem is configuration updates for a table which is
> > newly created. In my experience, this is often how I get bitten by this
> > race condition -- I create a table, I updated some property (typically
> set
> > an iterator/combiner/constraint), and then did some insert/scan before
> the
> > tabletserver I communicated with got my property update.
> >
> > I see this taking what is a technically difficult problem (assumption,
> > since no one has done it yet) and making the problem partially better. In
> > practice for me, this also means that how I often encountered this race
> > condition is addressed.
> >
> > I also don't see the changes that Jenna wrote as a blocker to
> implementing
> > a "proper" blocking configuration update.
> >
> > Can you clarify your level of concern with this change in: altering the
> > public API (as a standalone change), implementing a partial fix for the
> > larger problem, and the combination of the previous two points? It would
> be
> > much appreciated.
> >
> >
> >
> > Sean Busbey wrote:
> >
> >> -1
> >>
> >> This change alters our public API while not solving the underlying issue
> >> of
> >> race conditions in property updates.
> >>
> >> On Tue, Nov 25, 2014 at 11:14 AM, Christopher<ctubbsii@apache.org>
> >> wrote:
> >>
> >>  Committers, this is a consensus vote on whether or not to include
> Jenna's
> >>> patch for ACCUMULO-3176 to the 1.7.0-SNAPSHOT (master) branch.
> >>>
> >>> This patch improves the table creation API with the introduction of a
> >>> NewTableConfiguration object (similar to the pattern for
> >>> BatchWriterConfig), which allows us to be flexible on improving table
> >>> creation options in the future without creating many overloaded methods
> >>> (as
> >>> the table creation API has been plagued by in the past). The main goal
> of
> >>> the patch is to allow table properties to be set on a table at the time
> >>> of
> >>> creation, before any tablets are assigned, but it also lays the
> >>> foundation
> >>> for future table creation improvements. Creating initial table
> properties
> >>> was already present in the RPC calls, but not exposed in the API. This
> >>> can
> >>> support a number of use cases.
> >>>
> >>> Since an objection was raised by Sean Busbey (and a veto expected),
> I've
> >>> initiated this vote in lieu of applying the patch under lazy consensus
> so
> >>> that any veto votes can be justified and addressed here.
> >>>
> >>> Note: there are a few bugs in the Mock implementation of this that I've
> >>> fixed, as well as some minor deprecation warnings and javadoc
> >>> improvements
> >>> I'm adding, please apply your vote under the assumption that those will
> >>> be
> >>> fixed before it will be applied.
> >>>
> >>> Please vote in accordance with the bylaws for consensus voting.
> >>> My vote is +1.
> >>>
> >>> Thanks.
> >>>
> >>> --
> >>> Christopher L Tubbs II
> >>> http://gravatar.com/ctubbsii
> >>>
> >>>
> >>
> >>
> >>
>
>
> --
> Sean
>

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