You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by neykov <gi...@git.apache.org> on 2017/01/27 11:07:28 UTC
[GitHub] brooklyn-server pull request #539: Log a warning in case of max-concurrency ...
GitHub user neykov opened a pull request:
https://github.com/apache/brooklyn-server/pull/539
Log a warning in case of max-concurrency latch double-acquire
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/neykov/brooklyn-server max-concurrency
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/539.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 #539
----
commit fd734ebb572ee942bcd38a042807941e12ab690e
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2017-01-27T11:05:44Z
Log a warning in case of max-concurrency latch double-acquire
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #539: Log a warning in case of max-concurrency ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/539#discussion_r98198465
--- Diff: core/src/main/java/org/apache/brooklyn/core/sensor/ReleaseableLatch.java ---
@@ -75,7 +89,11 @@ public void acquire(Entity caller) {
@Override
public void release(Entity caller) {
- sem.release();
+ try {
+ sem.release();
+ } finally {
+ ownerEntities.remove(caller);
--- End diff --
Agree, this was my thinking as well. Can always update it when we need to address a more complex use-case.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #539: Log a warning in case of max-concurrency ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/539#discussion_r98187488
--- Diff: core/src/main/java/org/apache/brooklyn/core/sensor/ReleaseableLatch.java ---
@@ -56,8 +60,11 @@
}
private static class MaxConcurrencyLatch implements ReleaseableLatch {
+ private static final Logger LOG = LoggerFactory.getLogger(MaxConcurrencyLatch.class);
+
private int permits;
private transient final Semaphore sem;
+ private transient final Set<Entity> ownerEntities = new ConcurrentHashSet<Entity>();
--- End diff --
The `transient` makes me think you're trying to handle persistence of the `MaxConcurrencyLatch` - is that right?
If so, then this will fail after rebind because the field `sem` will be null. We'd need a readResolve method to set it, or a getter that lazily instantiates the semaphore.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #539: Log a warning in case of max-concurrency ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/539#discussion_r98187291
--- Diff: core/src/main/java/org/apache/brooklyn/core/sensor/ReleaseableLatch.java ---
@@ -66,6 +73,13 @@ public MaxConcurrencyLatch(int permits) {
@Override
public void acquire(Entity caller) {
+ if (!ownerEntities.add(caller)) {
+ LOG.warn("Entity {} trying to double-acquire permit with ~{} permits available and ~{} threads waiting in queue",
--- End diff --
Perhaps change this log message slightly:
```
LOG.warn("Entity {} acquiring permit multiple times with ~{} permits available ...
```
It could otherwise be mis-understood as saying that the entity was trying to do it, but that it's being skipped.
Worth adding javadoc to say what the semantics are for reentrant calls (and multiple calls by the same entity from different threads).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #539: Log a warning in case of max-concurrency ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/539#discussion_r98198377
--- Diff: core/src/main/java/org/apache/brooklyn/core/sensor/ReleaseableLatch.java ---
@@ -56,8 +60,11 @@
}
private static class MaxConcurrencyLatch implements ReleaseableLatch {
+ private static final Logger LOG = LoggerFactory.getLogger(MaxConcurrencyLatch.class);
+
private int permits;
private transient final Semaphore sem;
+ private transient final Set<Entity> ownerEntities = new ConcurrentHashSet<Entity>();
--- End diff --
Good spot, haven't considered this case. It's handled and covered by tests though - there's a `readResolve` further down. Added a comment.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #539: Log a warning in case of max-concurrency ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/539
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #539: Log a warning in case of max-concurrency ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/539#discussion_r98187655
--- Diff: core/src/main/java/org/apache/brooklyn/core/sensor/ReleaseableLatch.java ---
@@ -75,7 +89,11 @@ public void acquire(Entity caller) {
@Override
public void release(Entity caller) {
- sem.release();
+ try {
+ sem.release();
+ } finally {
+ ownerEntities.remove(caller);
--- End diff --
I think this is fine. If we wanted to get more complicated, we could use a MultiMap or some such so that we know how many times the entity has called acquire(). But that doesn't feel worth the complexity, so I like the code as-is.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #539: Log a warning in case of max-concurrency latch d...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/539
Thanks @neykov - LGTM; merging
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #539: Log a warning in case of max-concurrency ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/539#discussion_r98198266
--- Diff: core/src/main/java/org/apache/brooklyn/core/sensor/ReleaseableLatch.java ---
@@ -66,6 +73,13 @@ public MaxConcurrencyLatch(int permits) {
@Override
public void acquire(Entity caller) {
+ if (!ownerEntities.add(caller)) {
+ LOG.warn("Entity {} trying to double-acquire permit with ~{} permits available and ~{} threads waiting in queue",
--- End diff --
Updated, thanks for the input.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---