You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by al...@apache.org on 2014/11/28 19:23:29 UTC
[3/5] incubator-brooklyn git commit: PortForwardManager: incorporate
review comments
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 <al...@gmail.com>
Authored: Fri Nov 28 12:18:57 2014 +0000
Committer: Aled Sage <al...@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());