[WIP] Adds cluster_name parameter to listRecommendations API #944

shreyabiradar07 · 2023-09-13T11:53:49Z

This PR fixes #940

Enhancement of /listRecommendations API by adding cluster_name query parameter to retrieve recommendations for a specific cluster based on its name

khansaad · 2023-10-04T05:36:24Z

src/main/java/com/autotune/analyzer/services/ListRecommendations.java

@@ -145,7 +146,60 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
                            String.format(AnalyzerErrorConstants.APIErrors.ListRecommendationsAPI.INVALID_EXPERIMENT_NAME_MSG, experimentName)
                    );
                }
-            } else {
+            }else if(null != clusterName){


There should be space after braces and Parantheses

khansaad · 2023-10-04T05:38:28Z

src/main/java/com/autotune/analyzer/services/ListRecommendations.java

+                try {
+                    new ExperimentDBService().loadExperimentsAndRecommendationsFromDBByClusterName(mKruizeExperimentMap, clusterName);
+                } catch (Exception e) {
+                    LOGGER.error("Loading saved cluster {} failed: {} ", clusterName, e.getMessage());


Error message should be:
LOGGER.error("Loading experiments and recommendations based on cluster {} failed: {} ", clusterName, e.getMessage());

khansaad · 2023-10-04T05:40:53Z

src/main/java/com/autotune/analyzer/services/ListRecommendations.java

+                            String.format(AnalyzerErrorConstants.APIErrors.ListRecommendationsAPI.INVALID_CLUSTER_NAME_MSG, clusterName)
+                    );
+                }
+            }else {


Add Space here as well and re-check all other places

khansaad · 2023-10-04T05:44:02Z

src/main/java/com/autotune/database/dao/ExperimentDAO.java

    List<KruizeResultsEntry> loadResultsByExperimentName(String experimentName, String cluster_name, Timestamp interval_start_time, Integer limitRows) throws Exception;

+    // Load all results for a particular clusterName
+    List<KruizeResultsEntry> loadResultsByClusterName(String clusterName, Timestamp interval_start_time, Integer limitRows) throws Exception;


This method is not required. You can remove this.

khansaad · 2023-10-04T05:46:58Z

src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java

+                    .setParameter("clusterName", clusterName).list();
+            statusValue = "success";
+        } catch (Exception e) {
+            LOGGER.error("Not able to load cluster {} due to {}", clusterName, e.getMessage());


Change the Logger error message here as well as per the changes I suggested above.

khansaad · 2023-10-04T05:57:49Z

src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java

@@ -498,6 +520,36 @@ public List<KruizeResultsEntry> loadResultsByExperimentName(String experimentNam
        }
        return kruizeResultsEntries;
    }
+    @Override
+    public List<KruizeResultsEntry> loadResultsByClusterName(String clusterName, Timestamp interval_end_time, Integer limitRows) throws Exception {


Remove this whole implementation as well, we aren't using this anywhere.

khansaad · 2023-10-04T05:59:43Z

src/main/java/com/autotune/database/helper/DBConstants.java

@@ -23,8 +24,11 @@ public static final class SQLQUERY {
                KruizeConstants.JSONKeys.CLUSTER_NAME, KruizeConstants.JSONKeys.EXPERIMENT_NAME, KruizeConstants.JSONKeys.INTERVAL_START_TIME, KruizeConstants.JSONKeys.INTERVAL_END_TIME);
        public static final String SELECT_FROM_RESULTS_BY_EXP_NAME_AND_END_TIME = String.format("from KruizeResultsEntry k WHERE k.cluster_name = :%s and k.experiment_name = :%s and k.interval_end_time = :%s", KruizeConstants.JSONKeys.CLUSTER_NAME, KruizeConstants.JSONKeys.EXPERIMENT_NAME, KruizeConstants.JSONKeys.INTERVAL_END_TIME);
        public static final String SELECT_FROM_RESULTS_BY_EXP_NAME_AND_MAX_END_TIME = String.format("from KruizeResultsEntry k WHERE k.cluster_name = :%s and k.experiment_name = :%s and k.interval_end_time = (SELECT MAX(e.interval_end_time) FROM KruizeResultsEntry e  where e.experiment_name = :%s )", KruizeConstants.JSONKeys.CLUSTER_NAME, KruizeConstants.JSONKeys.EXPERIMENT_NAME, KruizeConstants.JSONKeys.EXPERIMENT_NAME);
+        public static final String SELECT_FROM_RESULTS_BY_CLUSTER_NAME = "from KruizeResultsEntry k WHERE k.cluster_name = :clusterName";
+        public static final String SELECT_FROM_RESULTS_BY_CLUSTER_NAME_AND_DATE_RANGE = String.format("from KruizeResultsEntry k WHERE k.cluster_name = :%s and k.interval_end_time <= :%s ORDER BY k.interval_end_time DESC", KruizeConstants.JSONKeys.CLUSTER_NAME, KruizeConstants.JSONKeys.INTERVAL_END_TIME, KruizeConstants.JSONKeys.INTERVAL_START_TIME);


Both of these constants can be removed as well

khansaad · 2023-10-04T06:00:47Z

src/main/java/com/autotune/database/service/ExperimentDBService.java

@@ -189,6 +189,53 @@ public void loadRecommendationsFromDBByName(Map<String, KruizeObject> mainKruize
            }
        }
    }
