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 2014/07/01 18:23:40 UTC

Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/
-----------------------------------------------------------

Review request for accumulo and Mike Drob.


Bugs: ACCUMULO-2967
    https://issues.apache.org/jira/browse/ACCUMULO-2967


Repository: accumulo


Description
-------

- Tests that we get an exception within a reasonable bound
- Changes timeout condition to check prior to place where an exception can be thrown.


Diffs
-----

  fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
  fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/23200/diff/


Testing
-------

prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.


Thanks,

Sean Busbey

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Bill Havanki <bh...@clouderagovt.com>.

> On July 1, 2014, 1:01 p.m., Bill Havanki wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java, line 119
> > <https://reviews.apache.org/r/23200/diff/2/?file=621394#file621394line119>
> >
> >     If the ZK object is successfully created and connected, the timeout check can still throw an exception. The if check here should also see if tryAgain is true.
> 
> Sean Busbey wrote:
>     That's what it originally did prior to any changes. I presumed it was intentional.

Hm. OK, then either way is fine with me.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47094
-----------------------------------------------------------


On July 1, 2014, 12:55 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 12:55 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Sean Busbey <se...@manvsbeard.com>.

> On July 1, 2014, 5:01 p.m., Bill Havanki wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java, line 119
> > <https://reviews.apache.org/r/23200/diff/2/?file=621394#file621394line119>
> >
> >     If the ZK object is successfully created and connected, the timeout check can still throw an exception. The if check here should also see if tryAgain is true.

That's what it originally did prior to any changes. I presumed it was intentional.


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47094
-----------------------------------------------------------


On July 1, 2014, 4:55 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:55 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47094
-----------------------------------------------------------



fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
<https://reviews.apache.org/r/23200/#comment82701>

    If the ZK object is successfully created and connected, the timeout check can still throw an exception. The if check here should also see if tryAgain is true.


- Bill Havanki


On July 1, 2014, 12:55 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 12:55 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/
-----------------------------------------------------------

(Updated July 1, 2014, 4:55 p.m.)


Review request for accumulo and Mike Drob.


Changes
-------

typo in javadocs, moved timeout check to end of loop.


Bugs: ACCUMULO-2967
    https://issues.apache.org/jira/browse/ACCUMULO-2967


Repository: accumulo


Description
-------

- Tests that we get an exception within a reasonable bound
- Changes timeout condition to check prior to place where an exception can be thrown.


Diffs (updated)
-----

  fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
  fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/23200/diff/


Testing
-------

prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.


Thanks,

Sean Busbey

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Sean Busbey <se...@manvsbeard.com>.

> On July 1, 2014, 4:37 p.m., Bill Havanki wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java, line 70
> > <https://reviews.apache.org/r/23200/diff/1/?file=621351#file621351line70>
> >
> >     Nit: separated (OK to fix on commit)

fixed locally.


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47079
-----------------------------------------------------------


On July 1, 2014, 4:23 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:23 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47079
-----------------------------------------------------------

Ship it!


Ship It!


fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
<https://reviews.apache.org/r/23200/#comment82682>

    Nit: separated (OK to fix on commit)


- Bill Havanki


On July 1, 2014, 12:23 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 12:23 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Sean Busbey <se...@manvsbeard.com>.

> On July 1, 2014, 4:32 p.m., Mike Drob wrote:
> > I assume the test fails prior to the change in the rest of the patch?

yes, even up to a timeout of 5 minutes.


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47082
-----------------------------------------------------------


On July 1, 2014, 4:23 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:23 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47082
-----------------------------------------------------------

Ship it!


I assume the test fails prior to the change in the rest of the patch?

- Mike Drob


On July 1, 2014, 4:23 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:23 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Josh Elser <jo...@gmail.com>.

> On July 1, 2014, 4:32 p.m., Josh Elser wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java, line 88
> > <https://reviews.apache.org/r/23200/diff/1/?file=621351#file621351line88>
> >
> >     Would be nice if we could skip this check on the first iteration.
> 
> Sean Busbey wrote:
>     We almost certainly won't have passed the timeout interval, why add the extra state tracking?
> 
> Josh Elser wrote:
>     To the same argument, why call to get the time again when we almost certainly haven't passed the timeout interval?

