brooklyn-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aleds...@apache.org
Subject [3/5] incubator-brooklyn git commit: PortForwardManager: incorporate review comments
Date Fri, 28 Nov 2014 18:23:29 GMT
PortForwardManager: incorporate review comments


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/d2f96a21
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/d2f96a21
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/d2f96a21

Branch: refs/heads/master
Commit: d2f96a21dbc7b8e6afbe859296e1dc2588bbde3b
Parents: 4a41504
Author: Aled Sage <aled.sage@gmail.com>
Authored: Fri Nov 28 12:18:57 2014 +0000
Committer: Aled Sage <aled.sage@gmail.com>
Committed: Fri Nov 28 12:20:31 2014 +0000

----------------------------------------------------------------------
 .../location/access/PortForwardManager.java     | 18 +++---
 .../location/access/PortForwardManagerImpl.java | 60 +++++++++-----------
 .../PortForwardManagerLocationResolverTest.java |  2 +-
 3 files changed, 39 insertions(+), 41 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d2f96a21/core/src/main/java/brooklyn/location/access/PortForwardManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManager.java b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
index 40f76c5..e80c28a 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManager.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
@@ -41,13 +41,17 @@ import com.google.common.net.HostAndPort;
  * To use, see {@link PortForwardManagerLocationResolver}, with code such as 
  * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
  * 
