Refactor: Move common logic to GenericRecommendationModel #1544
Conversation
There was a problem hiding this comment.
Thanks @bhanvimenghani for raising this PR to have a common code across recommendation models. There are some trivial changes which needs to be done. I have added my review comments, Please let me know if you need any clarification wrt any review comment.
Thanks!
| @@ -132,18 +134,18 @@ private void loadDefaultRecommendationModels() { | |||
| // create both cost and performance model by default | |||
| recommendationModels = new ArrayList<>(); | |||
| // Create Cost based model | |||
| CostBasedRecommendationModel costBasedRecommendationModel = new CostBasedRecommendationModel(); | |||
| CostBasedRecommendationModel costBasedRecommendationModel = new CostBasedRecommendationModel("cost", COST_RECOMMENDATION_TUNABLES); | |||
There was a problem hiding this comment.
Please use a constant in place of string cost
| registerModel(costBasedRecommendationModel); | ||
| // Create Performance based model | ||
| PerformanceBasedRecommendationModel performanceBasedRecommendationModel = new PerformanceBasedRecommendationModel(); | ||
| PerformanceBasedRecommendationModel performanceBasedRecommendationModel = new PerformanceBasedRecommendationModel("performance", PERFORMANCE_RECOMMENDATION_TUNABLES); |
There was a problem hiding this comment.
Please use a constant in place of string performance
| registerModel(performanceBasedRecommendationModel); | ||
| } | ||
|
|
||
| private void loadDefaultRecommendationModelForAutoAndRecreate() { | ||
| // create performance model by default | ||
| recommendationModels = new ArrayList<>(); | ||
| // Create Performance based model | ||
| PerformanceBasedRecommendationModel performanceBasedRecommendationModel = new PerformanceBasedRecommendationModel(); | ||
| PerformanceBasedRecommendationModel performanceBasedRecommendationModel = new PerformanceBasedRecommendationModel("performance", PERFORMANCE_RECOMMENDATION_TUNABLES); |
There was a problem hiding this comment.
Please use a constant in place of string performance
| @@ -161,11 +163,13 @@ private void loadCustomRecommendationModels(List<String> modelName) throws Inval | |||
| for (String model : modelName) { | |||
| if (KruizeConstants.JSONKeys.COST.equalsIgnoreCase(model)) { | |||
| // Create Cost based model | |||
| CostBasedRecommendationModel costBasedRecommendationModel = new CostBasedRecommendationModel(); | |||
| // Todo: add custom cost parameters over here | |||
| CostBasedRecommendationModel costBasedRecommendationModel = new CostBasedRecommendationModel("cost", COST_RECOMMENDATION_TUNABLES); | |||
There was a problem hiding this comment.
Please use a constant in place of string cost
| registerModel(costBasedRecommendationModel); | ||
| } else if (KruizeConstants.JSONKeys.PERFORMANCE.equalsIgnoreCase(model)) { | ||
| // Create Performance based model | ||
| PerformanceBasedRecommendationModel performanceBasedRecommendationModel = new PerformanceBasedRecommendationModel(); | ||
| // Todo: add custom performance parameters over here from the user inputs |
There was a problem hiding this comment.
Please use a constant in place of string performance
| public String name = RecommendationConstants.RecommendationEngine.ModelNames.COST; | ||
|
|
||
| public CostBasedRecommendationModel(String name, RecommendationTunables recommendationTunables) { | ||
| super( name, recommendationTunables ); |
There was a problem hiding this comment.
Can we pass the internally mentioned name variable, than relying on externally passed name string, as the name can be misleading.
I can create a CostBasedRecommendationModel with the name performance if I'm allowed to do this.
You can just pass the RecommendationTunables while by default the name if constant cost
Please let me know your thoughts on this @bhanvimenghani
There was a problem hiding this comment.
sure, I can make this change!
| public static final String name = RecommendationConstants.RecommendationEngine.ModelNames.PERFORMANCE; | ||
|
|
||
| public PerformanceBasedRecommendationModel(String name, RecommendationTunables recommendationTunables) { | ||
| super(name, recommendationTunables); |
There was a problem hiding this comment.
Same here, Can we use the constant / internal name var rather than allowing the instance creator to name it?
We can just pass the RecommendationTunables for creating the instance
| @@ -0,0 +1,38 @@ | |||
| package com.autotune.analyzer.recommendations.model; | |||
|
|
|||
There was a problem hiding this comment.
Please add a javadoc which describes the class.
|
|
||
| public double memoryPercentile; | ||
| public double cpuPercentile; | ||
| public double acceleratorPercentile; |
There was a problem hiding this comment.
I was thinking, should we give the dev the flexibility to update or edit these upon creation?
If these values are public, anywhere in the code these can be modified after creation. If we are not planning to update them, Can we please maintain them as private and only have getters but not setters (They should be assigned only via constructor)
There was a problem hiding this comment.
Also please make them final if we are not planning to have setters. They should be initialised in constructor and remain unchanged.
| @@ -35,6 +38,7 @@ public class KruizeConstants { | |||
| public static final String OPENSHIFT = "openshift"; | |||
| public static final String CONFIG_FILE = "KRUIZE_CONFIG_FILE"; | |||
| public static final String SQL_EXCEPTION_HELPER_PKG = "org.hibernate.engine.jdbc.spi"; | |||
| public static RecommendationTunables CostBasedRecommendationConstants; | |||
There was a problem hiding this comment.
We are not using this, also this is not initialized. Can we please remove it?
Description
This pr aims to make the Recommendation models code efficient and modular by creating a generic class that contains the common functionality between cost and performance models. And then the cost and performance models can extend the generic functionality and build on top of the base class.
Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Quay image: quay.io/bmenghan/common_code_changes:v2