Layers -13 Switch to Delete layer api for cleanup in tests#1846
Open
bhanvimenghani wants to merge 7 commits intokruize:mvp_demofrom
Open
Layers -13 Switch to Delete layer api for cleanup in tests#1846bhanvimenghani wants to merge 7 commits intokruize:mvp_demofrom
bhanvimenghani wants to merge 7 commits intokruize:mvp_demofrom
Conversation
Contributor
Reviewer's GuideAdds full CRUD support for Layer entities via REST APIs, standardizes JSON responses and error handling, introduces DB update/delete methods with metrics, and updates tests to use the new Delete Layer API instead of direct DB cleanup. Sequence diagram for the Delete Layer REST APIsequenceDiagram
actor Client
participant LayerService
participant ExperimentDAOImpl
participant Database
Client->>LayerService: HTTP DELETE /deleteLayer?name=layerName
LayerService->>LayerService: validate layerName param
alt invalid layerName
LayerService-->>Client: 400 JSON error INVALID_LAYER_NAME
else valid layerName
LayerService->>LayerService: validate query params against DELETE_LAYERS_QUERY_PARAMS_SUPPORTED
alt invalid query params
LayerService-->>Client: 400 JSON error INVALID_QUERY_PARAMS
else valid params
LayerService->>ExperimentDAOImpl: loadLayerByName(layerName)
ExperimentDAOImpl->>Database: SELECT_FROM_LAYER_BY_NAME
Database-->>ExperimentDAOImpl: existingLayers
alt layer not found
LayerService-->>Client: 404 JSON error DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME
else layer exists
LayerService->>ExperimentDAOImpl: deleteLayerByName(layerName)
ExperimentDAOImpl->>Database: DELETE_FROM_LAYER_BY_NAME
Database-->>ExperimentDAOImpl: deletedCount
alt delete success
LayerService-->>Client: 200 JSON success DELETE_LAYER_SUCCESS_MSG
else delete failed
LayerService-->>Client: 500 JSON error with DB message
end
end
end
end
Sequence diagram for the Update Layer REST APIsequenceDiagram
actor Client
participant LayerService
participant ExperimentDAOImpl
participant Database
Client->>LayerService: HTTP PUT /updateLayer?name=layerName with JSON body
LayerService->>LayerService: validate layerName param
alt invalid layerName
LayerService-->>Client: 400 JSON error INVALID_LAYER_NAME
else valid layerName
LayerService->>LayerService: validate query params against UPDATE_LAYERS_QUERY_PARAMS_SUPPORTED
alt invalid query params
LayerService-->>Client: 400 JSON error INVALID_QUERY_PARAMS
else valid params
LayerService->>LayerService: read request body
alt empty body
LayerService-->>Client: 400 JSON error INVALID_LAYER_JSON
else non empty body
LayerService->>LayerService: convertInputJSONToCreateLayer(inputData)
alt converter throws IllegalArgumentException or JSONException
LayerService-->>Client: 400 JSON error VALIDATION_FAILED or MISSING_REQUIRED_FIELD
else conversion ok
LayerService->>LayerService: compare URL layerName with kruizeLayer.layerName
alt mismatch
LayerService-->>Client: 400 JSON error LAYER_NAME_MISMATCH
else match
LayerService->>LayerService: validate(kruizeLayer) via LayerValidation
alt validation fails
LayerService-->>Client: 400 JSON error VALIDATION_FAILED
else validation ok
LayerService->>ExperimentDAOImpl: loadLayerByName(kruizeLayer.layerName)
ExperimentDAOImpl->>Database: SELECT_FROM_LAYER_BY_NAME
Database-->>ExperimentDAOImpl: existingLayers
alt not found
LayerService-->>Client: 404 JSON error LAYER_NOT_FOUND
else found
LayerService->>LayerService: convertToLayerEntry(kruizeLayer)
LayerService->>ExperimentDAOImpl: updateLayerToDB(layerEntry)
ExperimentDAOImpl->>Database: merge(kruizeLayerEntry)
Database-->>ExperimentDAOImpl: update result
alt update success
LayerService-->>Client: 200 JSON success UPDATE_LAYER_SUCCESS_MSG
else update failed
LayerService-->>Client: 500 JSON error UPDATE_LAYER_TO_DB_FAILURE
end
end
end
end
end
end
end
end
Updated class diagram for LayerService, ExperimentDAO, and related typesclassDiagram
class LayerService {
+doPost(HttpServletRequest request, HttpServletResponse response) void
+doGet(HttpServletRequest request, HttpServletResponse response) void
+doPut(HttpServletRequest request, HttpServletResponse response) void
+doDelete(HttpServletRequest request, HttpServletResponse response) void
-convertToLayerEntry(KruizeLayer kruizeLayer) KruizeLMLayerEntry
-sendSuccessResponse(HttpServletResponse response, String message, int httpStatusCode) void
-sendErrorResponse(HttpServletResponse response, Exception e, int httpStatusCode, String errorMsg) void
-createGsonObject() Gson
-sendJsonResponse(HttpServletResponse response, String message, int statusCode, String status) void
}
class ExperimentDAO {
<<interface>>
+addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+updateLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+deleteLayerByName(String layerName) ValidationOutputData
+loadAllLayers() List~KruizeLMLayerEntry~
+loadLayerByName(String layerName) List~KruizeLMLayerEntry~
}
class ExperimentDAOImpl {
+addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+updateLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+deleteLayerByName(String layerName) ValidationOutputData
+loadAllLayers() List~KruizeLMLayerEntry~
+loadLayerByName(String layerName) List~KruizeLMLayerEntry~
}
class MetricsConfig {
+timerAddLayerDB Timer
+timerLoadAllLayers Timer
+timerLoadLayerByName Timer
+timerUpdateLayerDB Timer
+timerDeleteLayerDB Timer
+timerBAddLayerDB Timer_Builder
+timerBLoadAllLayers Timer_Builder
+timerBLoadLayerByName Timer_Builder
+timerBUpdateLayerDB Timer_Builder
+timerBDeleteLayerDB Timer_Builder
}
class KruizeSupportedTypes {
+CREATE_LAYERS_QUERY_PARAMS_SUPPORTED Set~String~
+LIST_LAYERS_QUERY_PARAMS_SUPPORTED Set~String~
+UPDATE_LAYERS_QUERY_PARAMS_SUPPORTED Set~String~
+DELETE_LAYERS_QUERY_PARAMS_SUPPORTED Set~String~
}
class AnalyzerErrorConstants_UpdateLayerAPI {
+INVALID_LAYER_NAME String
+LAYER_NOT_FOUND String
+INVALID_LAYER_JSON String
+UPDATE_LAYER_TO_DB_FAILURE String
+LAYER_NAME_MISMATCH String
+VALIDATION_FAILED String
+UNEXPECTED_ERROR String
+INVALID_QUERY_PARAMS String
+MISSING_REQUIRED_FIELD String
}
class AnalyzerErrorConstants_DeleteLayerAPI {
+DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME String
+DELETE_LAYER_ENTRY_ERROR_MSG String
+INVALID_LAYER_NAME String
+UNEXPECTED_ERROR String
+INVALID_QUERY_PARAMS String
}
class KruizeConstants_LayerAPIMessages {
+CREATE_LAYER_SUCCESS_MSG String
+UPDATE_LAYER_SUCCESS_MSG String
+DELETE_LAYER_SUCCESS_MSG String
+VIEW_LAYERS_MSG String
+ADD_LAYER_TO_DB String
+UPDATE_LAYER_TO_DB String
+DELETE_LAYER_FROM_DB String
+LOAD_LAYER_FAILURE String
+LOAD_ALL_LAYERS_FAILURE String
}
class ServerContext {
+CREATE_LAYER String
+LIST_LAYERS String
+UPDATE_LAYER String
+DELETE_LAYER String
}
class AnalyzerConstants {
+LAYER_NAME String
}
LayerService ..> ExperimentDAO : uses
ExperimentDAOImpl ..|> ExperimentDAO
LayerService ..> MetricsConfig : uses metrics
LayerService ..> KruizeSupportedTypes : uses query param sets
LayerService ..> AnalyzerErrorConstants_UpdateLayerAPI : uses error messages
LayerService ..> AnalyzerErrorConstants_DeleteLayerAPI : uses error messages
LayerService ..> KruizeConstants_LayerAPIMessages : uses success messages
Analyzer ..> LayerService : registers servlet
ServerContext ..> LayerService : endpoint paths
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
LayerService, the query-parameter validation logic indoPutanddoDeleteis nearly identical and could be factored into a shared helper to reduce duplication and keep the supported-params checks consistent. sendJsonResponsecreates a newGsoninstance on every call; consider reusing a single preconfiguredGsonfield to avoid repeated allocations and keep serialization configuration centralized.- In
ExperimentDAOImpl.updateLayerToDBanddeleteLayerByName, the exception handling still usese.printStackTrace()alongside structured logging and does not consistently checktx.isActive()before rollback in all paths; cleaning this up and aligning rollback and logging behavior would make DB error handling more robust and less noisy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `LayerService`, the query-parameter validation logic in `doPut` and `doDelete` is nearly identical and could be factored into a shared helper to reduce duplication and keep the supported-params checks consistent.
- `sendJsonResponse` creates a new `Gson` instance on every call; consider reusing a single preconfigured `Gson` field to avoid repeated allocations and keep serialization configuration centralized.
- In `ExperimentDAOImpl.updateLayerToDB` and `deleteLayerByName`, the exception handling still uses `e.printStackTrace()` alongside structured logging and does not consistently check `tx.isActive()` before rollback in all paths; cleaning this up and aligning rollback and logging behavior would make DB error handling more robust and less noisy.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/services/LayerService.java" line_range="117-120" />
<code_context>
sendErrorResponse(response, null, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
String.format(AnalyzerErrorConstants.APIErrors.CreateLayerAPI.ADD_LAYER_TO_DB_FAILURE, addedToDB.getMessage()));
}
+ } catch (IllegalArgumentException e) {
+ // Handle validation errors from converter (e.g., null elements in arrays)
+ LOGGER.error("Invalid input in layer creation request: {}", e.getMessage());
+ sendErrorResponse(response, e, HttpServletResponse.SC_BAD_REQUEST,
+ "Validation failed: " + e.getMessage());
} catch (MonitoringAgentNotSupportedException e) {
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Align create-layer validation errors with existing error constants instead of hardcoding the message.
In the `IllegalArgumentException` branch, the error body is currently `
Suggested implementation:
```java
} catch (IllegalArgumentException e) {
// Handle validation errors from converter (e.g., null elements in arrays)
LOGGER.error("Invalid input in layer creation request: {}", e.getMessage());
sendErrorResponse(response, e, HttpServletResponse.SC_BAD_REQUEST,
String.format(AnalyzerErrorConstants.APIErrors.CreateLayerAPI.CREATE_LAYER_INVALID_REQUEST, e.getMessage()));
} catch (MonitoringAgentNotSupportedException e) {
LOGGER.error("Failed to create layer: {}", e.getMessage());
sendErrorResponse(response, new Exception(e), HttpServletResponse.SC_BAD_REQUEST,
String.format(AnalyzerErrorConstants.APIErrors.CreateLayerAPI.CREATE_LAYER_INVALID_REQUEST, e.getMessage()));
```
1. Ensure that `AnalyzerErrorConstants.APIErrors.CreateLayerAPI.CREATE_LAYER_INVALID_REQUEST` exists and matches the formatting used here (e.g., `"Validation failed: %s"`).
2. If a differently named constant already exists for create-layer validation errors, replace `CREATE_LAYER_INVALID_REQUEST` in the snippet with that existing constant instead of introducing a new one.
3. If `AnalyzerErrorConstants` is not already imported in this file, confirm that the import is present at the top of `LayerService.java`.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java" line_range="710-715" />
<code_context>
+ tx.commit();
+ validationOutputData.setSuccess(true);
+ statusValue = "success";
+ } catch (HibernateException e) {
+ LOGGER.error("Not able to update layer due to: {}", e.getMessage(), e);
+ if (tx != null && tx.isActive()) tx.rollback();
+ e.printStackTrace();
+ validationOutputData.setSuccess(false);
+ validationOutputData.setMessage(e.getMessage());
+ } catch (Exception e) {
+ LOGGER.error("Not able to update layer due to: {}", e.getMessage(), e);
</code_context>
<issue_to_address>
**suggestion:** Avoid redundant `printStackTrace()` and unify exception handling/logging in `updateLayerToDB`.
In `updateLayerToDB`, both catch blocks already log the exception (including the stack trace via the logger), so the extra `e.printStackTrace()` in the `HibernateException` branch is redundant and noisy. You can drop that call and consider merging the two catch blocks into a single one that logs once and sets `validationOutputData` consistently.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java" line_range="738-689" />
<code_context>
+ public ValidationOutputData deleteLayerByName(String layerName) {
</code_context>
<issue_to_address>
**suggestion:** Instrument `deleteLayerByName` with the new DB timers to keep metrics consistent.
You’ve added `timerDeleteLayerDB` and `timerBDeleteLayerDB` in `MetricsConfig`, but `deleteLayerByName` doesn’t use them. Please wrap the delete logic in a `Timer.Sample` and record the result via `MetricsConfig.timerDeleteLayerDB` with the appropriate status tag, matching the pattern used in `addLayerToDB`/`updateLayerToDB`.
Suggested implementation:
```java
@Override
public ValidationOutputData deleteLayerByName(String layerName) {
// Start DB metrics timer for deleteLayerByName
Timer.Sample sample = Timer.start(MetricsConfig.meterRegistry);
String deleteStatus = KruizeConstants.METRICS.TIMER_METRIC_STATUS_SUCCESS;
ValidationOutputData validationOutputData = new ValidationOutputData(false, null, null);
```
To fully implement the metrics instrumentation consistent with `addLayerToDB` / `updateLayerToDB`, you should also:
1. In the `catch` block(s) inside `deleteLayerByName`, set `deleteStatus` to the failure constant you use elsewhere (for example `KruizeConstants.METRICS.TIMER_METRIC_STATUS_FAILURE`) before handling/logging the exception.
2. In a `finally` block that always executes (outside your inner `try { tx = session.beginTransaction(); ... }` but inside the `try (Session session = ...)`), stop and record the timer using the same pattern as the other DB methods, for example something along the lines of:
```java
MetricsConfig.recordDBTimer(MetricsConfig.timerDeleteLayerDB, sample, deleteStatus);
```
or whatever helper / tagging pattern you already use for `addLayerToDB` and `updateLayerToDB`.
3. Ensure the following imports are present at the top of the file, adjusted to your actual package structure:
```java
import io.micrometer.core.instrument.Timer;
import com.autotune.utils.MetricsConfig;
import com.autotune.utils.KruizeConstants;
```
4. Mirror any status tag keys/values (`status`, `success`, `failure`, etc.) exactly as used in the existing DB-layer methods so that metrics remain consistent across operations.
</issue_to_address>
### Comment 4
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_create_layer.py" line_range="40" />
<code_context>
# Cleanup before test - clean up all known layer names
for layer_name in CLEANUP_LAYER_NAMES:
- delete_layer_from_db(layer_name)
+ delete_layer(layer_name)
yield
# Cleanup after test - clean up all known layer names
</code_context>
<issue_to_address>
**issue (testing):** Extend create-layer tests to cover new validation cases (empty body and null elements in arrays)
The new validation logic in `convertInputJSONToCreateLayer` and `LayerService.doPost` (empty/whitespace bodies and `null` elements in `queries`, `label`, and `tunables`) isn’t covered by the current tests. Please add targeted tests, e.g.:
- Empty request body to `/createLayer` → expect 400 with `CreateLayerAPI.INVALID_LAYER_JSON`.
- Valid JSON except `queries` contains a `null` element → expect 400 and the `IllegalArgumentException` message.
- Similar cases for `label` and `tunables` containing `null`.
This will verify the new validations and help prevent regressions in JSON parsing behavior.
</issue_to_address>
### Comment 5
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_create_layer.py" line_range="134" />
<code_context>
layer_name = input_json['layer_name']
# Cleanup before test to ensure clean state
- delete_layer_from_db(layer_name)
+ delete_layer(layer_name)
</code_context>
<issue_to_address>
**suggestion (testing):** Have cleanup logic assert on delete API response instead of ignoring failures
Cleanup now depends on `delete_layer(layer_name)` but its result isn’t checked, so failed deletes (wrong URL/params, server errors) could leave state dirty and cause flaky tests. Please move this into a shared helper/fixture that calls `delete_layer(layer_name)` and asserts the response `status_code` is 200 (deleted) or 404 (already absent), failing otherwise, so delete issues surface directly instead of as indirect test failures.
Suggested implementation:
```python
def ensure_layer_deleted(layer_name: str) -> None:
"""
Ensure a layer is deleted via the delete API.
Asserts that the delete response status code is either:
* 200 - layer successfully deleted
* 404 - layer already absent
Any other status code fails the test so cleanup issues surface directly.
"""
response = delete_layer(layer_name)
assert response.status_code in (200, 404), (
f"Failed to delete layer {layer_name!r}: "
f"expected status 200 or 404, got {response.status_code}"
)
@pytest.mark.layers
```
```python
yield
# Cleanup after test - clean up all known layer names
for layer_name in CLEANUP_LAYER_NAMES:
ensure_layer_deleted(layer_name)
```
```python
yield
# Cleanup after test - clean up all known layer names
for layer_name in CLEANUP_LAYER_NAMES:
ensure_layer_deleted(layer_name)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
bhanvimenghani
changed the title
bhanvimenghani
force-pushed
the
del_layers_for_cleanup
branch
from
ba08c72 to
2f0c1d9
Compare
Member
bharathappali
left a comment
bharathappali
left a comment
There was a problem hiding this comment.
Everything else LGTM, except the sourcery comments in tests code. @bhanvimenghani please kindly address the sourcery review comments. Thanks!
Contributor
Author
@bharathappali can you please take a look once again |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pr is on top of 1826 pr. In this PR we are switching over to make use of delete layer functionality in already existing layer related tests.
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
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Add full CRUD support for layer resources via REST APIs and update tests to use the new deleteLayer endpoint for cleanup.
New Features:
Bug Fixes:
Enhancements:
Tests: