You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2017/06/01 02:08:23 UTC
[GitHub] incubator-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
GitHub user necouchman opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/161
GUACAMOLE-102: Support Connection Load Balancing
Okay, this is a LONG shot, but I've taken a stab at a basic implementation of a weighted load balancing algorithm in the Guacamole client. This is likely to need a lot of work before it's actually ready for merging - I'm acknowledging that up-front :-).
This adds the table column for the connection weight into the DB schema, the necessary JDBC calls to grab, set, and update connection weight, and then the updates to the compare function to sort connections based on a "Weighted Least Connection" algorithm.
The algorithm itself has some nuances that are worth describing. First, I tried to incorporate as much of what was discussed in the JIRA issue as possible. Not setting the weight is acceptable, and will result in the algorithm calculating based on a weight of 1 when no weight is explicitly assigned. I don't know if this is ideal, but kind of necessary to do something like this when doing a WLC calculation. Weights of 0 and negative values cause the connection to be eliminated from the group - the thought is that these values can be used when updating weights externally/dynamically to denote systems that have not been polled yet (0) or that are administratively disabled (-1, etc.). For the purposes of sorting the connections, the 0 and negative values are also replaced with a 1 so that the compare function works as expected, but are those connections will later be bypassed by the acquire() method.
The compare() method now adds one to the number of active connections of both items during comparison in order to avoid a zero value. The rationale behind this is that it allows servers with higher weights and zero connections to be preferred over servers with lower weights and zero connections - without this addition anything with zero connections will end up basically equivalent regardless of its configured weight rather than actually being evaluated by weight.
The overall algorithm was taken from a combination of the LVS wiki page describing how they do WLC and a Citrix article on WLC connections.
This is a pretty simple initial implementation - I have some goals for building upon this in the future, but those are probably better discussed on the JIRA issue and not in this PR.
Fire away...
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-102
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/161.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 #161
----
commit 01d5fe9c106919f6e26f0d52cf23f95fb200d7c8
Author: Nick Couchman <vn...@apache.org>
Date: 2017-03-19T15:57:28Z
GUACAMOLE-102: Initial addition of connection weight to JDBC authentication extension
commit ce9c96ee6873ba5c17df086c56a330124c7728bb
Author: Nick Couchman <vn...@apache.org>
Date: 2017-03-19T20:24:56Z
GUACAMOLE-102: Finish adding connection weight attribute.
commit 0e167b3b0259f9f89b2e58392ab6db8039dc5e14
Author: Nick Couchman <vn...@apache.org>
Date: 2017-03-20T10:49:10Z
GUACAMOLE-102: Initial stab at a WLC algorithm.
commit c854951402762f21ab42a19502703a0386547f2b
Author: Nick Couchman <vn...@apache.org>
Date: 2017-03-20T13:06:04Z
GUACAMOLE-102: Tweak algorithm for computing the WLC vlaues.
commit 7f5a7acc562e272e40ed9541fffc2e0b891bf7cb
Author: Nick Couchman <vn...@apache.org>
Date: 2017-03-20T13:55:41Z
GUACAMOLE-102: Remove some extra debugging code, and continue to tweak the WLC algorithm.
commit a527d398d678858cafec49169cef4e549e5ea238
Author: Nick Couchman <vn...@apache.org>
Date: 2017-03-20T14:50:33Z
GUACAMOLE-102: Continue to work on the load balancing code.
commit 45573730e99e47d064d31b17a401a63644160174
Author: Nick Couchman <vn...@apache.org>
Date: 2017-03-20T18:55:35Z
GUACAMOLE-102: Adding some commentary for changes in the RestrictedGuacamoleTunnelService class.
commit a4ac17b5c7c33ee7c0cd4574a4e342043c9c02a2
Author: Nick Couchman <vn...@apache.org>
Date: 2017-05-31T19:22:31Z
GUACAMOLE-102: Deal with weights of 0, and properly dispose of connections with negative weights.
commit 4b7efcd03a137d68fa686f20ea9c710497748d60
Author: Nick Couchman <vn...@apache.org>
Date: 2017-06-01T00:18:25Z
GUACAMOLE-102: Remove redundant iterations through connections.
commit a58fde75c742439ba5bbca908b1a4ab885abbf32
Author: Nick Couchman <vn...@apache.org>
Date: 2017-06-01T01:39:16Z
GUACAMOLE-102: Clean up and simplify WLC sorting code.
----
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120202144
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,33 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, negative values
+ * indicate that the connection should not be used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an int. If the weight is
+ * null a default weight of 1 is returned. Zero and
+ * negative numbers are used to indicate the system is
+ * unavailable.
+ */
+ public int getConnectionWeight() {
+ if (connectionWeight == null)
+ return 1;
+ return connectionWeight.intValue();
--- End diff --
Right so, to be clear:
1. It makes sense that `getConnectionWeight()` of `ConnectionModel` returns `Integer`. Since this is a model object and the value can indeed be `null`, this actually should be the case.
2. Within `ModeledConnection`, a higher-level object which is expected to abstract away some of the low level detail, it makes sense for `getConnectionWeight()` to return `int`, automatically handling things like that `null` check and the default value.
It's definitely not safe to simply keep this particular function as returning `int` when `connectionWeight` can be `null`, but that's OK because this particular function should be returning `Integer`. It's the `ModeledConnection` version that should handle the other aspects, always returning a sensible `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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120485374
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -126,11 +131,19 @@
));
/**
+ * All attributes related to to load balancing in a logical form.
--- End diff --
"to to"
👗
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120276122
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,28 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, and anything
--- End diff --
It's nice to know that `null` is acceptable, but anyone calling this function will need to know the semantics of specifying that acceptable `null`.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120279468
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/resources/translations/en.json ---
@@ -19,6 +19,7 @@
"FIELD_HEADER_MAX_CONNECTIONS" : "Maximum number of connections:",
"FIELD_HEADER_MAX_CONNECTIONS_PER_USER" : "Maximum number of connections per user:",
+ "FIELD_HEADER_WEIGHT" : "Connection Weight for Load Balancing:",
--- End diff --
For the sake of consistency, this should follow the same style as the other headers (sentence case, not title 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] incubator-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349200
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,28 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, and anything
+ * less than one will prevent the connection from being used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an Integer.
--- End diff --
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120201145
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,8 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ return ((getActiveConnections(a).size() + 1) * b.getConnectionWeight() -
--- End diff --
Okay, so a couple of follow-up questions:
- How much does it actually mess up the math? Since basically all we're doing is sorting the array based on the sign and an integer value, does it actually mess up the calculation enough to even throw off the sorting?
- Is it a lose-lose situation? I'm trying to think of another way to run the calculation so as to account for zero connections but not throw off the math, and nothing is come to mind. Is it simply a case where we have to choose one path or the other, and accept that one of the cases is going to be flawed?
- If there isn't a way to resolve it so that both cases work, any preferences on which one to stick with the win on? I would assume we'd want to ignore the 0 connection case in favor of the other ones being correct (if it matters enough to worry about it), but any reason we'd want 0 to be correct over lower connection counts?
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120146139
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -393,6 +409,19 @@ public GuacamoleProxyConfiguration getGuacamoleProxyConfiguration()
port != null ? port : defaultConfig.getPort(),
encryptionMethod != null ? encryptionMethod : defaultConfig.getEncryptionMethod()
);
+ }
+
+ /**
+ * Returns the weight of the connection, or the default.
+ *
+ * @return
+ * The weight of the connection.
+ *
--- End diff --
Looks like a blank line snuck in here.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120251831
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,31 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, negative values
+ * indicate that the connection should not be used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an int. If the weight is
--- End diff --
I'm guessing this is from the previous version of this function? It currently just returns the connection weight, with the handling of default value, null, etc. being (correctly) elsewhere.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349170
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,28 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, and anything
+ * less than one will prevent the connection from being used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
--- End diff --
Took your suggestion and reworded this.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120507798
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -126,11 +131,19 @@
));
/**
+ * All attributes related to to load balancing in a logical form.
--- End diff --
Fine, since you don't like tu-tus, I've removed it.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120276038
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,28 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, and anything
+ * less than one will prevent the connection from being used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
--- End diff --
"calculating the weighted algorithms" sounds a bit ... odd. You don't calculate algorithms, and it's not immediately clear what "weighted algorithms" refers to when reading this comment alone.
Perhaps "... the connection weight used when applying weighted balancing algorithms"?
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120279344
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,24 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Active connections
+ int Ca = getActiveConnections(a).size();
+ int Cb = getActiveConnections(b).size();
+
+ // Assigned weight
+ int Wa = a.getConnectionWeight();
+ int Wb = b.getConnectionWeight();
+
+ // Net weight of connections
--- End diff --
Is "net weight" the standard term for this part of the calculations for WLC?
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120224307
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,8 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ return ((getActiveConnections(a).size() + 1) * b.getConnectionWeight() -
--- End diff --
> How much does it actually mess up the math? Since basically all we're doing is sorting the array based on the sign and an integer value, does it actually mess up the calculation enough to even throw off the sorting
Quite a bit, actually.
With the `+ 1`, the calculations effectively assume that each server has one more connection than they actually do. Depending on the weights involved, that one extra connection can unbalance things considerably:
Take a look at the 21 connection case above (row 24). Without the `+ 1`, the number of connections allocated to A and B stays around the expected 10:1 ratio... but *with* the `+ 1`, the ratio gets all the way up to 20:1 before things are rebalanced properly.
> Is it a lose-lose situation? I'm trying to think of another way to run the calculation so as to account for zero connections but not throw off the math, and nothing is come to mind. Is it simply a case where we have to choose one path or the other, and accept that one of the cases is going to be flawed?
The only way would be to compare the weights as a means of breaking ties. That way, servers are always ordered based on a correct calculation of their relative load, and a sensible choice is made in the case that each server is under equivalent load.
> If there isn't a way to resolve it so that both cases work, any preferences on which one to stick with the win on? I would assume we'd want to ignore the 0 connection case in favor of the other ones being correct (if it matters enough to worry about it), but any reason we'd want 0 to be correct over lower connection counts?
The one that's actually correct. ;)
In this case, though, it seems we can have it both ways - we just need to make sure the math is sound.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349093
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,28 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, and anything
--- End diff --
Hopefully the updated commentary here looks better. I didn't know exactly how to describe this since it should be coming from the DB - let me know if you're looking for something more specific.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119972090
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +188,15 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Get connection weight for the two systems being compared.
+ int weightA = a.getConnectionWeight().intValue();
--- End diff --
Yeah, that makes sense...and then we don't need all those assignments and can easily fit everything into a single return statement...
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120205724
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,33 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, negative values
+ * indicate that the connection should not be used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an int. If the weight is
+ * null a default weight of 1 is returned. Zero and
+ * negative numbers are used to indicate the system is
+ * unavailable.
+ */
+ public int getConnectionWeight() {
+ if (connectionWeight == null)
+ return 1;
+ return connectionWeight.intValue();
--- End diff --
Got it - should be cleared up with the latest round of updates. Thanks!
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120147205
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/java/org/apache/guacamole/auth/mysql/MySQLEnvironment.java ---
@@ -88,6 +88,12 @@
private int DEFAULT_MAX_CONNECTIONS = 0;
/**
+ * The default value for the connection weight for a connection in
+ * a balancing group.
+ */
+ private int DEFAULT_CONNECTION_WEIGHT = 0;
--- End diff --
Shouldn't the default connection weight be 1?
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120277002
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/PostgreSQLGuacamoleProperties.java ---
@@ -157,6 +157,15 @@ private PostgreSQLGuacamoleProperties() {}
};
+ public static final IntegerGuacamoleProperty
+ POSTGRESQL_DEFAULT_CONNECTION_WEIGHT =
--- End diff --
As with the this property's MySQL cousin, if not used - should remove. If we do want this, then it will need to be documented with a JavaDoc comment like the other one.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119971019
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +188,15 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Get connection weight for the two systems being compared.
+ int weightA = a.getConnectionWeight().intValue();
--- End diff --
Why not return `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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349374
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/PostgreSQLGuacamoleProperties.java ---
@@ -157,6 +157,15 @@ private PostgreSQLGuacamoleProperties() {}
};
+ public static final IntegerGuacamoleProperty
+ POSTGRESQL_DEFAULT_CONNECTION_WEIGHT =
--- End diff --
Oops...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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120251461
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,8 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ return ((getActiveConnections(a).size() + 1) * b.getConnectionWeight() -
--- End diff --
Yep - it looks much better now.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120190659
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -393,6 +409,19 @@ public GuacamoleProxyConfiguration getGuacamoleProxyConfiguration()
port != null ? port : defaultConfig.getPort(),
encryptionMethod != null ? encryptionMethod : defaultConfig.getEncryptionMethod()
);
+ }
+
+ /**
+ * Returns the weight of the connection, or the default.
--- End diff --
Cleaned up.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120352670
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -117,12 +117,18 @@
public static final String MAX_CONNECTIONS_PER_USER_NAME = "max-connections-per-user";
/**
+ * The connection weight for weighted algorithms.
+ */
+ public static final String CONNECTION_WEIGHT = "weight";
+
+ /**
* All attributes related to restricting user accounts, within a logical
* form.
*/
public static final Form CONCURRENCY_LIMITS = new Form("concurrency", Arrays.<Field>asList(
new NumericField(MAX_CONNECTIONS_NAME),
- new NumericField(MAX_CONNECTIONS_PER_USER_NAME)
+ new NumericField(MAX_CONNECTIONS_PER_USER_NAME),
+ new NumericField(CONNECTION_WEIGHT)
--- End diff --
That's fine with me - I was trying to avoid creating too many section breaks, especially for sections with a single item in them, and it seemed like this still fell under the heading of "Concurrency Limits", since it is used to impact how connections are concurrently applied to systems, but I'll break it out into another section.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120146889
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -193,6 +200,12 @@ public int compare(ModeledConnection a, ModeledConnection b) {
// Return the first unreserved connection
for (ModeledConnection connection : sortedConnections) {
+ // If connection weight is zero or negative, this host is disabled and should not be used.
+ if (connection.getConnectionWeight() < 1) {
+ logger.warn("Weight for {} is < 1, connection will be skipped.", connection.getName());
--- End diff --
Perhaps this should instead be logged at the debug level?
If zero and negative weights are intended mechanisms for disabling connection, logging this at the warning level will result in the logs being filled with messages warning the admin that they're using a feature as intended.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120038076
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +188,15 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Get connection weight for the two systems being compared.
+ int weightA = a.getConnectionWeight().intValue();
+ int weightB = b.getConnectionWeight().intValue();
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
--- End diff --
Actually ... doesn't adding 1 to both upset the intended property that a server with weight N will be given N times as many connections as a server with weight 1?
You'd expect a server with weight 4 to be given twice as many connections as a server with weight 2, but this would result in a server with weight 4 receiving 1.6666... times as many connections as a server with weight 2.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120145222
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,33 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, negative values
+ * indicate that the connection should not be used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an int. If the weight is
+ * null a default weight of 1 is returned. Zero and
+ * negative numbers are used to indicate the system is
+ * unavailable.
+ */
+ public int getConnectionWeight() {
+ if (connectionWeight == null)
+ return 1;
+ return connectionWeight.intValue();
--- End diff --
Java will do this automatically, by the way. If you have something like:
Integer foo = 2; // Automatically wrapped within Integer by the Java compiler
int bar = foo; // Automatically unwrapped by the Java compiler
things happen magically thanks to autoboxing / autounboxing: https://docs.oracle.com/javase/tutorial/java/data/autoboxing.html
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120190480
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/java/org/apache/guacamole/auth/mysql/MySQLEnvironment.java ---
@@ -88,6 +88,12 @@
private int DEFAULT_MAX_CONNECTIONS = 0;
/**
+ * The default value for the connection weight for a connection in
+ * a balancing group.
+ */
+ private int DEFAULT_CONNECTION_WEIGHT = 0;
--- End diff --
Removed this completely - the DB-specific default connection weight was not being used at all.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120277394
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/java/org/apache/guacamole/auth/mysql/MySQLGuacamoleProperties.java ---
@@ -175,6 +175,18 @@ private MySQLGuacamoleProperties() {}
};
/**
+ * The connection weight for connections in balancing groups.
--- End diff --
This is the default connection weight, not simply the connection weight.
That said, I don't see this property used anywhere. Perhaps it should simply be removed? Is there a use case where the default weight of 1 would need to be overridden?
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119914434
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -179,9 +187,27 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
-
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+
+ int weightA, weightB;
+ // Check if weight of a is non-null and retrieve it.
+ if (a.getConnectionWeight() != null && a.getConnectionWeight().intValue() > 0)
+ weightA = a.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightA = 1;
+
+ // Check if weight of b is null, assign 1 if it is.
+ if (b.getConnectionWeight() != null && b.getConnectionWeight().intValue() > 0)
+ weightB = b.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightB = 1;
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
+ int connsA = getActiveConnections(a).size() + 1;
+ int connsB = getActiveConnections(b).size() + 1;
+
+ return (connsA * 10000 / weightA) - (connsB * 10000 / weightB);
--- End diff --
There's no significance to the 10000 itself, that number is arbitrary. The overall idea, though, is to get the values high enough that the final calculation returns an integer. Because the number of connections is going to be a relatively low number (between 1 and 100, maybe 1000), and the weights can be relatively high (100, 1000, 10000, etc.), the idea is to get the numerator sufficiently high so that an integer calculation will actually happen.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119914637
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -179,9 +187,27 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
-
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+
+ int weightA, weightB;
+ // Check if weight of a is non-null and retrieve it.
+ if (a.getConnectionWeight() != null && a.getConnectionWeight().intValue() > 0)
--- End diff --
Checking for `null`, using a default value, etc. seems the sort of logic that should be within `ModeledConnection`, rather than repeated here. That way you could simply call `a.getWeight()` or `a.getConnectionWeight()` and it will always just work?
Only the underlying model object (and the code applying attribute changes) need actually worry about such internal concerns.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120185733
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,33 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, negative values
+ * indicate that the connection should not be used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an int. If the weight is
+ * null a default weight of 1 is returned. Zero and
+ * negative numbers are used to indicate the system is
+ * unavailable.
+ */
+ public int getConnectionWeight() {
+ if (connectionWeight == null)
+ return 1;
+ return connectionWeight.intValue();
--- End diff --
I think I ran into an issue with the automatic (un)boxing stuff and went that route, but it seems to be fine, now, so I've cleaned that up. Also moved the null check/default code to the ModeledConnection class.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120352180
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/resources/translations/en.json ---
@@ -19,6 +19,7 @@
"FIELD_HEADER_MAX_CONNECTIONS" : "Maximum number of connections:",
"FIELD_HEADER_MAX_CONNECTIONS_PER_USER" : "Maximum number of connections per user:",
+ "FIELD_HEADER_WEIGHT" : "Connection Weight for Load Balancing:",
--- End diff --
Should be cleaned up - also added a section for the soon-to-be added form...
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120346221
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,24 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Active connections
+ int Ca = getActiveConnections(a).size();
--- End diff --
Both the lack of camel case and the "Net Weight" came from referencing the following pages:
http://kb.linuxvirtualserver.org/wiki/Weighted_Least-Connection_Scheduling
https://www.citrix.com/blogs/2010/10/01/load-balancing-weights/
https://www.citrix.com/blogs/2010/09/02/load-balancing-least-connections/
(as did the initial, and now-removed, concept of multiplying by the arbitrary 10K number). Anyway, I don't know that "Net Weight" is actually called out in those, but the Nw abbreviation is used in the Citrix page. I can go with something else - computed weight or calculated weight - if that makes more sense.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119970989
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +188,15 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Get connection weight for the two systems being compared.
+ int weightA = a.getConnectionWeight().intValue();
+ int weightB = b.getConnectionWeight().intValue();
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
--- End diff --
Why? What do you have against 0? ;)
Especially considering that you:
> ... removed the checking for > 0. Since anything < 1 gets eliminated from consideration later on, it doesn't really matter how those connections sort, so there isn't much point in checking.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120190510
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/schema/upgrade/upgrade-pre-0.9.12.sql ---
@@ -0,0 +1,25 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+--
+
+--
+-- Add per-user password set date
+--
+
+ALTER TABLE guacamole_connection
+ ADD COLUMN connection_weight int(11);
--- End diff --
Fixed.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120278409
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,28 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, and anything
+ * less than one will prevent the connection from being used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an Integer.
--- End diff --
To avoid confusion, I suggest removing "as an Integer" here, as that wording implies that the connection weight is a different type under other circumstances. If the intent is to document the return type, that's already inherently documented by the function prototype.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120145721
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,33 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, negative values
+ * indicate that the connection should not be used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an int. If the weight is
+ * null a default weight of 1 is returned. Zero and
+ * negative numbers are used to indicate the system is
+ * unavailable.
+ */
+ public int getConnectionWeight() {
+ if (connectionWeight == null)
+ return 1;
+ return connectionWeight.intValue();
--- End diff --
Shouldn't this logic be within `ModeledConnection`? The model object itself should be relatively logic-free, being the pure (ish) intermediary between the database tables and Java.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120251692
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -55,6 +55,13 @@
private Integer maxConnectionsPerUser;
/**
+ * The weight of the connection for the purposes of calculating
+ * WRR algorithm. null indicates nothing has been set, -1 indicates
--- End diff --
WLC, not WRR, right?
Is it -1 that is intended to be the documented way to indicate the system is unavailable, or zero?
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349504
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,24 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Active connections
+ int Ca = getActiveConnections(a).size();
--- End diff --
Changed the variable names to be more consistent with standards.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120190547
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -193,6 +200,12 @@ public int compare(ModeledConnection a, ModeledConnection b) {
// Return the first unreserved connection
for (ModeledConnection connection : sortedConnections) {
+ // If connection weight is zero or negative, this host is disabled and should not be used.
+ if (connection.getConnectionWeight() < 1) {
+ logger.warn("Weight for {} is < 1, connection will be skipped.", connection.getName());
--- End diff --
Changed to debug.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120277753
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -393,6 +409,21 @@ public GuacamoleProxyConfiguration getGuacamoleProxyConfiguration()
port != null ? port : defaultConfig.getPort(),
encryptionMethod != null ? encryptionMethod : defaultConfig.getEncryptionMethod()
);
+ }
+
+ /**
+ * Returns the weight of the connection, or a default
--- End diff --
Assuming the wording of the similar functions in `ConnectionModel` can be clarified, I would suggest doing the same here, such that it's clear what the "weight" of a connection means to someone looking at this function.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120279970
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -117,12 +117,18 @@
public static final String MAX_CONNECTIONS_PER_USER_NAME = "max-connections-per-user";
/**
+ * The connection weight for weighted algorithms.
+ */
+ public static final String CONNECTION_WEIGHT = "weight";
+
+ /**
* All attributes related to restricting user accounts, within a logical
* form.
*/
public static final Form CONCURRENCY_LIMITS = new Form("concurrency", Arrays.<Field>asList(
new NumericField(MAX_CONNECTIONS_NAME),
- new NumericField(MAX_CONNECTIONS_PER_USER_NAME)
+ new NumericField(MAX_CONNECTIONS_PER_USER_NAME),
+ new NumericField(CONNECTION_WEIGHT)
--- End diff --
Perhaps this should go in a different `Form`? As is, it will be rendered as one of several fields in a group labeled "Concurrency Limits", which is not what this relates to.
This would also help clean up the wording of the field itself, currently "Connection Weight for Load Balancing", as locating the field within a section titled "Load Balancing" (or similar) would remove the need to further qualify "Connection Weight" with the phrase "for Load Balancing".
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349255
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -117,12 +117,18 @@
public static final String MAX_CONNECTIONS_PER_USER_NAME = "max-connections-per-user";
/**
+ * The connection weight for weighted algorithms.
--- End diff --
Cleaned this up.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120037634
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +188,15 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Get connection weight for the two systems being compared.
+ int weightA = a.getConnectionWeight().intValue();
+ int weightB = b.getConnectionWeight().intValue();
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
--- End diff --
Nope - sorting connections with zero active connections by their weight makes sense, but if that's what you're aiming for here, then perhaps that's what the comment should describe?
The thing that wouldn't make sense is arbitrarily avoiding zero.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120146564
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,8 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ return ((getActiveConnections(a).size() + 1) * b.getConnectionWeight() -
--- End diff --
Repeating from an older comment for convenience: though that `+ 1` does result in the connection weight having an effect when the number of connections is zero, it also offsets the math such that the proportions governing weights in WLC are wrong, particularly for smaller numbers of connections.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119922302
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -179,9 +187,27 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
-
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+
+ int weightA, weightB;
+ // Check if weight of a is non-null and retrieve it.
+ if (a.getConnectionWeight() != null && a.getConnectionWeight().intValue() > 0)
+ weightA = a.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightA = 1;
+
+ // Check if weight of b is null, assign 1 if it is.
+ if (b.getConnectionWeight() != null && b.getConnectionWeight().intValue() > 0)
+ weightB = b.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightB = 1;
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
+ int connsA = getActiveConnections(a).size() + 1;
+ int connsB = getActiveConnections(b).size() + 1;
+
+ return (connsA * 10000 / weightA) - (connsB * 10000 / weightB);
--- End diff --
> ... the idea is to get the numerator sufficiently high so that an integer calculation will actually happen.
Ick. =/
With normal WLC, the idea is that a server with weight N should be allocated N times as many connections as a server with weight 1, correct? That way weights can be calculated based on the proportion of resources available on each server relative to some arbitrary baseline?
I agree that multiplying each of the compared values by a constant will not alter the above relationship, but I'm not a fan of the arbitrariness of that constant here.
If that constant were instead `weightB * weightA`, this would become:
return (connsA * weightB * weightA / weightA) - (connsB * weightB * weightA / weightB);
which clearly reduces to:
return (connsA * weightB) - (connsB * weightA);
which is nice and integery. You won't get the same values, *but* you will get the same sign, which is all that matters here.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119971562
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +188,15 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Get connection weight for the two systems being compared.
+ int weightA = a.getConnectionWeight().intValue();
+ int weightB = b.getConnectionWeight().intValue();
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
--- End diff --
It seems like this might actually matter for number of connections. If you have connections A, B, and C, with weights 1, 2, and 3 respectively, and no connections to any of them, then, ideally, with a weighted algorithm, you'd like C to get the first connection because it has the larger weight. If the number of active connections is 0, then the net weight for a host evaluates to zero, which messes up this balancing case a little bit. It may not be the end of the world, but it's less than ideal. By adding one to both of the connections being compared, connections with 0 active connections will be "correctly" (or, maybe, more ideally?) evaluated for their weight.
The reason that I removed the check for the weight calculation is that items with a weight < 1 are removed, anyway, so the sorting for those items doesn't matter, at all. This isn't true for active connections, obviously, so it seems like adding one here makes the weight behave as expected when a connection has zero active connections.
Or am I worrying too much about a corner 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] incubator-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120253273
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -55,6 +55,13 @@
private Integer maxConnectionsPerUser;
/**
+ * The weight of the connection for the purposes of calculating
+ * WRR algorithm. null indicates nothing has been set, -1 indicates
--- End diff --
Yep...replaced a few instances of that, already, but missed this one. Should be fixed.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/161
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349335
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -393,6 +409,21 @@ public GuacamoleProxyConfiguration getGuacamoleProxyConfiguration()
port != null ? port : defaultConfig.getPort(),
encryptionMethod != null ? encryptionMethod : defaultConfig.getEncryptionMethod()
);
+ }
+
+ /**
+ * Returns the weight of the connection, or a default
--- End diff --
Clarified.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120354957
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -117,12 +117,18 @@
public static final String MAX_CONNECTIONS_PER_USER_NAME = "max-connections-per-user";
/**
+ * The connection weight for weighted algorithms.
+ */
+ public static final String CONNECTION_WEIGHT = "weight";
+
+ /**
* All attributes related to restricting user accounts, within a logical
* form.
*/
public static final Form CONCURRENCY_LIMITS = new Form("concurrency", Arrays.<Field>asList(
new NumericField(MAX_CONNECTIONS_NAME),
- new NumericField(MAX_CONNECTIONS_PER_USER_NAME)
+ new NumericField(MAX_CONNECTIONS_PER_USER_NAME),
+ new NumericField(CONNECTION_WEIGHT)
--- End diff --
Moved to a separate section/form.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120276406
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -117,12 +117,18 @@
public static final String MAX_CONNECTIONS_PER_USER_NAME = "max-connections-per-user";
/**
+ * The connection weight for weighted algorithms.
--- End diff --
Again, "weighted algorithms" is vague out of context. I would suggest wording things such that it's clear we're referring to weighted connection balancing algorithms.
This is also not the connection weight, but rather the name of the attribute which specified the connection weight.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120253317
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionModel.java ---
@@ -165,6 +172,31 @@ public Integer getMaxConnectionsPerUser() {
}
/**
+ * Sets the connection weight.
+ *
+ * @param connectionWeight
+ * The weight of the connection. null is acceptable, negative values
+ * indicate that the connection should not be used.
+ */
+ public void setConnectionWeight(Integer connectionWeight) {
+ this.connectionWeight = connectionWeight;
+ }
+
+ /**
+ * Returns the connection weight used in calculating the
+ * weighted algorithms.
+ *
+ * @return
+ * The connection weight as an int. If the weight is
--- End diff --
Yes, removed most of that 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] incubator-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119970885
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -179,9 +187,27 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
-
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+
+ int weightA, weightB;
+ // Check if weight of a is non-null and retrieve it.
+ if (a.getConnectionWeight() != null && a.getConnectionWeight().intValue() > 0)
+ weightA = a.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightA = 1;
+
+ // Check if weight of b is null, assign 1 if it is.
+ if (b.getConnectionWeight() != null && b.getConnectionWeight().intValue() > 0)
+ weightB = b.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightB = 1;
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
+ int connsA = getActiveConnections(a).size() + 1;
+ int connsB = getActiveConnections(b).size() + 1;
+
+ return (connsA * 10000 / weightA) - (connsB * 10000 / weightB);
--- End diff --
Ran through a few test cases with the updated code and that calculation seems to work well, so I've pushed the changes.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119925359
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -179,9 +187,27 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
-
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+
+ int weightA, weightB;
+ // Check if weight of a is non-null and retrieve it.
+ if (a.getConnectionWeight() != null && a.getConnectionWeight().intValue() > 0)
+ weightA = a.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightA = 1;
+
+ // Check if weight of b is null, assign 1 if it is.
+ if (b.getConnectionWeight() != null && b.getConnectionWeight().intValue() > 0)
+ weightB = b.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightB = 1;
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
+ int connsA = getActiveConnections(a).size() + 1;
+ int connsB = getActiveConnections(b).size() + 1;
+
+ return (connsA * 10000 / weightA) - (connsB * 10000 / weightB);
--- End diff --
Yes, clearly not the best code ever written.
> With normal WLC, the idea is that a server with weight N should be allocated N times as many connections as a server with weight 1, correct?
Well, I'd say that the idea of weight in general is as you say - but the WLC also factors in the number of current connections. There's also WRR, which accounts for the last time a system was used for a connection combined with weight. But, yes, that's the overall idea...
> return (connsA * weightB) - (connsB * weightA);
Works for me, I'll update the code. I agree that the actual value that is returned or computed does not matter - the sign and the ability to get a meaningful integer value are really all that I was going for, and that seems to solve it more elegantly :-). I'll update the code, test a few times and commit the changes.
Thanks for the help!
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120278905
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,24 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Active connections
+ int Ca = getActiveConnections(a).size();
--- End diff --
Any reason the variables here don't follow the `headlessCamelCase` convention used by the other Java code?
Depending on why you went this route, I'm not sure this needs to be changed, but this function is slightly hard to read with how similar all the variables look.
Anyway, not necessarily an issue worth further iterations, but I'm interested to hear what you were thinking at the time and what your thoughts are now.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120248638
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,8 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ return ((getActiveConnections(a).size() + 1) * b.getConnectionWeight() -
--- End diff --
Okay, that makes sense. So I guess what we're going for here is keeping the ratio under the specified limit - that is, if 10:1 is the configured ratio (based on configured weights of the servers), then work up to that, then drop back down. It's still, mathematically, unbalanced (you end up with a 5.5:1 ratio), but it keeps it "in bounds" so to speak.
I've tweaked the code such that it computes the net weight of each of the connections, and, if they're equal, returns the difference between the configured weight of each system, otherwise returns the difference in the net weight. Does that look more like what we're going for in a WLC algorithm?
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119970741
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -179,9 +187,27 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
-
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+
+ int weightA, weightB;
+ // Check if weight of a is non-null and retrieve it.
+ if (a.getConnectionWeight() != null && a.getConnectionWeight().intValue() > 0)
--- End diff --
Put the null check in the getConnectionWeight method, and then removed the checking for > 0. Since anything < 1 gets eliminated from consideration later on, it doesn't really matter how those connections sort, so there isn't much point in checking.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r119913776
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -179,9 +187,27 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
-
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+
+ int weightA, weightB;
+ // Check if weight of a is non-null and retrieve it.
+ if (a.getConnectionWeight() != null && a.getConnectionWeight().intValue() > 0)
+ weightA = a.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightA = 1;
+
+ // Check if weight of b is null, assign 1 if it is.
+ if (b.getConnectionWeight() != null && b.getConnectionWeight().intValue() > 0)
+ weightB = b.getConnectionWeight().intValue();
+ // In all other cases assign 1 for sorting.
+ else
+ weightB = 1;
+
+ // Get current active connections, add 1 to both to avoid calculations with 0.
+ int connsA = getActiveConnections(a).size() + 1;
+ int connsB = getActiveConnections(b).size() + 1;
+
+ return (connsA * 10000 / weightA) - (connsB * 10000 / weightB);
--- End diff --
What's the significance of 10000 here?
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349409
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/java/org/apache/guacamole/auth/mysql/MySQLGuacamoleProperties.java ---
@@ -175,6 +175,18 @@ private MySQLGuacamoleProperties() {}
};
/**
+ * The connection weight for connections in balancing groups.
--- End diff --
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120145949
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -393,6 +409,19 @@ public GuacamoleProxyConfiguration getGuacamoleProxyConfiguration()
port != null ? port : defaultConfig.getPort(),
encryptionMethod != null ? encryptionMethod : defaultConfig.getEncryptionMethod()
);
+ }
+
+ /**
+ * Returns the weight of the connection, or the default.
--- End diff --
"or the default" is unclear here.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120190604
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ModeledConnection.java ---
@@ -393,6 +409,19 @@ public GuacamoleProxyConfiguration getGuacamoleProxyConfiguration()
port != null ? port : defaultConfig.getPort(),
encryptionMethod != null ? encryptionMethod : defaultConfig.getEncryptionMethod()
);
+ }
+
+ /**
+ * Returns the weight of the connection, or the default.
+ *
+ * @return
+ * The weight of the connection.
+ *
--- End diff --
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120147081
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/schema/upgrade/upgrade-pre-0.9.12.sql ---
@@ -0,0 +1,25 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+--
+
+--
+-- Add per-user password set date
+--
+
+ALTER TABLE guacamole_connection
+ ADD COLUMN connection_weight int(11);
--- End diff --
With 0.9.12 already out, and 0.9.13 pending, this should be `upgrade-pre-0.9.14.sql`.
---
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-guacamole-client pull request #161: GUACAMOLE-102: Support Connect...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/161#discussion_r120349586
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/RestrictedGuacamoleTunnelService.java ---
@@ -180,8 +187,24 @@ protected ModeledConnection acquire(RemoteAuthenticatedUser user,
@Override
public int compare(ModeledConnection a, ModeledConnection b) {
- return getActiveConnections(a).size()
- - getActiveConnections(b).size();
+ // Active connections
+ int Ca = getActiveConnections(a).size();
+ int Cb = getActiveConnections(b).size();
+
+ // Assigned weight
+ int Wa = a.getConnectionWeight();
+ int Wb = b.getConnectionWeight();
+
+ // Net weight of connections
--- End diff --
Re-worded this.
---
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.
---