You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Laszlo Hornyak <la...@gmail.com> on 2013/09/30 19:39:43 UTC
Review Request 14405: ConstantTimeBackoff test and cleanup
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14405/
-----------------------------------------------------------
Review request for cloudstack.
Repository: cloudstack-git
Description
-------
- javadoc changed - the old one was copy-pasted from AgentShell
- start and stop method removed - they did the same as the overridden methods
- _counter removed as it was only written, but never read
- remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
- Tests created for the above scenarios.
Diffs
-----
agent/src/com/cloud/agent/AgentShell.java bf1e818
utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java 976e369
utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/14405/diff/
Testing
-------
test included
Thanks,
Laszlo Hornyak
Re: Review Request 14405: ConstantTimeBackoff test and cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
> On Sept. 30, 2013, 6:12 p.m., Darren Shepherd wrote:
> > utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java, line 60
> > <https://reviews.apache.org/r/14405/diff/1/?file=359645#file359645line60>
> >
> > I wouldn't log at WARN level for this, can you change to DEBUG. Hitting that catch block is expected as part of the wakeup behavior
Can we agree in an INFO level log? It seems it is not a usual flow of events, but caused by administrator interaction through a JMX (see ConstantTimeBackoffMBean)
- Laszlo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14405/#review26503
-----------------------------------------------------------
On Sept. 30, 2013, 5:39 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14405/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2013, 5:39 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - javadoc changed - the old one was copy-pasted from AgentShell
> - start and stop method removed - they did the same as the overridden methods
> - _counter removed as it was only written, but never read
> - remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
> - Tests created for the above scenarios.
>
>
> Diffs
> -----
>
> agent/src/com/cloud/agent/AgentShell.java bf1e818
> utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java 976e369
> utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14405/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 14405: ConstantTimeBackoff test and cleanup
Posted by Darren Shepherd <da...@gmail.com>.
> On Sept. 30, 2013, 6:12 p.m., Darren Shepherd wrote:
> > utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java, line 60
> > <https://reviews.apache.org/r/14405/diff/1/?file=359645#file359645line60>
> >
> > I wouldn't log at WARN level for this, can you change to DEBUG. Hitting that catch block is expected as part of the wakeup behavior
>
> Laszlo Hornyak wrote:
> Can we agree in an INFO level log? It seems it is not a usual flow of events, but caused by administrator interaction through a JMX (see ConstantTimeBackoffMBean)
How about INFO, but don't print stack trace (so e.getMessage())? I'd rather it not look like an error. Or maybe not log the exception at all as its seems misleading. I hate to debate over log statements, so change if you want or not. I'll commit this when you close the issue.
- Darren
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14405/#review26503
-----------------------------------------------------------
On Sept. 30, 2013, 8:27 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14405/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2013, 8:27 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - javadoc changed - the old one was copy-pasted from AgentShell
> - start and stop method removed - they did the same as the overridden methods
> - _counter removed as it was only written, but never read
> - remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
> - Tests created for the above scenarios.
>
>
> Diffs
> -----
>
> agent/src/com/cloud/agent/AgentShell.java bf1e818
> utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java 976e369
> utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14405/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 14405: ConstantTimeBackoff test and cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
> On Sept. 30, 2013, 6:12 p.m., Darren Shepherd wrote:
> > utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java, line 60
> > <https://reviews.apache.org/r/14405/diff/1/?file=359645#file359645line60>
> >
> > I wouldn't log at WARN level for this, can you change to DEBUG. Hitting that catch block is expected as part of the wakeup behavior
>
> Laszlo Hornyak wrote:
> Can we agree in an INFO level log? It seems it is not a usual flow of events, but caused by administrator interaction through a JMX (see ConstantTimeBackoffMBean)
>
> Darren Shepherd wrote:
> How about INFO, but don't print stack trace (so e.getMessage())? I'd rather it not look like an error. Or maybe not log the exception at all as its seems misleading. I hate to debate over log statements, so change if you want or not. I'll commit this when you close the issue.
Ok, agreed. It is not an error and the stack trace would be the same all the time. I will fix this.
- Laszlo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14405/#review26503
-----------------------------------------------------------
On Sept. 30, 2013, 8:27 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14405/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2013, 8:27 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - javadoc changed - the old one was copy-pasted from AgentShell
> - start and stop method removed - they did the same as the overridden methods
> - _counter removed as it was only written, but never read
> - remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
> - Tests created for the above scenarios.
>
>
> Diffs
> -----
>
> agent/src/com/cloud/agent/AgentShell.java bf1e818
> utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java 976e369
> utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14405/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 14405: ConstantTimeBackoff test and cleanup
Posted by Darren Shepherd <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14405/#review26503
-----------------------------------------------------------
utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java
<https://reviews.apache.org/r/14405/#comment51681>
I wouldn't log at WARN level for this, can you change to DEBUG. Hitting that catch block is expected as part of the wakeup behavior
utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java
<https://reviews.apache.org/r/14405/#comment51682>
Can you add the standard apache ASL /* licenese header */ to all new files?
- Darren Shepherd
On Sept. 30, 2013, 5:39 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14405/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2013, 5:39 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - javadoc changed - the old one was copy-pasted from AgentShell
> - start and stop method removed - they did the same as the overridden methods
> - _counter removed as it was only written, but never read
> - remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
> - Tests created for the above scenarios.
>
>
> Diffs
> -----
>
> agent/src/com/cloud/agent/AgentShell.java bf1e818
> utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java 976e369
> utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14405/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 14405: ConstantTimeBackoff test and cleanup
Posted by Darren Shepherd <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14405/#review26688
-----------------------------------------------------------
Ship it!
master: 826c69fd29bb98d3f631596f72a6eb3525d83aa2
- Darren Shepherd
On Oct. 1, 2013, 5:32 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14405/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2013, 5:32 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> - javadoc changed - the old one was copy-pasted from AgentShell
> - start and stop method removed - they did the same as the overridden methods
> - _counter removed as it was only written, but never read
> - remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
> - Tests created for the above scenarios.
>
>
> Diffs
> -----
>
> agent/src/com/cloud/agent/AgentShell.java bf1e818
> utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java 976e369
> utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14405/diff/
>
>
> Testing
> -------
>
> test included
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 14405: ConstantTimeBackoff test and cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14405/
-----------------------------------------------------------
(Updated Oct. 1, 2013, 5:32 p.m.)
Review request for cloudstack.
Changes
-------
No stack trace logged in this version
Repository: cloudstack-git
Description
-------
- javadoc changed - the old one was copy-pasted from AgentShell
- start and stop method removed - they did the same as the overridden methods
- _counter removed as it was only written, but never read
- remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
- Tests created for the above scenarios.
Diffs (updated)
-----
agent/src/com/cloud/agent/AgentShell.java bf1e818
utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java 976e369
utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/14405/diff/
Testing
-------
test included
Thanks,
Laszlo Hornyak
Re: Review Request 14405: ConstantTimeBackoff test and cleanup
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14405/
-----------------------------------------------------------
(Updated Sept. 30, 2013, 8:27 p.m.)
Review request for cloudstack.
Changes
-------
- license comment added to the test file
- log level decreased to INFO
Repository: cloudstack-git
Description
-------
- javadoc changed - the old one was copy-pasted from AgentShell
- start and stop method removed - they did the same as the overridden methods
- _counter removed as it was only written, but never read
- remove from _asleep map was moved to a finally block, to make sure it is removed even in case of the thread gets interrupted
- Tests created for the above scenarios.
Diffs (updated)
-----
agent/src/com/cloud/agent/AgentShell.java bf1e818
utils/src/com/cloud/utils/backoff/impl/ConstantTimeBackoff.java 976e369
utils/test/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/14405/diff/
Testing
-------
test included
Thanks,
Laszlo Hornyak