[GitHub] [calcite-avatica] joshelser commented on a diff in pull request #217: CALCITE-5581: Client side LB implementation

["joshelser (via GitHub)" <gi...@apache.org>]

joshelser commented on code in PR #217:
URL: https://github.com/apache/calcite-avatica/pull/217#discussion_r1171645775


##########
site/_docs/client_reference.md:
##########
@@ -193,3 +193,52 @@ on-hover images for the permalink, but oh well.
 : _Default_: `false`.
 
 : _Required_: No.
+
+<strong><a name="use_client_side_lb" href="#use_client_side_lb">use_client_side_lb</a></strong>
+
+: _Description_: Enables the client side load-balancing.
+: _Default_: `false`.
+
+: _Required_: No.
+
+<strong><a name="lb_urls" href="#lb_urls">lb_urls</a></strong>
+
+: _Description_: List of URLs in a comma separated format, for example "URL1,URL2...URLn", to be used by the client side
+load balancer. Depending on the load balancing strategy, load balancer selects one of the URLs from the list.
+
+: _Default_: `null`.
+
+: _Required_: No.
+
+<strong><a name="lb_strategy" href="#lb_strategy">lb_strategy</a></strong>
+
+: _Description_: The load balancing strategy to be used by the client side load balancer. It must be a fully qualified
+Java class name which implements `org.apache.calcite.avatica.ha.LBStrategy`. Two implementations are provided

Review Comment:
   "Three" implementations appear to be provided.



##########
core/src/main/java/org/apache/calcite/avatica/ha/RandomSelectLBStrategy.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.avatica.ha;
+
+import org.apache.calcite.avatica.ConnectionConfig;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Random;
+
+/**
+ * Random Select strategy for client side load balancing.
+ */
+public class RandomSelectLBStrategy implements LBStrategy {

Review Comment:
   If you choose a random Avatica server every time, the client will (effectively) be stuck in a retry loop to re-create new Connection and Statements on that Avatica server, to then be bounced to a different Avatica server.
   
   I think this implementation would be overtly harmful should a user ever try to configure it.



##########
core/src/main/java/org/apache/calcite/avatica/ha/RoundRobinLBStrategy.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.
+ */
+package org.apache.calcite.avatica.ha;
+
+import org.apache.calcite.avatica.ConnectionConfig;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Round Robin strategy for client side load balancing.
+ * Its implemented it as a singleton so that we can maintain state
+ * i.e. which URL was last used from the list of URLs specified.
+ */
+public class RoundRobinLBStrategy implements LBStrategy {

Review Comment:
   Same with this implementation, if the caller is invoking a different Avatica server every call, the client will be "stuck" constantly trying to create and use the Statement/Connection which were on the n-1th server.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org