You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by nakomis <gi...@git.apache.org> on 2015/03/02 18:37:49 UTC
[GitHub] incubator-brooklyn pull request: Adds addAssociationListener and r...
GitHub user nakomis opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/538
Adds addAssociationListener and removeAssociationListener to PortForward...
...Manager
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nakomis/incubator-brooklyn feature/portforwardmanager-associationlisteners
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/538.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 #538
----
commit 46d45de77de8bc6852be7b872aa644b62765bca1
Author: Martin Harris <gi...@nakomis.com>
Date: 2015-03-02T17:36:15Z
Adds addAssociationListener and removeAssociationListener to PortForwardManager
----
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25636341
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
@@ -46,6 +47,42 @@
@Beta
public interface PortForwardManager extends Location {
+ @Beta
+ class AssociationMetadata {
+ private final String publicIpId;
+ private final HostAndPort publicEndpoint;
+ private final Location location;
+ private final int privatePort;
+
+ public AssociationMetadata(String publicIpId, HostAndPort publicEndpoint, Location location, int privatePort) {
--- End diff --
Worth a javadoc to say: `Users are discouraged from calling this constructor; the signature may change in future releases. Instead, instances will be created automatically by Brooklyn to be passed to the {@link AssociationListener#onEvent(AssociationMetadata)} method.`
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25700196
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
@@ -181,6 +188,20 @@ protected void associateImpl(String publicIpId, HostAndPort publicEndpoint, Loca
onChanged();
}
+ private void emitAssociationCreatedEvent(String publicIpId, HostAndPort publicEndpoint, Location location, int privatePort) {
+ AssociationMetadata metadata = new AssociationMetadata(publicIpId, publicEndpoint, location, privatePort);
+ for (Map.Entry<AssociationListener, Predicate<? super AssociationMetadata>> entry : associationListeners.entrySet()) {
+ if (entry.getValue().apply(metadata)) {
+ try {
+ entry.getKey().onAssociationCreated(metadata);
+ } catch (Exception e) {
+ Exceptions.propagateIfFatal(e);
+ log.warn("Exception thrown when emitting association creation event {}: {}", metadata, e);
--- End diff --
This will just give the toString of the exception, rather than the stacktrace - it would make debugging problems very hard. For example, it might tell you there was an NPE without ever telling you which line that null pointer happened on. Suggest changing it to:
log.warn("Exception thrown when emitting association creation event "+metadata, e);
Same for log message in `emitAssociationDeletedEvent`.
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25638589
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
@@ -46,6 +47,42 @@
@Beta
public interface PortForwardManager extends Location {
+ @Beta
+ class AssociationMetadata {
--- End diff --
Worth adding `toString()` to this - you rely on a nice toString in a debug message of https://github.com/brooklyncentral/advanced-networking/pull/39
e.g.:
public String toString() {
return Objects.toStringHelper()
.add("publicIpId", publicIpId)
.add("publicEndpoint", publicEndpoint)
.add("location", location)
.add("privatePort", privatePort)
.build();
}
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25694801
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
@@ -208,6 +223,7 @@ public boolean forgetPortMapping(String publicIpId, int publicPort) {
PortMapping old;
synchronized (mutex) {
old = mappings.remove(makeKey(publicIpId, publicPort));
+ emitAssociationDeletedEvent(associationMetadataFromPortMapping(old));
--- End diff --
Guard this with `if (old != null)` - i.e. was something really removed?
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#issuecomment-76819539
Looks good; a few comments worth addressing. Ping me when it's ready to review again.
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25695467
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
@@ -181,6 +187,15 @@ protected void associateImpl(String publicIpId, HostAndPort publicEndpoint, Loca
onChanged();
}
+ private void emitAssociationCreatedEvent(String publicIpId, HostAndPort publicEndpoint, Location location, int privatePort) {
+ AssociationMetadata metadata = new AssociationMetadata(publicIpId, publicEndpoint, location, privatePort);
+ for (Map.Entry<AssociationListener, Predicate<? super AssociationMetadata>> entry : associationListeners.entrySet()) {
+ if (entry.getValue().apply(metadata)) {
+ entry.getKey().onAssociationCreated(metadata);
--- End diff --
Should wrap this in try-catch block, to log.warn any exception. An exception in one listener shouldn't cause the other listeners to not be called. However, in the catch block first do `Exceptions.propagateIfFatal(e)`.
Same goes for the call to `onAssociationDeleted(metadata)`.
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25636490
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
@@ -92,6 +129,12 @@
* subsequently be looked up using {@link #lookup(String, int)}.
*/
public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort);
+
+ @Beta
+ public void addAssociationListener(AssociationListener listener, Predicate<? super AssociationMetadata> filter);
--- End diff --
Worth javadoc to say `Registers a listener, which will be notified each time a new port mapping is associated. See {@link #associate(String, HostAndPort, int)} and {@link #associate(String, HostAndPort, Location, int)}.`
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25674284
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
@@ -38,12 +25,25 @@
import brooklyn.location.basic.AbstractLocation;
import brooklyn.mementos.LocationMemento;
import brooklyn.util.collections.MutableMap;
-
import com.google.common.base.Objects.ToStringHelper;
+import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.net.HostAndPort;
+import org.slf4j.Logger;
--- End diff --
I'm finding inconsistencies in how Eclipse organises the imports. Configuring IntelliJ to have the same imports list as Eclipse unfortunately doesn't work. As @neykov put it: "One of the IDEs (don't remember which) was interleaving packages not specified in the list, while the other was putting them at the start and then listing the remaining packages."
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25636815
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
@@ -46,6 +47,42 @@
@Beta
public interface PortForwardManager extends Location {
+ @Beta
+ class AssociationMetadata {
+ private final String publicIpId;
+ private final HostAndPort publicEndpoint;
+ private final Location location;
+ private final int privatePort;
+
+ public AssociationMetadata(String publicIpId, HostAndPort publicEndpoint, Location location, int privatePort) {
+ this.publicIpId = publicIpId;
+ this.publicEndpoint = publicEndpoint;
+ this.location = location;
+ this.privatePort = privatePort;
+ }
+
+ public String getPublicIpId() {
+ return publicIpId;
+ }
+
+ public HostAndPort getPublicEndpoint() {
+ return publicEndpoint;
+ }
+
+ public Location getLocation() {
+ return location;
+ }
+
+ public int getPrivatePort() {
+ return privatePort;
+ }
+ }
+
+ @Beta
+ interface AssociationListener {
+ void onEvent(AssociationMetadata metadata);
--- End diff --
Do you think we should call this on removal of a port mapping? I think we probably should.
The reason is that it impacts the choice of method name, so impacts backwards compatibility. How about changing this to `onAssociationCreated(AssociationMetadata)` and `onAssociationDeleted(AssociationMetadata)`?
For the impl of the latter, see `PortForwardManagerImpl.forgetPortMapping(...)` and `forgetPortMappings(...)` - i.e. everywhere that it removes from the `PortForwardManagerImpl.mappings` map.
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25635499
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
@@ -38,12 +25,25 @@
import brooklyn.location.basic.AbstractLocation;
import brooklyn.mementos.LocationMemento;
import brooklyn.util.collections.MutableMap;
-
import com.google.common.base.Objects.ToStringHelper;
+import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.net.HostAndPort;
+import org.slf4j.Logger;
--- End diff --
Try not to let Intellij reorganise the imports. The vast majority of our code uses the Eclipse defaults for imports, so we should stick to that in classes that use it.
You can set the formatting in Intellij to use the Eclipse defaults, but I'm not sure exactly how. Check with @richardcloudsoft ? If you find out, can you please update https://brooklyn.incubator.apache.org/v/latest/dev/env/ide/index.html
But I won't let that block merging of this PR.
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25637180
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
@@ -160,11 +162,22 @@ protected int getNextPort() {
@Override
public void associate(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort) {
associateImpl(publicIpId, publicEndpoint, l, privatePort);
+ emitAssociationEvent(publicIpId, publicEndpoint, l, privatePort);
}
@Override
public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort) {
associateImpl(publicIpId, publicEndpoint, null, privatePort);
+ emitAssociationEvent(publicIpId, publicEndpoint, null, privatePort);
+ }
+
+ private void emitAssociationEvent(String publicIpId, HostAndPort publicEndpoint, Location location, int privatePort) {
+ AssociationMetadata metadata = new AssociationMetadata(publicIpId, publicEndpoint, location, privatePort);
+ for (AssociationListener listener : associationListeners.keySet()) {
+ if (associationListeners.get(listener).apply(metadata)) {
--- End diff --
Looks dangerous in a multi-threaded world. You've separated the call of `keySet()` and `get(listener)`, so you've introduced the chance for another thread to remove the listener in between those two calls.
Better to do the following (thus allowing `ConcurrentHashMap` to give its guarantees):
for (Map.Entry<AssociationListener, Predicate<? super AssociationMetadata> entry : associationListeners.entrySet()) {
if (entry.getValue().apply(metadata)) {
entry.getKey().onEvent(metadata);
}
}
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/538
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#issuecomment-76923371
PR comments addressed
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#issuecomment-76977138
Thanks - 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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#issuecomment-76975103
PR comments addressed again
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25694968
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
@@ -92,6 +144,16 @@
* subsequently be looked up using {@link #lookup(String, int)}.
*/
public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort);
+
+ /**
+ * Registers a listener, which will be notified each time a new port mapping is associated. See {@link #associate(String, HostAndPort, int)}
+ * and {@link #associate(String, HostAndPort, Location, int)}.
+ */
+ @Beta
+ public void addAssociationListener(AssociationListener listener, Predicate<? super AssociationMetadata> filter);
--- End diff --
Could do with a unit test in PortForwardManagerTest, that the `onAssociationCreated` and `onAssociationDeleted` methods get called.
---
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] incubator-brooklyn pull request: Adds addAssociationListener and r...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/538#discussion_r25637263
--- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
@@ -78,7 +78,9 @@
private static final Logger log = LoggerFactory.getLogger(PortForwardManagerImpl.class);
protected final Map<String,PortMapping> mappings = new LinkedHashMap<String,PortMapping>();
-
+
+ private Map<AssociationListener, Predicate<? super AssociationMetadata>> associationListeners = new ConcurrentHashMap<AssociationListener, Predicate<? super AssociationMetadata>>();
--- End diff --
Declare the field `final`. It makes reasoning about the code simpler for folk reading or modifying it later.
---
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.
---