+    // Todo - confirm if experimentResultData to be changed to clusterResultData
+    public void loadResultsFromDBByClusterName(Map<String, KruizeObject> mainKruizeExperimentMap, String clusterName, Timestamp interval_end_time, Integer limitRows) throws Exception {


This method isn't required as well, can be removed.

khansaad · 2023-10-04T06:06:52Z

src/main/java/com/autotune/database/service/ExperimentDBService.java

+                    }
+                }
+                if (failureThreshHold > 0 && failureCount == failureThreshHold) {
+                    throw new Exception("Experiments of cluster " + clusterName + " unable to load from DB.");


Change this exception message so that it makes more sense

khansaad · 2023-10-04T06:07:46Z

src/main/java/com/autotune/database/service/ExperimentDBService.java

@@ -313,6 +388,16 @@ public void loadExperimentAndRecommendationsFromDBByName(Map<String, KruizeObjec
        loadRecommendationsFromDBByName(mainKruizeExperimentMap, experimentName);
    }

+    public void loadExperimentsAndResultsFromDBByClusterName(Map<String, KruizeObject> mainKruizeExperimentMap, String clusterName) throws Exception {


Remove this as well.

… results

shreyabiradar07 · 2023-11-27T08:20:56Z

@khansaad - I have incorporated review comments and rebased branch 'add_clusterName_parameter' onto 'mvp_demo'.
Can you please review the updated PR?

dinogun · 2024-10-03T17:32:44Z

Closing stale PRs

khansaad assigned shreyabiradar07 Sep 14, 2023

khansaad added enhancement New feature or request remote_monitoring API Requires API Changes labels Sep 14, 2023

khansaad added this to the Kruize 0.0.20_rm Release milestone Sep 14, 2023

khansaad linked an issue Sep 14, 2023 that may be closed by this pull request

[API] Add an API to list Recommendations for a given cluster. #940

Open

khansaad self-requested a review September 14, 2023 07:35

shreyabiradar07 force-pushed the add_clusterName_parameter branch from 2a4d524 to 214e623 Compare September 29, 2023 08:46

shreyabiradar07 marked this pull request as ready for review October 3, 2023 13:00

khansaad reviewed Oct 4, 2023

View reviewed changes

rbadagandi1 modified the milestones: Kruize 0.0.20_rm Release, Kruize 0.0.22_rm Release Oct 11, 2023

shreyabiradar07 added 9 commits November 27, 2023 12:07

add declaration for clusterName

3b5499c

rebase with mvp_demo adding clusterName functionality

e9b4117

resolve conflicts of ExperimentDAOImpl and DBConstants

8dc5622

resolve code conflicts by loading results by clusterName

47b0236

refactor dbconstants for cluster_name

9632fe6

refactor code changes with experiment_name as key

4d6ff95

insert spaces after braces and parentheses

1bf0002

removes redundant methods and DB constants using cluster_name to load…

9a0f06b

… results

refactor error handling and logger messages

117fa52

shreyabiradar07 force-pushed the add_clusterName_parameter branch from 5dc255e to 117fa52 Compare November 27, 2023 08:02

add DB index for cluster_name in kruize_recommendations table

962283c

rbadagandi1 removed this from the Kruize 0.0.23_rm Release milestone Jun 3, 2024

dinogun closed this Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Adds cluster_name parameter to listRecommendations API #944

[WIP] Adds cluster_name parameter to listRecommendations API #944

shreyabiradar07 commented Sep 13, 2023 •

edited

Loading

khansaad Oct 4, 2023

shreyabiradar07 Oct 4, 2023

khansaad Oct 4, 2023

khansaad Oct 4, 2023

khansaad Oct 4, 2023

khansaad Oct 4, 2023

khansaad Oct 4, 2023

khansaad Oct 4, 2023

khansaad Oct 4, 2023

khansaad Oct 4, 2023

khansaad Oct 4, 2023

shreyabiradar07 commented Nov 27, 2023

dinogun commented Oct 3, 2024

[WIP] Adds cluster_name parameter to listRecommendations API #944

[WIP] Adds cluster_name parameter to listRecommendations API #944

Conversation

shreyabiradar07 commented Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyabiradar07 commented Nov 27, 2023

dinogun commented Oct 3, 2024

shreyabiradar07 commented Sep 13, 2023 •

edited

Loading