You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by alasdairhodge <gi...@git.apache.org> on 2018/01/11 13:21:03 UTC
[GitHub] brooklyn-server pull request #930: WithMutexes cleanup
GitHub user alasdairhodge opened a pull request:
https://github.com/apache/brooklyn-server/pull/930
WithMutexes cleanup
Modifies signature of `WithMutexes.acquireMutex()` to throw unchecked `RuntimeInterruptedException` to ease the burden on callers.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alasdairhodge/brooklyn-server withmutexes-cleanup
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/930.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 #930
----
commit fe2c43569a1f3f736e5716a293028fd5b6ce7fa2
Author: Alasdair Hodge <gi...@...>
Date: 2018-01-11T11:37:48Z
Change signature of WithMutexes.acquireMutex() to throw unchecked RuntimeInterruptedException
commit f341f290a6dd82ca923afe9615563db3c2446eb9
Author: Alasdair Hodge <gi...@...>
Date: 2018-01-11T13:13:01Z
Cleanup mutex handling in AbstractSoftwareProcessSshDriver
----
---
[GitHub] brooklyn-server issue #930: WithMutexes cleanup
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/930
I agree this is an improvement, and is worth doing.
It's worth noting that it is not backwards compatible, if you consider the `org.apache.brooklyn.util.core.mutex.MutexSupport` to be part of our "public" api: code that has a `catch (InterruptedException e)` will no longer compile. It's a simple change in downstream code to either remove the catch block, or to instead catch the `RuntimeInterruptedException`.
I think it's acceptable to do this now (particularly before we release a 1.0.0).
If no-one objects, I'll merge once jenkins confirms the tests pass.
---
[GitHub] brooklyn-server pull request #930: WithMutexes cleanup
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/930#discussion_r160968622
--- Diff: core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java ---
@@ -1016,12 +1015,8 @@ public String findPassword() {
/** @deprecated since 1.0.0; mutex-related methods are now accessible via {@link #mutexes()} */
@Override
@Deprecated
- public void acquireMutex(String mutexId, String description) throws RuntimeInterruptedException {
- try {
- mutexes().acquireMutex(mutexId, description);
- } catch (InterruptedException ie) {
- throw new RuntimeInterruptedException("Interrupted waiting for mutex: " + mutexId, ie);
- }
+ public void acquireMutex(String mutexId, String description) {
--- End diff --
I would leave the `throws` in place, and change the `MutexSupport.acquireMutex` to also declare `throws RuntimeInterruptedException`. Because it's runtime you can ignore it as a caller if you want, but the tooling support in IDEs will get some help for anyone who does want to catch it.
---
[GitHub] brooklyn-server pull request #930: WithMutexes cleanup
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/930
---
[GitHub] brooklyn-server pull request #930: WithMutexes cleanup
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/930#discussion_r160970643
--- Diff: core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java ---
@@ -1016,12 +1015,8 @@ public String findPassword() {
/** @deprecated since 1.0.0; mutex-related methods are now accessible via {@link #mutexes()} */
@Override
@Deprecated
- public void acquireMutex(String mutexId, String description) throws RuntimeInterruptedException {
- try {
- mutexes().acquireMutex(mutexId, description);
- } catch (InterruptedException ie) {
- throw new RuntimeInterruptedException("Interrupted waiting for mutex: " + mutexId, ie);
- }
+ public void acquireMutex(String mutexId, String description) {
--- End diff --
Effective Java (Item 62) recommends against this for unchecked exceptions, although I've no strong feelings.
---