[GitHub] accumulo pull request #122: ACCUMULO-1604: added check for negate option in ...
Github user dhutchis commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/122#discussion_r69675388
--- Diff:
core/src/main/java/org/apache/accumulo/core/iterators/user/ColumnAgeOffFilter.java
---
@@ -43,6 +43,9 @@ public TTLSet(Map objectStrings) {
for (Entry entry : objectStrings.entrySet()) {
String column = entry.getKey();
String ttl = entry.getValue();
+// skip the negate option, it will cause an exception to be thrown
+if (column.equals(NEGATE))
--- End diff --
A user might want to use this filter on a column named NEGATE. Could you
add a condition `&& !TTL.equalsIgnoreCase("true") &&
!ttl.equalsIgnoreCase("false")`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
Re: [DISCUSS] Proposed binary packaging changes
I assume you mean pull request. (JIRA link is below). Will do. On Tue, Jul 5, 2016, 19:43 Sean Busbey wrote: > Sounds good. Please post a link here to the JIRA? > > > > On Tue, Jul 5, 2016 at 4:58 PM, Christopher wrote: > > On Thu, Jun 30, 2016 at 5:43 PM Christopher wrote: > > > >> Hi all, > >> > >> I'd like to bring to your attention > >> https://issues.apache.org/jira/browse/ACCUMULO-4356, for discussion. > Feel > >> free to comment here or on the issue. > >> > > > > Reading back through all the replies, I don't see a *strong* consensus, > but > > it does seem like there's some acceptance of my proposal (perhaps with > some > > reservations). > > > > It seems people are mostly "okay" with this, so long as it's pushed off > to > > the future (2.0+), and is accompanied by some automated way of > downloading > > dependency jars, and collating their LICENSE/NOTICE files. > > > > So, unless there's more discussion here, my intention is to proceed to > > create a pull request against the 2.0 branch (currently: master) which > > replaces our assembly bundling with a downloader script. That way, if > > there's any additional feedback on the specific implementation, folks > will > > be able to comment directly on that. > > > > -- > busbey >
Re: [DISCUSS] Proposed binary packaging changes
Sounds good. Please post a link here to the JIRA? On Tue, Jul 5, 2016 at 4:58 PM, Christopher wrote: > On Thu, Jun 30, 2016 at 5:43 PM Christopher wrote: > >> Hi all, >> >> I'd like to bring to your attention >> https://issues.apache.org/jira/browse/ACCUMULO-4356, for discussion. Feel >> free to comment here or on the issue. >> > > Reading back through all the replies, I don't see a *strong* consensus, but > it does seem like there's some acceptance of my proposal (perhaps with some > reservations). > > It seems people are mostly "okay" with this, so long as it's pushed off to > the future (2.0+), and is accompanied by some automated way of downloading > dependency jars, and collating their LICENSE/NOTICE files. > > So, unless there's more discussion here, my intention is to proceed to > create a pull request against the 2.0 branch (currently: master) which > replaces our assembly bundling with a downloader script. That way, if > there's any additional feedback on the specific implementation, folks will > be able to comment directly on that. -- busbey
Re: [DISCUSS] Proposed binary packaging changes
On Thu, Jun 30, 2016 at 5:43 PM Christopher wrote: > Hi all, > > I'd like to bring to your attention > https://issues.apache.org/jira/browse/ACCUMULO-4356, for discussion. Feel > free to comment here or on the issue. > Reading back through all the replies, I don't see a *strong* consensus, but it does seem like there's some acceptance of my proposal (perhaps with some reservations). It seems people are mostly "okay" with this, so long as it's pushed off to the future (2.0+), and is accompanied by some automated way of downloading dependency jars, and collating their LICENSE/NOTICE files. So, unless there's more discussion here, my intention is to proceed to create a pull request against the 2.0 branch (currently: master) which replaces our assembly bundling with a downloader script. That way, if there's any additional feedback on the specific implementation, folks will be able to comment directly on that.
[GitHub] accumulo pull request #122: ACCUMULO-1604: added check for negate option in ...
GitHub user milleruntime opened a pull request: https://github.com/apache/accumulo/pull/122 ACCUMULO-1604: added check for negate option in ColumnAgeOffFilter an⦠â¦d new test in FilterTest You can merge this pull request into a Git repository by running: $ git pull https://github.com/milleruntime/accumulo ACCUMULO-1604 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/accumulo/pull/122.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #122 commit b045255770676e034b6a394768c34af81a13192c Author: Mike Miller Date: 2016-07-05T19:11:26Z ACCUMULO-1604: added check for negate option in ColumnAgeOffFilter and new test in FilterTest --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: ACCUMULO-1604 Bug Fix
On Tue, Jul 5, 2016 at 11:50 AM Mike Miller wrote: > Is > there a preferred technique of code submission for non-committers? > > My personal preference: GitHub pull requests. :)
Re: ACCUMULO-1604 Bug Fix
Feel free to use `git-am` to create a patch or just submit a
pull-request against the master branch of apache/accumulo on Github.
Mike Miller wrote:
OK since the problem is really with the ColumnAgeOffFilter, I will go with
the simple check in the constructor and not make any changes to Filter. Is
there a preferred technique of code submission for non-committers?
On Fri, Jul 1, 2016 at 3:28 PM, Keith Turner wrote:
On Fri, Jul 1, 2016 at 3:24 PM, Mike Miller
wrote:
What about an enum in the superclass (Filter.java) that contains all the
options specific to Filter?
public static enum FilterOptions { NEGATE }
And then skip any options in that enum in the constructor of TTLSet.
Someone would still have to include new options in the enum but at least
it
would be more obvious.
That seems ok. Could also have a protected method, something like
super.isValidOption(String). Not sure I like that method name. A method
gives the super class more flexibility.
On Fri, Jul 1, 2016 at 2:32 PM, Keith Turner wrote:
On Fri, Jul 1, 2016 at 1:29 PM, Mike Miller
wrote:
Hello Devs,
For those of you I have yet to meet, my name is Mike Miller and I am
the
newbie who is going to be helping out on Accumulo development. I have
been
working with Accumulo as a Java developer on an ingest and query
project
for the past year. I doubt I'll be able to fill the shoes of past
contributors but I hope that I can make a positive contribution to
the
project.
I have browsed the JIRA tickets labelled "newbie" and selected the
ACCUMULO-1604
bug
to
fix (more so as a learning exercise than anything). I just wanted to
make
sure I am reproducing the bug correctly and to propose a solution.
I believe I reproduced the bug by getting the ColumnAgeOffFilter to
throw
an "IllegalArgumentException: bad TTL options" in the
o.a.a.core.iterators.user.FilterTest.test2a(). I did this by adding
the
following line:
ColumnAgeOffFilter.setNegate(is, true);
This Exception is caused by: NumberFormatException: For input string
"true"
The comments in the ticket discuss adding "column:" prefix to options
for
the ColumnAgeOffFilter. Please correct me if I am wrong but I think
it
can
be fixed without having to change the syntax of the options. Wouldn't
adding a check to the loop in the constructor of TTLSet to ignore the
NEGATE option on any strings in objectStrings param prevent this
error
from
occurring? The negate option would still be seen by the parent,
without
any further changes.
That would fix it. It does not solve the case where new options are
added
to the superclass in the future. However the option you proposed is
infinitely better than doing nothing and I would be in favor of making
that
change.
Implementing the "column:" prefix in a backwards compatible way is very
tricky, especially if considering the case where someone currently has
an
actual column name with that prefix.
Looking forward to working with you folks!
-Mike M.
Re: ACCUMULO-1604 Bug Fix
OK since the problem is really with the ColumnAgeOffFilter, I will go with
the simple check in the constructor and not make any changes to Filter. Is
there a preferred technique of code submission for non-committers?
On Fri, Jul 1, 2016 at 3:28 PM, Keith Turner wrote:
> On Fri, Jul 1, 2016 at 3:24 PM, Mike Miller
> wrote:
>
> > What about an enum in the superclass (Filter.java) that contains all the
> > options specific to Filter?
> > public static enum FilterOptions { NEGATE }
> > And then skip any options in that enum in the constructor of TTLSet.
> > Someone would still have to include new options in the enum but at least
> it
> > would be more obvious.
> >
>
>
> That seems ok. Could also have a protected method, something like
> super.isValidOption(String). Not sure I like that method name. A method
> gives the super class more flexibility.
>
>
> >
> > On Fri, Jul 1, 2016 at 2:32 PM, Keith Turner wrote:
> >
> > > On Fri, Jul 1, 2016 at 1:29 PM, Mike Miller
> > > wrote:
> > >
> > > > Hello Devs,
> > > >
> > > > For those of you I have yet to meet, my name is Mike Miller and I am
> > the
> > > > newbie who is going to be helping out on Accumulo development. I have
> > > been
> > > > working with Accumulo as a Java developer on an ingest and query
> > project
> > > > for the past year. I doubt I'll be able to fill the shoes of past
> > > > contributors but I hope that I can make a positive contribution to
> the
> > > > project.
> > > >
> > > > I have browsed the JIRA tickets labelled "newbie" and selected the
> > > > ACCUMULO-1604
> > bug
> > > to
> > > > fix (more so as a learning exercise than anything). I just wanted to
> > make
> > > > sure I am reproducing the bug correctly and to propose a solution.
> > > >
> > > > I believe I reproduced the bug by getting the ColumnAgeOffFilter to
> > throw
> > > > an "IllegalArgumentException: bad TTL options" in the
> > > > o.a.a.core.iterators.user.FilterTest.test2a(). I did this by adding
> the
> > > > following line:
> > > > ColumnAgeOffFilter.setNegate(is, true);
> > > > This Exception is caused by: NumberFormatException: For input string
> > > "true"
> > > >
> > > > The comments in the ticket discuss adding "column:" prefix to options
> > for
> > > > the ColumnAgeOffFilter. Please correct me if I am wrong but I think
> it
> > > can
> > > > be fixed without having to change the syntax of the options. Wouldn't
> > > > adding a check to the loop in the constructor of TTLSet to ignore the
> > > > NEGATE option on any strings in objectStrings param prevent this
> error
> > > from
> > > > occurring? The negate option would still be seen by the parent,
> > without
> > > > any further changes.
> > > >
> > >
> > >
> > > That would fix it. It does not solve the case where new options are
> > added
> > > to the superclass in the future. However the option you proposed is
> > > infinitely better than doing nothing and I would be in favor of making
> > that
> > > change.
> > >
> > > Implementing the "column:" prefix in a backwards compatible way is very
> > > tricky, especially if considering the case where someone currently has
> an
> > > actual column name with that prefix.
> > >
> > >
> > > >
> > > > Looking forward to working with you folks!
> > > > -Mike M.
> > > >
> > >
> >
>
[GitHub] accumulo issue #121: ACCUMULO-4353: Stabilize tablet assignment during trans...
Github user ShawnWalker commented on the issue: https://github.com/apache/accumulo/pull/121 > The stop-here.sh command has the master unload the tablets I think. How will this patch handle that case? This patch won't handle such a case at all. I'm sure it shows my inexperience with Accumulo, but I was unaware of this script. I'm more familiar with engineering and dealing with [crash-only software](https://www.usenix.org/legacy/events/hotos03/tech/full_papers/candea/candea.pdf). I had assumed that a tserver would be stopped by SIGTERM or SIGKILL. I'm open to suggestions on how to handle this use case. My current thought would be to make unloading a tablet this way suspend the tablet instead of unassigning it. I.e. in `tserver.TabletServer.UnloadTabletHandler.run()` at line 2012, call `TabletStateStore.suspend(...)` instead of `TabletStateStore.unassign(...)`. > When a tablet server is suspended, all queries will block right? When a *tablet* is suspended, all queries against that tablet do seem to block (or possibly time out). > I see you are suspending the metadata tablets too. By default, metadata tablets won't be suspended, even if the metadata table (or global configuration) has `tablet.suspend.duration` set. One must also set the option `master.metadata.suspendable` to true (default false). The check for this is handled at Master.java:1154. Note to self: Looking back at that code, I realize that this check is made only once (at startup), instead of rechecking for updated configuration. Should probably make that check repeatedly. > I see you are storing the host and port in the metadata for a suspended tablet. Sometimes we have tservers come up with a different host or port. In that case, I guess the tablets will wait until the suspend duration to be reassigned. This is correct. Tablet suspension is essentially incompatible with dynamic port assignment. Of course, this wouldn't be the only part of Accumulo to suffer under random/dynamic port assignment. Specifying `tserv.port.client==0` or `tserv.port.search==true` breaks assumptions in other places too. Some I know of: * I decided to match host+port based on code in `server.master.balance.DefaultLoadBalancer.getAssignment()`. That code uses host+port to match a tablet's `last` column, for preserving locality. If the tserver's port changes, the `last` column is effectively ignored, reducing locality. * Having walked the logic path for `stop-here.sh`, my read is that `server.util.Admin.stopTabletServer(...)` (used by stop-here.sh) assumes tserver(s) on the specified host (resp. localhost) will be on port(s) specified by `tserv.port.client`. Hence, running a tserver with `tserv.port.client`==0 will render `stop-here.sh` ineffective. Similarly, running a tserver with `tserv.port.search==true` risks rendering `stop-here.sh` ineffective. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
