smolnar82 commented on a change in pull request #469:
URL: https://github.com/apache/knox/pull/469#discussion_r670198311
##########
File path: gateway-provider-ha/pom.xml
##########
@@ -105,6 +105,12 @@
<artifactId>javax.servlet-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
+ <scope>compile</scope>
Review comment:
No need for the `compile` scope; that's the default.
##########
File path:
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/provider/impl/HaDescriptorFactory.java
##########
@@ -31,19 +31,81 @@ public static HaDescriptor createDescriptor() {
public static HaServiceConfig createServiceConfig(String serviceName,
String config) {
Map<String, String> configMap = parseHaConfiguration(config);
- String enabledValue = configMap.get(CONFIG_PARAM_ENABLED);
- String maxFailoverAttempts =
configMap.get(CONFIG_PARAM_MAX_FAILOVER_ATTEMPTS);
- String failoverSleep = configMap.get(CONFIG_PARAM_FAILOVER_SLEEP);
- String zookeeperEnsemble =
configMap.get(CONFIG_PARAM_ZOOKEEPER_ENSEMBLE);
- String zookeeperNamespace =
configMap.get(CONFIG_PARAM_ZOOKEEPER_NAMESPACE);
- String stickySessionEnabled =
configMap.get(CONFIG_STICKY_SESSIONS_ENABLED);
- String loadBalancingEnabled =
configMap.get(CONFIG_LOAD_BALANCING_ENABLED);
- String stickySessionCookieName =
configMap.get(STICKY_SESSION_COOKIE_NAME);
- String noFallbackEnabled = configMap.get(CONFIG_NO_FALLBACK_ENABLED);
- return createServiceConfig(serviceName, enabledValue,
maxFailoverAttempts, failoverSleep,
- zookeeperEnsemble, zookeeperNamespace, loadBalancingEnabled,
stickySessionEnabled, stickySessionCookieName, noFallbackEnabled);
+ boolean enabled = DEFAULT_ENABLED;
+ int maxFailoverAttempts = DEFAULT_MAX_FAILOVER_ATTEMPTS;
+ int failoverSleep = DEFAULT_FAILOVER_SLEEP;
+ boolean stickySessionsEnabled = DEFAULT_STICKY_SESSIONS_ENABLED;
+ boolean loadBalancingEnabled = DEFAULT_LOAD_BALANCING_ENABLED;
+ boolean noFallbackEnabled = DEFAULT_NO_FALLBACK_ENABLED;
+ String stickySessionCookieName = DEFAULT_STICKY_SESSION_COOKIE_NAME;
+ String disableStickySessionForUserAgents =
DEFAULT_DISABLE_LB_USER_AGENTS;
+
+ String enabledValue = configMap.get(CONFIG_PARAM_ENABLED);
+ String maxFailoverAttemptsValue =
configMap.get(CONFIG_PARAM_MAX_FAILOVER_ATTEMPTS);
+ String failoverSleepValue = configMap.get(CONFIG_PARAM_FAILOVER_SLEEP);
Review comment:
You may use `Map.getOrDefault(...)` to save the above default settings.
Even better, if I were you I'd add some private methods like that:
```
getBooleanValue(Map configMap, String configName, boolean default) {
final configValue = configMap.get(configName);
return StringUtils.isNotBlank(configValue) ?
Boolean.parseBoolean(configValue) : default;
}
```
Then you could initialize your local variables like this:
```
final boolean enabled = getBooleanValue(configMap, CONFIG_PARAM_ENABLED,
DEFAULT_ENABLED);
final int maxFailoverAttempts = getIntValue(configMap,
CONFIG_PARAM_MAX_FAILOVER_ATTEMPTS, DEFAULT_MAX_FAILOVER_ATTEMPTS);
...
```
##########
File path:
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/ConfigurableHADispatch.java
##########
@@ -89,6 +91,13 @@ public void init() {
if(stickySessionsEnabled) {
stickySessionCookieName = serviceConfig.getStickySessionCookieName();
}
+ /* if a configurable user agent list is defined to turn off Knox LB used
it */
+
if(!StringUtils.isBlank(serviceConfig.getDisableStickySessionForUserAgents())) {
+ /* configured user agents are comma separate list which *can* contain
whitespaces */
+ disableLoadBalancingForUserAgents =
Arrays.asList(serviceConfig.getDisableStickySessionForUserAgents()
Review comment:
IMO, `HaServiceConfig.getDisableStickySessionForUserAgents()` 's return
type should be `List<String>` and this logic should be implemented there.
##########
File path:
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/i18n/HaDispatchMessages.java
##########
@@ -44,4 +44,7 @@
@Message(level = MessageLevel.ERROR, text = "noFallback flag is turned on
for sticky session so aborting request without retrying")
void noFallbackError();
+
+ @Message(level = MessageLevel.INFO, text = "Knox HA loadbalancing disabled,
detected user agent user-agent: {0}, configured user-agents to disable
loadbalancing: {1}")
Review comment:
`user agent user-agent` seems odd to me.
##########
File path:
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/ConfigurableHADispatch.java
##########
@@ -89,6 +91,13 @@ public void init() {
if(stickySessionsEnabled) {
stickySessionCookieName = serviceConfig.getStickySessionCookieName();
}
+ /* if a configurable user agent list is defined to turn off Knox LB used
it */
+
if(!StringUtils.isBlank(serviceConfig.getDisableStickySessionForUserAgents())) {
Review comment:
`StringUtils.isNotBlank` is easier to read than `!StringUtils.isBlank`
##########
File path:
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/provider/HaServiceConfig.java
##########
@@ -57,4 +57,8 @@
boolean isNoFallbackEnabled();
void setNoFallbackEnabled(boolean noFallbackEnabled);
+
+ void setDisableStickySessionForUserAgents(String
disableStickySessionForUserAgents);
+
+ String getDisableStickySessionForUserAgents();
Review comment:
Should return a `List<String>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
