You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Sean Busbey <se...@manvsbeard.com> on 2013/10/23 21:08:42 UTC
Review Request 14880: ACCUMULO-1785 Alter config.sh to optionally just
verify environment instead of making changes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14880/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-1785
https://issues.apache.org/jira/browse/ACCUMULO-1785
Repository: accumulo
Description
-------
Updates config.sh to skip automagic standalone mode and avoid writing to the local filesystem given an optional environment variable. Adds additional error handling since this changes the invariant that a conf/masters file will exist.
Diffs
-----
bin/config.sh 9277406
Diff: https://reviews.apache.org/r/14880/diff/
Testing
-------
functional tests
starting w/o env variable set and no master/slaves correctly started in standalone mode.
starting w/env variable and no required variables and no master/slaves correctly complained and failed.
starting w/env variable and required variables and no master/slaves correctly init w/o writing crap, correctly failed to use start-all
starting with a normal cluster config
Thanks,
Sean Busbey
Re: Review Request 14880: ACCUMULO-1785 Alter config.sh to optionally just
verify environment instead of making changes
Posted by Joey Echeverria <jo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14880/#review27829
-----------------------------------------------------------
Ship it!
I think it's best to keep the message about the missing masters file on STDERR and that Sean's comments addressed Tubbs's other questions, so I'm +1 on the patch once the change that's now in ACCUMULO-1810 is removed from this patch.
- Joey Echeverria
On Oct. 23, 2013, 7:08 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14880/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 7:08 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1785
> https://issues.apache.org/jira/browse/ACCUMULO-1785
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Updates config.sh to skip automagic standalone mode and avoid writing to the local filesystem given an optional environment variable. Adds additional error handling since this changes the invariant that a conf/masters file will exist.
>
>
> Diffs
> -----
>
> bin/config.sh 9277406
>
> Diff: https://reviews.apache.org/r/14880/diff/
>
>
> Testing
> -------
>
> functional tests
> starting w/o env variable set and no master/slaves correctly started in standalone mode.
> starting w/env variable and no required variables and no master/slaves correctly complained and failed.
> starting w/env variable and required variables and no master/slaves correctly init w/o writing crap, correctly failed to use start-all
> starting with a normal cluster config
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 14880: ACCUMULO-1785 Alter config.sh to optionally just
verify environment instead of making changes
Posted by John Vines <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14880/#review28118
-----------------------------------------------------------
Ship it!
Ship It!
- John Vines
On Oct. 23, 2013, 7:08 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14880/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 7:08 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1785
> https://issues.apache.org/jira/browse/ACCUMULO-1785
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Updates config.sh to skip automagic standalone mode and avoid writing to the local filesystem given an optional environment variable. Adds additional error handling since this changes the invariant that a conf/masters file will exist.
>
>
> Diffs
> -----
>
> bin/config.sh 9277406
>
> Diff: https://reviews.apache.org/r/14880/diff/
>
>
> Testing
> -------
>
> functional tests
> starting w/o env variable set and no master/slaves correctly started in standalone mode.
> starting w/env variable and no required variables and no master/slaves correctly complained and failed.
> starting w/env variable and required variables and no master/slaves correctly init w/o writing crap, correctly failed to use start-all
> starting with a normal cluster config
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 14880: ACCUMULO-1785 Alter config.sh to optionally just
verify environment instead of making changes
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Oct. 23, 2013, 8:19 p.m., kturner wrote:
> > I would prefer to remove all of the automatic actions and replace them with messages (like those in your patch) instructing the user what to do. I have seen unwanted behavior in the past. Howerver, I do not think a behavioral change like this should be made in 1.4. So I like your approach of being able to optionally suppress the automation actions.
follow up discussion for breaking it into another script or accumulo command in 1.6.0-SNAPSHOT on dev@accumulo?
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14880/#review27415
-----------------------------------------------------------
On Oct. 23, 2013, 7:08 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14880/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 7:08 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1785
> https://issues.apache.org/jira/browse/ACCUMULO-1785
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Updates config.sh to skip automagic standalone mode and avoid writing to the local filesystem given an optional environment variable. Adds additional error handling since this changes the invariant that a conf/masters file will exist.
>
>
> Diffs
> -----
>
> bin/config.sh 9277406
>
> Diff: https://reviews.apache.org/r/14880/diff/
>
>
> Testing
> -------
>
> functional tests
> starting w/o env variable set and no master/slaves correctly started in standalone mode.
> starting w/env variable and no required variables and no master/slaves correctly complained and failed.
> starting w/env variable and required variables and no master/slaves correctly init w/o writing crap, correctly failed to use start-all
> starting with a normal cluster config
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 14880: ACCUMULO-1785 Alter config.sh to optionally just
verify environment instead of making changes
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14880/#review27415
-----------------------------------------------------------
I would prefer to remove all of the automatic actions and replace them with messages (like those in your patch) instructing the user what to do. I have seen unwanted behavior in the past. Howerver, I do not think a behavioral change like this should be made in 1.4. So I like your approach of being able to optionally suppress the automation actions.
- kturner
On Oct. 23, 2013, 7:08 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14880/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 7:08 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1785
> https://issues.apache.org/jira/browse/ACCUMULO-1785
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Updates config.sh to skip automagic standalone mode and avoid writing to the local filesystem given an optional environment variable. Adds additional error handling since this changes the invariant that a conf/masters file will exist.
>
>
> Diffs
> -----
>
> bin/config.sh 9277406
>
> Diff: https://reviews.apache.org/r/14880/diff/
>
>
> Testing
> -------
>
> functional tests
> starting w/o env variable set and no master/slaves correctly started in standalone mode.
> starting w/env variable and no required variables and no master/slaves correctly complained and failed.
> starting w/env variable and required variables and no master/slaves correctly init w/o writing crap, correctly failed to use start-all
> starting with a normal cluster config
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 14880: ACCUMULO-1785 Alter config.sh to optionally just
verify environment instead of making changes
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14880/#review27904
-----------------------------------------------------------
Looks fine to me given Keith and Christopher's comments.
- Josh Elser
On Oct. 23, 2013, 7:08 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14880/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 7:08 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1785
> https://issues.apache.org/jira/browse/ACCUMULO-1785
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Updates config.sh to skip automagic standalone mode and avoid writing to the local filesystem given an optional environment variable. Adds additional error handling since this changes the invariant that a conf/masters file will exist.
>
>
> Diffs
> -----
>
> bin/config.sh 9277406
>
> Diff: https://reviews.apache.org/r/14880/diff/
>
>
> Testing
> -------
>
> functional tests
> starting w/o env variable set and no master/slaves correctly started in standalone mode.
> starting w/env variable and no required variables and no master/slaves correctly complained and failed.
> starting w/env variable and required variables and no master/slaves correctly init w/o writing crap, correctly failed to use start-all
> starting with a normal cluster config
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 14880: ACCUMULO-1785 Alter config.sh to optionally just
verify environment instead of making changes
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Oct. 23, 2013, 8:40 p.m., Christopher Tubbs wrote:
> > bin/config.sh, lines 139-142
> > <https://reviews.apache.org/r/14880/diff/1/?file=369928#file369928line139>
> >
> > This looks like it will spit stuff to STDERR if masters file isn't there and ACCUMULO_VERIFY_ONLY is set, and won't set GC.
Right, it will output a message to stderr that CONF_DIR/masters doesn't exist. Should I suppress that message?
It will still set GC if it is not set (provided CONF_DIR/gc exists)
> On Oct. 23, 2013, 8:40 p.m., Christopher Tubbs wrote:
> > bin/config.sh, lines 147-149
> > <https://reviews.apache.org/r/14880/diff/1/?file=369928#file369928line147>
> >
> > Can this still happen if ACCUMULO_VERIFY_ONLY is set and no masters file exists?
yes. CONF_DIR/gc still gets loaded if it exists. if GC was set, none of this would be checked at all.
If none of those are true, config.sh will exit with non-zero status to indicate an error.
> On Oct. 23, 2013, 8:40 p.m., Christopher Tubbs wrote:
> > bin/config.sh, lines 152-159
> > <https://reviews.apache.org/r/14880/diff/1/?file=369928#file369928line152>
> >
> > Same issue with GC not being set applies to MONITOR not being set and can't be set from master if ACCUMULO_VERIFY_ONLY is set and no masters file.
if there was no masters file, and no monitor file, and MONITOR wasn't set, the script will exist with non-zero status.
> On Oct. 23, 2013, 8:40 p.m., Christopher Tubbs wrote:
> > bin/config.sh, line 42
> > <https://reviews.apache.org/r/14880/diff/1/?file=369928#file369928line42>
> >
> > This should be a separate commit
broken out into ACCUMULO-1810
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14880/#review27417
-----------------------------------------------------------
On Oct. 23, 2013, 7:08 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14880/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 7:08 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1785
> https://issues.apache.org/jira/browse/ACCUMULO-1785
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Updates config.sh to skip automagic standalone mode and avoid writing to the local filesystem given an optional environment variable. Adds additional error handling since this changes the invariant that a conf/masters file will exist.
>
>
> Diffs
> -----
>
> bin/config.sh 9277406
>
> Diff: https://reviews.apache.org/r/14880/diff/
>
>
> Testing
> -------
>
> functional tests
> starting w/o env variable set and no master/slaves correctly started in standalone mode.
> starting w/env variable and no required variables and no master/slaves correctly complained and failed.
> starting w/env variable and required variables and no master/slaves correctly init w/o writing crap, correctly failed to use start-all
> starting with a normal cluster config
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 14880: ACCUMULO-1785 Alter config.sh to optionally just
verify environment instead of making changes
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14880/#review27417
-----------------------------------------------------------
bin/config.sh
<https://reviews.apache.org/r/14880/#comment53275>
This should be a separate commit
bin/config.sh
<https://reviews.apache.org/r/14880/#comment53277>
This looks like it will spit stuff to STDERR if masters file isn't there and ACCUMULO_VERIFY_ONLY is set, and won't set GC.
bin/config.sh
<https://reviews.apache.org/r/14880/#comment53276>
Can this still happen if ACCUMULO_VERIFY_ONLY is set and no masters file exists?
bin/config.sh
<https://reviews.apache.org/r/14880/#comment53278>
Same issue with GC not being set applies to MONITOR not being set and can't be set from master if ACCUMULO_VERIFY_ONLY is set and no masters file.
- Christopher Tubbs
On Oct. 23, 2013, 3:08 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14880/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 3:08 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1785
> https://issues.apache.org/jira/browse/ACCUMULO-1785
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Updates config.sh to skip automagic standalone mode and avoid writing to the local filesystem given an optional environment variable. Adds additional error handling since this changes the invariant that a conf/masters file will exist.
>
>
> Diffs
> -----
>
> bin/config.sh 9277406
>
> Diff: https://reviews.apache.org/r/14880/diff/
>
>
> Testing
> -------
>
> functional tests
> starting w/o env variable set and no master/slaves correctly started in standalone mode.
> starting w/env variable and no required variables and no master/slaves correctly complained and failed.
> starting w/env variable and required variables and no master/slaves correctly init w/o writing crap, correctly failed to use start-all
> starting with a normal cluster config
>
>
> Thanks,
>
> Sean Busbey
>
>