You could add another lastCheckedTime which is initialized to startTime outside the loop. Then, at the end of the loop, you could set lastCheckedTime equal to System.currentTimeMillis(). That would make me happy and alleviate some extra boolean.


- Josh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47081
-----------------------------------------------------------


On July 1, 2014, 4:23 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:23 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Josh Elser <jo...@gmail.com>.

> On July 1, 2014, 4:32 p.m., Josh Elser wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java, line 88
> > <https://reviews.apache.org/r/23200/diff/1/?file=621351#file621351line88>
> >
> >     Would be nice if we could skip this check on the first iteration.
> 
> Sean Busbey wrote:
>     We almost certainly won't have passed the timeout interval, why add the extra state tracking?

To the same argument, why call to get the time again when we almost certainly haven't passed the timeout interval?


- Josh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47081
-----------------------------------------------------------


On July 1, 2014, 4:23 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:23 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Josh Elser <jo...@gmail.com>.

> On July 1, 2014, 4:32 p.m., Josh Elser wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java, line 88
> > <https://reviews.apache.org/r/23200/diff/1/?file=621351#file621351line88>
> >
> >     Would be nice if we could skip this check on the first iteration.
> 
> Sean Busbey wrote:
>     We almost certainly won't have passed the timeout interval, why add the extra state tracking?
> 
> Josh Elser wrote:
>     To the same argument, why call to get the time again when we almost certainly haven't passed the timeout interval?
> 
> Josh Elser wrote:
>     You could add another lastCheckedTime which is initialized to startTime outside the loop. Then, at the end of the loop, you could set lastCheckedTime equal to System.currentTimeMillis(). That would make me happy and alleviate some extra boolean.
> 
> Sean Busbey wrote:
>     update patch moves the check to the end of the loop. how's that?

Looks fine. I missed that the old variant was also doing the same thing the first time around. Thanks.


- Josh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47081
-----------------------------------------------------------


On July 1, 2014, 4:55 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:55 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Sean Busbey <se...@manvsbeard.com>.

> On July 1, 2014, 4:32 p.m., Josh Elser wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java, line 88
> > <https://reviews.apache.org/r/23200/diff/1/?file=621351#file621351line88>
> >
> >     Would be nice if we could skip this check on the first iteration.

We almost certainly won't have passed the timeout interval, why add the extra state tracking?


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47081
-----------------------------------------------------------


On July 1, 2014, 4:23 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:23 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Sean Busbey <se...@manvsbeard.com>.

> On July 1, 2014, 4:32 p.m., Josh Elser wrote:
> > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java, line 88
> > <https://reviews.apache.org/r/23200/diff/1/?file=621351#file621351line88>
> >
> >     Would be nice if we could skip this check on the first iteration.
> 
> Sean Busbey wrote:
>     We almost certainly won't have passed the timeout interval, why add the extra state tracking?
> 
> Josh Elser wrote:
>     To the same argument, why call to get the time again when we almost certainly haven't passed the timeout interval?
> 
> Josh Elser wrote:
>     You could add another lastCheckedTime which is initialized to startTime outside the loop. Then, at the end of the loop, you could set lastCheckedTime equal to System.currentTimeMillis(). That would make me happy and alleviate some extra boolean.

update patch moves the check to the end of the loop. how's that?


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47081
-----------------------------------------------------------


On July 1, 2014, 4:55 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:55 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>

Re: Review Request 23200: ACCUMULO-2967 Unknown Host should result in timeout.

Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23200/#review47081
-----------------------------------------------------------



fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
<https://reviews.apache.org/r/23200/#comment82685>

    Would be nice if we could skip this check on the first iteration.


- Josh Elser


On July 1, 2014, 4:23 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23200/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 4:23 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-2967
>     https://issues.apache.org/jira/browse/ACCUMULO-2967
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> - Tests that we get an exception within a reasonable bound
> - Changes timeout condition to check prior to place where an exception can be thrown.
> 
> 
> Diffs
> -----
> 
>   fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 205ff01809141f4fcabf860dbb93469207ac2842 
>   fate/src/test/java/org/apache/accumulo/fate/zookeeper/ZooSessionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23200/diff/
> 
> 
> Testing
> -------
> 
> prior to change, given test fails on timeout (tested up to a 5 minute wait). post change test passes.
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>