[GitHub] [servicecomb-java-chassis] liubao68 commented on a change in pull request #1894: [SCB-2043] flow control support custom / bug fix

[GitBox]

liubao68 commented on a change in pull request #1894:
URL: 
https://github.com/apache/servicecomb-java-chassis/pull/1894#discussion_r462717447



##
File path: 
handlers/handler-flowcontrol-qps/src/main/java/org/apache/servicecomb/qps/QpsControllerManager.java
##
@@ -209,22 +210,25 @@ public QpsControllerManager setGlobalQpsStrategy(String 
globalLimitKey, String g
   }
 
   private AbstractQpsStrategy chooseStrategy(String globalConfigKey, Long 
limit, Long bucket,
-  String strategyKey) {
-AbstractQpsStrategy strategy;
-switch (StrategyType.parseStrategyType(strategyKey)) {
-  case FixedWindow:
-strategy = new FixedWindowStrategy(globalConfigKey, limit);
-break;
-  case LeakyBucket:
-strategy = new LeakyBucketStrategy(globalConfigKey, limit);
-break;
-  case TokenBucket:
-strategy = new TokenBucketStrategy(globalConfigKey, limit, bucket);
+  String strategyName) {
+if (StringUtils.isEmpty(strategyName)) {
+  strategyName = "FixedWindow";
+}
+AbstractQpsStrategy strategy = null;
+List strategyFactories = SPIServiceUtils
+.getOrLoadSortedService(IStrategyFactory.class);
+for (IStrategyFactory strategyFactory : strategyFactories) {
+  strategy = strategyFactory.getStrategy(strategyName);
+  if (strategy != null) {
 break;
-  case SlidingWindow:
-  default:
-strategy = new FixedWindowStrategy(globalConfigKey, limit);
+  }
+}
+if (strategy == null) {
+  throw new ServiceCombException("the qps strategy name is not exist , 
please check.");

Review comment:
   exeception message is better to include `strategyName`

##
File path: 
handlers/handler-flowcontrol-qps/src/main/java/org/apache/servicecomb/qps/strategy/DefaultStrategyFactory.java
##
@@ -14,27 +14,20 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-
 package org.apache.servicecomb.qps.strategy;
 
-import org.apache.commons.lang3.StringUtils;
-
-public enum StrategyType {
-  TokenBucket,
-  LeakyBucket,
-  FixedWindow,
-  SlidingWindow;
-
-
-  public static StrategyType parseStrategyType(String type) {
-if (StringUtils.isEmpty(type)) {
-  return StrategyType.FixedWindow;
-}
+public class DefaultStrategyFactory implements IStrategyFactory {
 
-try {
-  return StrategyType.valueOf(type.toUpperCase());
-} catch (IllegalArgumentException e) {
-  return StrategyType.FixedWindow;
+  public AbstractQpsStrategy getStrategy(String strategyName) {

Review comment:
   It's better to use `createStrategy`





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.

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

[GitHub] [servicecomb-java-chassis] liubao68 commented on a change in pull request #1894: [SCB-2043] flow control support custom / bug fix

[GitBox]

liubao68 commented on a change in pull request #1894:
URL: 
https://github.com/apache/servicecomb-java-chassis/pull/1894#discussion_r461262652



##
File path: 
handlers/handler-flowcontrol-qps/src/main/java/org/apache/servicecomb/qps/QpsControllerManager.java
##
@@ -211,6 +212,8 @@ public QpsControllerManager setGlobalQpsStrategy(String 
globalLimitKey, String g
   private AbstractQpsStrategy chooseStrategy(String globalConfigKey, Long 
limit, Long bucket,
   String strategyKey) {
 AbstractQpsStrategy strategy;
+AbstractQpsStrategy customStrategy = SPIServiceUtils

Review comment:
   Maybe this code can be better refactored.
   
   1. all strategies are SPI 
   2. any implementation have a name
   3. choose the target strategy by configured name and default to 'FixedWindow'

##
File path: 
handlers/handler-flowcontrol-qps/src/main/java/org/apache/servicecomb/qps/QpsControllerManager.java
##
@@ -211,6 +212,8 @@ public QpsControllerManager setGlobalQpsStrategy(String 
globalLimitKey, String g
   private AbstractQpsStrategy chooseStrategy(String globalConfigKey, Long 
limit, Long bucket,
   String strategyKey) {
 AbstractQpsStrategy strategy;
+AbstractQpsStrategy customStrategy = SPIServiceUtils

Review comment:
   Maybe this code can be better refactored.
   
   1. all strategies are SPI 
   2. any implementation have a name
   3. choose the target strategy by configured name and default to 'FixedWindow'
   
   By doing this, the strategy enum and case statements can be removed. 

##
File path: 
handlers/handler-flowcontrol-qps/src/main/java/org/apache/servicecomb/qps/QpsControllerManager.java
##
@@ -211,6 +212,8 @@ public QpsControllerManager setGlobalQpsStrategy(String 
globalLimitKey, String g
   private AbstractQpsStrategy chooseStrategy(String globalConfigKey, Long 
limit, Long bucket,
   String strategyKey) {
 AbstractQpsStrategy strategy;
+AbstractQpsStrategy customStrategy = SPIServiceUtils

Review comment:
   Hidden parameters in implementation and I think will work. 

##
File path: 
handlers/handler-flowcontrol-qps/src/main/java/org/apache/servicecomb/qps/QpsControllerManager.java
##
@@ -211,6 +212,8 @@ public QpsControllerManager setGlobalQpsStrategy(String 
globalLimitKey, String g
   private AbstractQpsStrategy chooseStrategy(String globalConfigKey, Long 
limit, Long bucket,
   String strategyKey) {
 AbstractQpsStrategy strategy;
+AbstractQpsStrategy customStrategy = SPIServiceUtils

Review comment:
   Hidden parameters in implementation and I think will work. You can think 
of users will custom strategies like this, how will they do it?

##
File path: 
handlers/handler-flowcontrol-qps/src/main/java/org/apache/servicecomb/qps/QpsControllerManager.java
##
@@ -210,21 +209,17 @@ public QpsControllerManager setGlobalQpsStrategy(String 
globalLimitKey, String g
 
   private AbstractQpsStrategy chooseStrategy(String globalConfigKey, Long 
limit, Long bucket,
   String strategyKey) {
-AbstractQpsStrategy strategy;
-switch (StrategyType.parseStrategyType(strategyKey)) {
-  case FixedWindow:
-strategy = new FixedWindowStrategy(globalConfigKey, limit);
-break;
-  case LeakyBucket:
-strategy = new LeakyBucketStrategy(globalConfigKey, limit);
-break;
-  case TokenBucket:
-strategy = new TokenBucketStrategy(globalConfigKey, limit, bucket);
-break;
-  case SlidingWindow:
-  default:
-strategy = new FixedWindowStrategy(globalConfigKey, limit);
+AbstractQpsStrategy strategy = new FixedWindowStrategy(globalConfigKey, 
limit);

Review comment:
   strategyKey can have default value 'FixedWindow', wrong used defined 
value can throw exception. So here do not need new a default strategy. 

##
File path: 
handlers/handler-flowcontrol-qps/src/main/java/org/apache/servicecomb/qps/QpsControllerManager.java
##
@@ -210,21 +209,17 @@ public QpsControllerManager setGlobalQpsStrategy(String 
globalLimitKey, String g
 
   private AbstractQpsStrategy chooseStrategy(String globalConfigKey, Long 
limit, Long bucket,
   String strategyKey) {
-AbstractQpsStrategy strategy;
-switch (StrategyType.parseStrategyType(strategyKey)) {
-  case FixedWindow:
-strategy = new FixedWindowStrategy(globalConfigKey, limit);
-break;
-  case LeakyBucket:
-strategy = new LeakyBucketStrategy(globalConfigKey, limit);
-break;
-  case TokenBucket:
-strategy = new TokenBucketStrategy(globalConfigKey, limit, bucket);
-break;
-  case SlidingWindow:
-  default:
-strategy = new FixedWindowStrategy(globalConfigKey, limit);
+AbstractQpsStrategy strategy = new FixedWindowStrategy(globalConfigKey, 
limit);
+List customStrategy = SPIServiceUtils
+.getOrLoadSortedService(AbstractQpsStrategy.class);
+for