-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix failures due to delete experiment #1437
base: mvp_demo
Are you sure you want to change the base?
Conversation
Signed-off-by: Saad Khan <[email protected]>
Signed-off-by: Saad Khan <[email protected]>
Signed-off-by: Saad Khan <[email protected]>
@khansaad - local monitoring fault tolerant test also needs to be updated |
Signed-off-by: Saad Khan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested some optimizations, Apart from that, everything looks good to me!
@@ -171,6 +171,14 @@ protected void doPost(HttpServletRequest request, HttpServletResponse response) | |||
protected void doDelete(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { | |||
Map<String, KruizeObject> mKruizeExperimentMap = new ConcurrentHashMap<String, KruizeObject>(); | |||
String inputData = ""; | |||
String rm = request.getParameter(AnalyzerConstants.ServiceConstants.RM); | |||
boolean rmTable = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the if
block below is used to just match the value and set the rmTable
, we could simplify it with
// Check if rm is not null and set to true
boolean rmTable = (null != rm && AnalyzerConstants.BooleanString.TRUE.equalsIgnoreCase(rm.trim()));
json_file = open(input_json_file, "r") | ||
input_json = json.loads(json_file.read()) | ||
|
||
print("\nDeleting the experiment...") | ||
url = URL + "/createExperiment" | ||
if rm: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are relaying on the boolean param which is passed to the function to maintain it's string representation, we can directly use the string
representation of the boolean instead of placing a if-else
block.
I was suggesting something like this:
PARAMS = {'rm': str(rm).lower()}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khansaad - Changes look fine. Please ensure all the tests using delete experiment work as expected and share the results.
Description
This PR adds
rm
flag for the delete experiment API and corresponding test file changesFixes # (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
Include any additional information such as links, test results, screenshots here