You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2021/07/14 22:15:35 UTC
[GitHub] [knox] moresandeep opened a new pull request #469: KNOX-2634 - Disable HA loadbalancing for ODBC driver and make this setting configurable.
moresandeep opened a new pull request #469:
URL: https://github.com/apache/knox/pull/469
## What changes were proposed in this pull request?
Add a way to turn off HA loadbalancing based on user agent.
This is done because some non-browser clients such as ODBC drivers do not honor multiple SET-COOKIE headers in response body. This config is to work around that issue.
This PR proposed a configuration in HA Provider called `disableLoadBalancingForUserAgents` which takes a comma separated list of user agents that should disable the HA loadbalancing ifeature.
Following is an example config
```
<provider>
<role>ha</role>
<name>HaProvider</name>
<enabled>true</enabled>
<param>
<name>HIVE</name> <value>enableStickySession=true;enableLoadBalancing=true;enabled=true;maxFailoverAttempts=42;failoverSleep=50;maxRetryAttempts=1;disableLoadBalancingForUserAgents=Test User Agent, Test User Agent2,Test User Agent3 ,Test User Agent4 ;retrySleep=1000</value>
</param>
</provider>
```
Default value of `disableLoadBalancingForUserAgents=ClouderaODBCDriverforApacheHive`
## How was this patch tested?
This patch was tested on a local cluster
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #469: KNOX-2634 - Disable HA loadbalancing for ODBC driver and make this setting configurable.
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #469:
URL: https://github.com/apache/knox/pull/469#discussion_r670550117
##########
File path: gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/provider/impl/HaServiceConfigConstants.java
##########
@@ -53,4 +59,6 @@
boolean DEFAULT_NO_FALLBACK_ENABLED = false;
String DEFAULT_STICKY_SESSION_COOKIE_NAME = "KNOX_BACKEND";
+
+ String DEFAULT_DISABLE_LB_USER_AGENTS = "ClouderaODBCDriverforApacheHive";
Review comment:
Since this is coming from the HA config and will be a string I had it set up to mimic the config but looking at Sandor's and your comment looks like array/list might be appropriate.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #469: KNOX-2634 - Disable HA loadbalancing for ODBC driver and make this setting configurable.
Posted by GitBox <gi...@apache.org>.
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #469: KNOX-2634 - Disable HA loadbalancing for ODBC driver and make this setting configurable.
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #469:
URL: https://github.com/apache/knox/pull/469#discussion_r670474747
##########
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);
Review comment:
(minor) nit: The method name is misleading to me.
setStickySessionDisabledUserAgents() ?
##########
File path: gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/provider/impl/HaServiceConfigConstants.java
##########
@@ -53,4 +59,6 @@
boolean DEFAULT_NO_FALLBACK_ENABLED = false;
String DEFAULT_STICKY_SESSION_COOKIE_NAME = "KNOX_BACKEND";
+
+ String DEFAULT_DISABLE_LB_USER_AGENTS = "ClouderaODBCDriverforApacheHive";
Review comment:
What if the default includes multiple values in the future? Should this be an array type instead?
##########
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:
(minor) nit: The method name is misleading to me.
List<String> getStickySessionDisabledUserAgents() ?
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #469: KNOX-2634 - Disable HA loadbalancing for ODBC driver and make this setting configurable.
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #469:
URL: https://github.com/apache/knox/pull/469#discussion_r670590337
##########
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:
Right, Phil mentioned the same, I'll change it.
##########
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:
true, will change it.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep merged pull request #469: KNOX-2634 - Disable HA loadbalancing for ODBC driver and make this setting configurable.
Posted by GitBox <gi...@apache.org>.
moresandeep merged pull request #469:
URL: https://github.com/apache/knox/pull/469
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org