- * Implementations typically will not know anything about what the firewall/IP actually is,
they just handle a
- * unique identifier for it. It is recommended, however, to {@link #recordPublicIpHostname(String,
String)} an
- * accessible hostname with the identifier. This is required in order to use {@link #lookup(Location,
int)}.
+ * @see PortForwardManagerImpl for implementation notes and considerations.
  */
 @Beta
 public interface PortForwardManager extends Location {
 
+    /**
+     * The intention is that there is one PortForwardManager instance per "scope". If you

+     * use global, then it will be a shared instance (for that management context). If you

+     * pass in your own name (e.g. "docker-fjie3") then it will shared with just any other
+     * places that use that same location spec (e.g. {@code portForwardManager(scope=docker-fjie3)}).
+     */
     public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
             "scope",
             "The scope that this applies to, defaulting to global",
@@ -80,7 +84,7 @@ public interface PortForwardManager extends Location {
     public void associate(String publicIpId, HostAndPort publicEndpoint, Location l, int
privatePort);
 
     /**
-     * Records a mapping for pubilcIpId:privatePort to a public endpoint, such that it can
+     * Records a mapping for publicIpId:privatePort to a public endpoint, such that it can
      * subsequently be looked up using {@link #lookup(String, int)}.
      */
     public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort);
@@ -111,7 +115,7 @@ public interface PortForwardManager extends Location {
     public HostAndPort lookup(String publicIpId, int privatePort);
 
     /** 
-     * Clears the given port mapping, returning the true if there was a match.
+     * Clears the given port mapping, returning true if there was a match.
      */
     public boolean forgetPortMapping(String publicIpId, int publicPort);
     
@@ -180,7 +184,7 @@ public interface PortForwardManager extends Location {
     /**
      * Clears a previous call to {@link #recordPublicIpHostname(String, String)}.
      * 
-     * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMapping(Location,
int)}
+     * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMappings(Location)}
      */
     @Deprecated
     public boolean forgetPublicIpHostname(String publicIpId);
@@ -216,7 +220,7 @@ public interface PortForwardManager extends Location {
     public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId);
 
     /** 
-     * @see #forgetPortMapping(String, int)
+     * @see {@link #forgetPortMapping(String, int)} and {@link #forgetPortMappings(Location)}
      * 
      * @deprecated since 0.7.0; this method will be internal only
      */

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d2f96a21/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
index 5b8c072..21d7b03 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
@@ -48,44 +48,38 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 import com.google.common.net.HostAndPort;
 
+/**
+ * 
+ * @author aled
+ *
+ * TODO This implementation is not efficient, and currently has a cap of about 50000 rules.
+ * Need to improve the efficiency and scale.
+ * A quick win could be to use a different portReserved counter for each publicIpId,
+ * when calling acquirePublicPort?
+ * 
+ * TODO Callers need to be more careful in acquirePublicPort for which ports are actually
in use.
+ * If multiple apps sharing the same public-ip (e.g. in the same vcloud-director vOrg) then
they 
+ * must not allocate the same public port (e.g. ensure they share the same PortForwardManager
+ * by using the same scope in 
+ * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
+ * However, this still doesn't check if the port is *actually* available. For example, if
a
+ * different Brooklyn instance is also deploying there then we can get port conflicts, or
if 
+ * some ports in that range are already in use (e.g. due to earlier dev/test runs) then this
+ * will not be respected. Callers should probably figure out the port number themselves,
but
+ * that also leads to concurrency issues.
+ * 
+ * TODO The publicIpId means different things to different callers:
+ * <ul>
+ *   <li> In acquirePublicPort() it is (often?) an identifier of the actual public
ip.
+ *   <li> In later calls to associate(), it is (often?) an identifier for the target
machine
+ *        such as the jcloudsMachine.getJcloudsId().
+ * </ul>
+ */
 @SuppressWarnings("serial")
 public class PortForwardManagerImpl extends AbstractLocation implements PortForwardManager
{
 
-    // TODO This implementation is not efficient, and currently has a cap of about 50000
rules.
-    // Need to improve the efficiency and scale.
-    // A quick win could be to use a different portReserved counter for each publicIpId,
-    // when calling acquirePublicPort?
-    
-    // TODO Callers need to be more careful in acquirePublicPort for which ports are actually
in use.
-    // If multiple apps sharing the same public-ip (e.g. in the same vcloud-director vOrg)
then they 
-    // must not allocate the same public port (so can share the same PortForwardManagerAuthority
-    // via PortForwardManager.Factory.sharedInstance).
-    // However, this still doesn't check if the port is *actually* available. For example,
if a
-    // different Brooklyn instance is also deploying there then we can get port conflicts,
or if 
-    // some ports in that range are already in use (e.g. due to earlier dev/test runs) then
this
-    // will not be respected. Callers should probably figure out the port number themselves,
but
-    // that also leads to concurrency issues.
-
-    // TODO The publicIpId means different things to different callers:
-    //  - In acquirePublicPort() it is (often?) an identifier of the actual public ip.
-    //  - In later calls to associate(), it is (often?) an identifier for the target machine
-    //    such as the jcloudsMachine.getJcloudsId().
-    
-    // TODO Should PortForwardManager be changed to extend AbstractLocation, or is this wrapper
ok?
-    
     private static final Logger log = LoggerFactory.getLogger(PortForwardManagerImpl.class);
     
-    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
-            "scope",
-            "The scope that this applies to, defaulting to global",
-            "global");
-
-    @Beta
-    public static final ConfigKey<Integer> PORT_FORWARD_MANAGER_STARTING_PORT = ConfigKeys.newIntegerConfigKey(
-            "brooklyn.portForwardManager.startingPort",
-            "The starting port for assigning port numbers, such as for DNAT",
-            11000);
-    
     protected final Map<String,PortMapping> mappings = new LinkedHashMap<String,PortMapping>();
     
     @Deprecated

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d2f96a21/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
b/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
index cc2677a..a699bbc 100644
--- a/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
+++ b/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
@@ -75,7 +75,7 @@ public class PortForwardManagerLocationResolverTest {
     private void assertNotSame(Location loc1, Location loc2) {
         Assert.assertNotNull(loc1);
         Assert.assertNotNull(loc2);
-        Assert.assertTrue(loc2 instanceof PortForwardManager, "loc1="+loc1);
+        Assert.assertTrue(loc1 instanceof PortForwardManager, "loc1="+loc1);
         Assert.assertTrue(loc2 instanceof PortForwardManager, "loc2="+loc2);
         Assert.assertNotSame(loc1, loc2);
         Assert.assertNotEquals(((PortForwardManager)loc1).getId(), ((PortForwardManager)loc2).getId());


Mime
View raw message