Open
Conversation
Contributor
Reviewer's GuideAdds full CRUD support and stronger validation for Layer APIs (create, list, update, delete), migrates tests to use HTTP-based helpers instead of DB hacks, and significantly expands positive/negative test coverage for layer creation, listing, updating, and deletion, including performance and edge cases. Sequence diagram for the updateLayer REST APIsequenceDiagram
actor Client
participant LayerService
participant Converters
participant LayerValidation
participant ExperimentDAOImpl
participant DB
participant MetricsConfig
Client->>LayerService: HTTP PUT /updateLayer?name=layerName
LayerService->>LayerService: getParameter(AnalyzerConstants.LAYER_NAME)
LayerService-->>Client: 400 INVALID_LAYER_NAME (if missing/empty)
rect rgb(235, 235, 255)
LayerService->>LayerService: validate query params using KruizeSupportedTypes.UPDATE_LAYERS_QUERY_PARAMS_SUPPORTED
LayerService-->>Client: 400 INVALID_QUERY_PARAMS (if invalid)
end
LayerService->>LayerService: read request body
LayerService-->>Client: 400 INVALID_LAYER_JSON (if empty)
LayerService->>Converters: convertInputJSONToCreateLayer(inputData)
Converters->>Converters: validate apiVersion, kind, metadata, layer_presence, tunables
Converters-->>LayerService: KruizeLayer
LayerService-->>Client: 400 VALIDATION_FAILED (IllegalArgumentException)
LayerService->>LayerService: compare URL layerName with kruizeLayer.getLayerName()
LayerService-->>Client: 400 LAYER_NAME_MISMATCH (if mismatch)
LayerService->>LayerValidation: validate(kruizeLayer)
LayerValidation-->>LayerService: ValidationOutputData
LayerService-->>Client: 400 VALIDATION_FAILED (if not success)
LayerService->>ExperimentDAOImpl: loadLayerByName(layerName)
ExperimentDAOImpl->>DB: SELECT_FROM_LAYER_BY_NAME
DB-->>ExperimentDAOImpl: List<KruizeLMLayerEntry>
ExperimentDAOImpl-->>LayerService: existingLayers
LayerService-->>Client: 404 LAYER_NOT_FOUND (if empty)
LayerService->>LayerService: convertToLayerEntry(kruizeLayer)
LayerService->>ExperimentDAOImpl: updateLayerToDB(KruizeLMLayerEntry)
ExperimentDAOImpl->>MetricsConfig: Timer.start(meterRegistry)
ExperimentDAOImpl->>DB: merge(kruizeLayerEntry) inside transaction
DB-->>ExperimentDAOImpl: commit or error
ExperimentDAOImpl->>MetricsConfig: stop(timerUpdateLayerDB)
ExperimentDAOImpl-->>LayerService: ValidationOutputData
alt update success
LayerService-->>Client: 200 JSON SUCCESS with UPDATE_LAYER_SUCCESS_MSG
else update failure
LayerService-->>Client: 500 JSON ERROR UPDATE_LAYER_TO_DB_FAILURE
end
Sequence diagram for the deleteLayer REST APIsequenceDiagram
actor Client
participant LayerService
participant ExperimentDAOImpl
participant DB
participant MetricsConfig
Client->>LayerService: HTTP DELETE /deleteLayer?name=layerName
LayerService->>LayerService: getParameter(AnalyzerConstants.LAYER_NAME)
LayerService-->>Client: 400 INVALID_LAYER_NAME (if missing/empty)
LayerService->>LayerService: validate query params using KruizeSupportedTypes.DELETE_LAYERS_QUERY_PARAMS_SUPPORTED
LayerService-->>Client: 400 INVALID_QUERY_PARAMS (if invalid)
LayerService->>ExperimentDAOImpl: loadLayerByName(layerName)
ExperimentDAOImpl->>DB: SELECT_FROM_LAYER_BY_NAME
DB-->>ExperimentDAOImpl: List<KruizeLMLayerEntry>
ExperimentDAOImpl-->>LayerService: existingLayers
LayerService-->>Client: 404 DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME (if empty)
LayerService->>ExperimentDAOImpl: deleteLayerByName(layerName)
ExperimentDAOImpl->>DB: DELETE_FROM_LAYER_BY_NAME
DB-->>ExperimentDAOImpl: deletedCount
ExperimentDAOImpl-->>LayerService: ValidationOutputData
alt delete success
LayerService-->>Client: 200 JSON SUCCESS with DELETE_LAYER_SUCCESS_MSG
else delete failure
LayerService-->>Client: 500 JSON ERROR with message
end
Updated class diagram for Layer CRUD, validation, DAO, and metricsclassDiagram
class LayerService {
<<Servlet>>
+void doPost(HttpServletRequest request, HttpServletResponse response)
+void doGet(HttpServletRequest request, HttpServletResponse response)
+void doPut(HttpServletRequest request, HttpServletResponse response)
+void doDelete(HttpServletRequest request, HttpServletResponse response)
-KruizeLMLayerEntry convertToLayerEntry(KruizeLayer kruizeLayer)
-void sendSuccessResponse(HttpServletResponse response, String message, int httpStatusCode)
-void sendErrorResponse(HttpServletResponse response, Exception e, int httpStatusCode, String message)
-Gson createGsonObject()
-void sendJsonResponse(HttpServletResponse response, String message, int statusCode, String status)
}
class Converters {
+static KruizeLayer convertInputJSONToCreateLayer(String inputData)
-static String getRequiredString(JSONObject json, String fieldName, String objectType, String fieldDescription)
}
class Tunable {
-String name
-String upperBoundValue
-String lowerBoundValue
-Double step
-List~String~ choices
+void validate() throws InvalidBoundsException
-void validateBounds() throws InvalidBoundsException
-void validateCategorical()
}
class ExperimentDAO {
<<interface>>
+ValidationOutputData addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry)
+ValidationOutputData updateLayerToDB(KruizeLMLayerEntry kruizeLayerEntry)
+ValidationOutputData deleteLayerByName(String layerName)
+List~KruizeLMLayerEntry~ loadAllLayers() throws Exception
+List~KruizeLMLayerEntry~ loadLayerByName(String layerName) throws Exception
}
class ExperimentDAOImpl {
+ValidationOutputData addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry)
+ValidationOutputData updateLayerToDB(KruizeLMLayerEntry kruizeLayerEntry)
+ValidationOutputData deleteLayerByName(String layerName)
+List~KruizeLMLayerEntry~ loadAllLayers() throws Exception
+List~KruizeLMLayerEntry~ loadLayerByName(String layerName) throws Exception
}
class MetricsConfig {
+static Timer timerAddLayerDB
+static Timer timerLoadAllLayers
+static Timer timerLoadLayerByName
+static Timer timerUpdateLayerDB
+static Timer timerDeleteLayerDB
+static Timer.Builder timerBAddLayerDB
+static Timer.Builder timerBLoadAllLayers
+static Timer.Builder timerBLoadLayerByName
+static Timer.Builder timerBUpdateLayerDB
+static Timer.Builder timerBDeleteLayerDB
+static MeterRegistry meterRegistry()
}
class KruizeSupportedTypes {
+static Set~String~ CREATE_LAYERS_QUERY_PARAMS_SUPPORTED
+static Set~String~ LIST_LAYERS_QUERY_PARAMS_SUPPORTED
+static Set~String~ UPDATE_LAYERS_QUERY_PARAMS_SUPPORTED
+static Set~String~ DELETE_LAYERS_QUERY_PARAMS_SUPPORTED
}
class AnalyzerErrorConstants_APIErrors_UpdateLayerAPI {
+static String INVALID_LAYER_NAME
+static String LAYER_NOT_FOUND
+static String INVALID_LAYER_JSON
+static String UPDATE_LAYER_TO_DB_FAILURE
+static String LAYER_NAME_MISMATCH
+static String VALIDATION_FAILED
+static String UNEXPECTED_ERROR
+static String INVALID_QUERY_PARAMS
+static String MISSING_REQUIRED_FIELD
}
class AnalyzerErrorConstants_APIErrors_DeleteLayerAPI {
+static String DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME
+static String DELETE_LAYER_ENTRY_ERROR_MSG
+static String INVALID_LAYER_NAME
+static String UNEXPECTED_ERROR
+static String INVALID_QUERY_PARAMS
}
class KruizeConstants_LayerAPIMessages {
+static String CREATE_LAYER_SUCCESS_MSG
+static String UPDATE_LAYER_SUCCESS_MSG
+static String DELETE_LAYER_SUCCESS_MSG
+static String VIEW_LAYERS_MSG
+static String ADD_LAYER_TO_DB
+static String UPDATE_LAYER_TO_DB
+static String DELETE_LAYER_FROM_DB
+static String LOAD_LAYER_FAILURE
+static String LOAD_ALL_LAYERS_FAILURE
}
class DBConstants_SQLQUERY {
+static String SELECT_FROM_LAYER
+static String SELECT_FROM_LAYER_BY_NAME
+static String DELETE_FROM_LAYER_BY_NAME
}
class AnalyzerConstants {
+static String LAYER_NAME
+static String API_VERSION
+static String KIND
}
class ServerContext {
+static String CREATE_LAYER
+static String LIST_LAYERS
+static String UPDATE_LAYER
+static String DELETE_LAYER
}
class KruizeLayer
class KruizeLMLayerEntry
class LayerValidation {
+ValidationOutputData validate(KruizeLayer kruizeLayer)
}
class ValidationOutputData {
+boolean success
+String message
+boolean isSuccess()
+void setSuccess(boolean success)
+String getMessage()
+void setMessage(String message)
}
LayerService --> Converters : uses
LayerService --> LayerValidation : uses
LayerService --> KruizeLayer : creates/reads
LayerService --> KruizeLMLayerEntry : converts
LayerService --> ExperimentDAO : depends on
ExperimentDAOImpl ..|> ExperimentDAO
ExperimentDAOImpl --> KruizeLMLayerEntry : persists
ExperimentDAOImpl --> DBConstants_SQLQUERY : uses
ExperimentDAOImpl --> MetricsConfig : records metrics
Tunable --> AnalyzerErrorConstants_APIErrors_UpdateLayerAPI : uses messages via CreateLayerAPI
Converters --> Tunable : constructs
LayerService --> KruizeConstants_LayerAPIMessages : uses messages
LayerService --> AnalyzerErrorConstants_APIErrors_UpdateLayerAPI : uses errors
LayerService --> AnalyzerErrorConstants_APIErrors_DeleteLayerAPI : uses errors
LayerService --> KruizeSupportedTypes : validates query params
LayerService --> AnalyzerConstants : uses keys
LayerService --> ServerContext : registered under paths
ServerContext --> LayerService : mapped servlet
MetricsConfig --> Timer : uses (Micrometer)
File-Level Changes
Possibly linked issues
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 4 issues, and left some high level feedback:
- In
Converters.convertInputJSONToCreateLayer, usingoptStringfordetailswill turn an explicit JSONnullinto the literal string "null"; if you intend to preservenullasnull, consider checkingisNull()and only callinggetStringwhen the field is non-null. - The new DB metric builders
timerBDeleteLayerDB/timerDeleteLayerDBare never used inExperimentDAOImpl.deleteLayerByName; align this method withupdateLayerToDBby starting/stopping a timer so the delete-layer DB path is actually instrumented. - In
LayerService.doDelete, you first load the layer by name and then calldeleteLayerByName; consider removing the pre-check and relying on the delete count to determine existence to avoid an extra DB round-trip and a duplicated not-found check.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Converters.convertInputJSONToCreateLayer`, using `optString` for `details` will turn an explicit JSON `null` into the literal string "null"; if you intend to preserve `null` as `null`, consider checking `isNull()` and only calling `getString` when the field is non-null.
- The new DB metric builders `timerBDeleteLayerDB` / `timerDeleteLayerDB` are never used in `ExperimentDAOImpl.deleteLayerByName`; align this method with `updateLayerToDB` by starting/stopping a timer so the delete-layer DB path is actually instrumented.
- In `LayerService.doDelete`, you first load the layer by name and then call `deleteLayerByName`; consider removing the pre-check and relying on the delete count to determine existence to avoid an extra DB round-trip and a duplicated not-found check.
## Individual Comments
### Comment 1
<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:** Delete-layer DB operation is missing timer instrumentation despite metrics builders being defined.
`MetricsConfig` defines `timerDeleteLayerDB` and `timerBDeleteLayerDB`, but `deleteLayerByName` doesn’t start/stop a `Timer.Sample` or tag `timerBDeleteLayerDB` like `addLayerToDB`/`updateLayerToDB`. This leaves delete operations out of DB metrics. Please wrap the delete logic in a `Timer.Sample` and tag `timerBDeleteLayerDB` (e.g. with `status`) to keep CRUD observability consistent.
Suggested implementation:
```java
@Override
public ValidationOutputData deleteLayerByName(String layerName) {
// Start delete-layer DB timer and initialize status for metrics
Timer.Sample deleteLayerDBTimerSample = Timer.start(MetricsConfig.meterRegistry);
String deleteLayerDBStatus = KruizeConstants.DB_OPERATION_STATUS.FAILED;
ValidationOutputData validationOutputData = new ValidationOutputData(false, null, null);
Transaction tx = null;
try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {
try {
tx = session.beginTransaction();
Query query = session.createQuery(DBConstants.SQLQUERY.DELETE_FROM_LAYER_BY_NAME, null);
query.setParameter("layerName", layerName);
int deletedCount = query.executeUpdate();
if (deletedCount == 0) {
deleteLayerDBStatus = KruizeConstants.DB_OPERATION_STATUS.NOT_FOUND;
```
To fully wire up the metrics, you should also:
1. Ensure the following imports exist at the top of the file (or adjust to match your existing Micrometer setup):
- `import io.micrometer.core.instrument.Timer;`
- `import com.autotune.common.metrics.MetricsConfig;` (or the correct package where `MetricsConfig` lives)
- `import com.autotune.utils.KruizeConstants;` (if not already imported)
2. In the `else` branch corresponding to `if (deletedCount == 0)` (after the delete operation succeeds), set:
- `deleteLayerDBStatus = KruizeConstants.DB_OPERATION_STATUS.SUCCESS;`
3. In the `catch` block(s) of this method, leave `deleteLayerDBStatus` as `FAILED` or explicitly set it to `FAILED` if you prefer being explicit.
4. In the `finally` block of this method (after the outer `try (Session ...)`), record and stop the timers, following the conventions used in `addLayerToDB` / `updateLayerToDB`. For example, mirroring those methods:
- `MetricsConfig.timerBDeleteLayerDB.tags("status", deleteLayerDBStatus).register(MetricsConfig.meterRegistry);`
- `deleteLayerDBTimerSample.stop(MetricsConfig.timerDeleteLayerDB);`
Match the exact tagging keys/values and registry usage to how `timerBAddLayerDB` / `timerBUpdateLayerDB` and `timerAddLayerDB` / `timerUpdateLayerDB` are used elsewhere in `ExperimentDAOImpl`.
</issue_to_address>
### Comment 2
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_update_layer.py" line_range="41-50" />
<code_context>
- delete_layer_from_db(layer_name)
-
@pytest.mark.layers
@pytest.mark.sanity
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests for empty/whitespace-only request body on updateLayer
Since `updateLayer` now returns `AnalyzerErrorConstants.APIErrors.UpdateLayerAPI.INVALID_LAYER_JSON` for empty or whitespace-only bodies, please add a negative test that exercises this case (e.g., `update_layer(name, tmp_file)` with an empty/blank body or a raw `requests.put` with no body) and asserts the status code and `UPDATE_LAYER_INVALID_JSON_MSG`. This will make the new behavior explicit and guard against regressions.
Suggested implementation:
```python
mandatory_fields = [
("apiVersion", ERROR_STATUS_CODE, ERROR_STATUS),
("kind", ERROR_STATUS_CODE, ERROR_STATUS),
("metadata", ERROR_STATUS_CODE, ERROR_STATUS),
("layer_name", ERROR_STATUS_CODE, ERROR_STATUS),
("layer_presence", ERROR_STATUS_CODE, ERROR_STATUS),
("tunables", ERROR_STATUS_CODE, ERROR_STATUS),
]
@pytest.mark.layers
@pytest.mark.sanity
def test_update_layer_empty_body_invalid_json(cluster_type, tmp_path):
"""
Test Description: Verify updateLayer returns INVALID_LAYER_JSON for an empty request body.
"""
form_kruize_url(cluster_type)
empty_file = tmp_path / "empty_layer.json"
empty_file.write_text("")
# update_layer is expected to send the contents of empty_file as the request body
response = update_layer("test-layer-empty-body", empty_file)
assert response.status_code == requests.codes.bad_request
assert response.json()["message"] == UPDATE_LAYER_INVALID_JSON_MSG
@pytest.mark.layers
@pytest.mark.sanity
def test_update_layer_whitespace_body_invalid_json(cluster_type, tmp_path):
"""
Test Description: Verify updateLayer returns INVALID_LAYER_JSON for a whitespace-only request body.
"""
form_kruize_url(cluster_type)
whitespace_file = tmp_path / "whitespace_layer.json"
whitespace_file.write_text(" \n\t ")
# update_layer is expected to send the contents of whitespace_file as the request body
response = update_layer("test-layer-whitespace-body", whitespace_file)
assert response.status_code == requests.codes.bad_request
assert response.json()["message"] == UPDATE_LAYER_INVALID_JSON_MSG
@pytest.mark.layers
@pytest.mark.sanity
def test_update_layer(cluster_type):
```
To make these tests run successfully, verify and adjust the following in the same file (or appropriate shared test utilities):
1. Ensure `requests` is imported if it is not already:
- `import requests`
2. Ensure `update_layer` is imported/available and that its signature matches the usage `update_layer(layer_name, json_file_path)`.
- If the helper currently takes different parameters (e.g., includes `cluster_type` or URL), either:
- adapt the tests to that signature, or
- create a thin wrapper `update_layer(name, path)` that uses the existing helper.
3. Confirm that `UPDATE_LAYER_INVALID_JSON_MSG` is imported from wherever it is defined (likely a constants module tied to `AnalyzerErrorConstants.APIErrors.UpdateLayerAPI.INVALID_LAYER_JSON`).
4. If `update_layer` does not use the file path directly and instead expects raw JSON or bytes, adjust the test to:
- open the file and pass `""` or `" \n\t "` as the body, or
- call the underlying `requests.put` directly using the correct updateLayer URL and parameters.
</issue_to_address>
### Comment 3
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_delete_layer.py" line_range="40-49" />
<code_context>
- delete_layer_from_db(layer_name)
-
@pytest.mark.layers
@pytest.mark.sanity
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for invalid query parameters on deleteLayer
The new deleteLayer behavior validates query parameters against `DELETE_LAYERS_QUERY_PARAMS_SUPPORTED` and returns `DeleteLayerAPI.INVALID_QUERY_PARAMS` for extras, but current negative tests only cover invalid names. Please add a test calling `/deleteLayer` with `name=foo&extra=bar` that asserts a 400 response and the `INVALID_QUERY_PARAMS` message to cover this branch.
Suggested implementation:
```python
# A. POSITIVE SCENARIOS - Delete Layer Successfully (4 tests)
# Note: We don't test sequential deletion of multiple layers separately because
# test_delete_layer_verify_removal already covers deletion behavior with multiple
# layers (verifying one is removed while others remain).
# =============================================================================
@pytest.mark.layers
@pytest.mark.sanity
def test_delete_layer_success(cluster_type):
"""
Test Description: This test validates deleteLayer API successfully deletes an existing layer
"""
form_kruize_url(cluster_type)
input_json_file = layer_dir / "container-config.json"
with open(input_json_file, "r") as f:
layer_data = json.load(f)
=======
# =============================================================================
# A. POSITIVE SCENARIOS - Delete Layer Successfully (4 tests)
# Note: We don't test sequential deletion of multiple layers separately because
# test_delete_layer_verify_removal already covers deletion behavior with multiple
# layers (verifying one is removed while others remain).
# =============================================================================
@pytest.mark.layers
@pytest.mark.sanity
def test_delete_layer_success(cluster_type):
"""
Test Description: This test validates deleteLayer API successfully deletes an existing layer
"""
form_kruize_url(cluster_type)
input_json_file = layer_dir / "container-config.json"
with open(input_json_file, "r") as f:
layer_data = json.load(f)
# =============================================================================
# B. NEGATIVE SCENARIOS - Delete Layer Failures (invalid input, missing layer, etc.)
# =============================================================================
@pytest.mark.layers
@pytest.mark.sanity
def test_delete_layer_invalid_query_params(cluster_type):
"""
Test Description: This test validates deleteLayer API rejects unsupported query parameters
and returns INVALID_QUERY_PARAMS with HTTP 400.
"""
form_kruize_url(cluster_type)
# Use a syntactically valid layer name and add an extra, unsupported query parameter
params = {
"name": "test-layer-invalid-query-param",
"extra": "unsupported",
}
response = requests.delete(
f"{get_kruize_url(cluster_type)}/deleteLayer",
params=params,
timeout=DEFAULT_TIMEOUT,
)
assert response.status_code == HTTPStatus.BAD_REQUEST
response_json = response.json()
# Ensure we are hitting the INVALID_QUERY_PARAMS branch introduced in deleteLayer
assert response_json.get("message") == DeleteLayerAPI.INVALID_QUERY_PARAMS.value
```
I assumed the presence of:
1. `requests`, `HTTPStatus`, `DEFAULT_TIMEOUT`, `get_kruize_url`, and `DeleteLayerAPI` already being imported/used elsewhere in this test module (they are typically used in the existing deleteLayer tests). If any of these are not imported, add appropriate imports at the top of the file, reusing existing conventions from other REST API test files in this repo.
2. A section `B. NEGATIVE SCENARIOS` did not previously exist; if you already have negative deleteLayer tests, you may want to move this new test into that section instead of introducing a new header.
3. If the rest of the tests use a helper wrapper (e.g., `delete_layer(...)`) instead of calling `requests.delete` directly, adapt this test to call that helper with `params` containing an extra key so it still exercises the `INVALID_QUERY_PARAMS` branch and asserts the same 400 + message.
</issue_to_address>
### Comment 4
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_list_layers.py" line_range="31-33" />
<code_context>
- delete_layer_from_db(layer_name)
-
@pytest.mark.layers
@pytest.mark.sanity
</code_context>
<issue_to_address>
**nitpick (testing):** Clarify the intent and stability of the performance-oriented listLayers test
This test currently measures response time but doesn’t assert on it, which keeps it stable. However, the “performance” framing in the name/docstring may lead others to add strict timing checks later and introduce flakiness. Please either clarify in the docstring that it only validates correctness/basic scalability (no latency SLO), or, if a loose upper bound is desired, add that timing assertion now so expectations are explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -689,6 +689,83 @@ public ValidationOutputData addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) { | |||
| return validationOutputData; | |||
Contributor
There was a problem hiding this comment.
suggestion: Delete-layer DB operation is missing timer instrumentation despite metrics builders being defined.
MetricsConfig defines timerDeleteLayerDB and timerBDeleteLayerDB, but deleteLayerByName doesn’t start/stop a Timer.Sample or tag timerBDeleteLayerDB like addLayerToDB/updateLayerToDB. This leaves delete operations out of DB metrics. Please wrap the delete logic in a Timer.Sample and tag timerBDeleteLayerDB (e.g. with status) to keep CRUD observability consistent.
Suggested implementation:
@Override
public ValidationOutputData deleteLayerByName(String layerName) {
// Start delete-layer DB timer and initialize status for metrics
Timer.Sample deleteLayerDBTimerSample = Timer.start(MetricsConfig.meterRegistry);
String deleteLayerDBStatus = KruizeConstants.DB_OPERATION_STATUS.FAILED;
ValidationOutputData validationOutputData = new ValidationOutputData(false, null, null);
Transaction tx = null;
try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {
try {
tx = session.beginTransaction();
Query query = session.createQuery(DBConstants.SQLQUERY.DELETE_FROM_LAYER_BY_NAME, null);
query.setParameter("layerName", layerName);
int deletedCount = query.executeUpdate();
if (deletedCount == 0) {
deleteLayerDBStatus = KruizeConstants.DB_OPERATION_STATUS.NOT_FOUND;To fully wire up the metrics, you should also:
-
Ensure the following imports exist at the top of the file (or adjust to match your existing Micrometer setup):
import io.micrometer.core.instrument.Timer;import com.autotune.common.metrics.MetricsConfig;(or the correct package whereMetricsConfiglives)import com.autotune.utils.KruizeConstants;(if not already imported)
-
In the
elsebranch corresponding toif (deletedCount == 0)(after the delete operation succeeds), set:deleteLayerDBStatus = KruizeConstants.DB_OPERATION_STATUS.SUCCESS;
-
In the
catchblock(s) of this method, leavedeleteLayerDBStatusasFAILEDor explicitly set it toFAILEDif you prefer being explicit. -
In the
finallyblock of this method (after the outertry (Session ...)), record and stop the timers, following the conventions used inaddLayerToDB/updateLayerToDB. For example, mirroring those methods:MetricsConfig.timerBDeleteLayerDB.tags("status", deleteLayerDBStatus).register(MetricsConfig.meterRegistry);deleteLayerDBTimerSample.stop(MetricsConfig.timerDeleteLayerDB);
Match the exact tagging keys/values and registry usage to how
timerBAddLayerDB/timerBUpdateLayerDBandtimerAddLayerDB/timerUpdateLayerDBare used elsewhere inExperimentDAOImpl.
Comment on lines
+41
to
+50
| @pytest.mark.layers | ||
| @pytest.mark.sanity | ||
| def test_update_layer(cluster_type): | ||
| """ | ||
| Test Description: This test validates the response status code of updateLayer API by passing a | ||
| valid input for the layer update | ||
| """ | ||
| form_kruize_url(cluster_type) | ||
|
|
||
| input_json_file = layer_dir / "container-config.json" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add explicit tests for empty/whitespace-only request body on updateLayer
Since updateLayer now returns AnalyzerErrorConstants.APIErrors.UpdateLayerAPI.INVALID_LAYER_JSON for empty or whitespace-only bodies, please add a negative test that exercises this case (e.g., update_layer(name, tmp_file) with an empty/blank body or a raw requests.put with no body) and asserts the status code and UPDATE_LAYER_INVALID_JSON_MSG. This will make the new behavior explicit and guard against regressions.
Suggested implementation:
mandatory_fields = [
("apiVersion", ERROR_STATUS_CODE, ERROR_STATUS),
("kind", ERROR_STATUS_CODE, ERROR_STATUS),
("metadata", ERROR_STATUS_CODE, ERROR_STATUS),
("layer_name", ERROR_STATUS_CODE, ERROR_STATUS),
("layer_presence", ERROR_STATUS_CODE, ERROR_STATUS),
("tunables", ERROR_STATUS_CODE, ERROR_STATUS),
]
@pytest.mark.layers
@pytest.mark.sanity
def test_update_layer_empty_body_invalid_json(cluster_type, tmp_path):
"""
Test Description: Verify updateLayer returns INVALID_LAYER_JSON for an empty request body.
"""
form_kruize_url(cluster_type)
empty_file = tmp_path / "empty_layer.json"
empty_file.write_text("")
# update_layer is expected to send the contents of empty_file as the request body
response = update_layer("test-layer-empty-body", empty_file)
assert response.status_code == requests.codes.bad_request
assert response.json()["message"] == UPDATE_LAYER_INVALID_JSON_MSG
@pytest.mark.layers
@pytest.mark.sanity
def test_update_layer_whitespace_body_invalid_json(cluster_type, tmp_path):
"""
Test Description: Verify updateLayer returns INVALID_LAYER_JSON for a whitespace-only request body.
"""
form_kruize_url(cluster_type)
whitespace_file = tmp_path / "whitespace_layer.json"
whitespace_file.write_text(" \n\t ")
# update_layer is expected to send the contents of whitespace_file as the request body
response = update_layer("test-layer-whitespace-body", whitespace_file)
assert response.status_code == requests.codes.bad_request
assert response.json()["message"] == UPDATE_LAYER_INVALID_JSON_MSG
@pytest.mark.layers
@pytest.mark.sanity
def test_update_layer(cluster_type):To make these tests run successfully, verify and adjust the following in the same file (or appropriate shared test utilities):
- Ensure
requestsis imported if it is not already:import requests
- Ensure
update_layeris imported/available and that its signature matches the usageupdate_layer(layer_name, json_file_path).- If the helper currently takes different parameters (e.g., includes
cluster_typeor URL), either:- adapt the tests to that signature, or
- create a thin wrapper
update_layer(name, path)that uses the existing helper.
- If the helper currently takes different parameters (e.g., includes
- Confirm that
UPDATE_LAYER_INVALID_JSON_MSGis imported from wherever it is defined (likely a constants module tied toAnalyzerErrorConstants.APIErrors.UpdateLayerAPI.INVALID_LAYER_JSON). - If
update_layerdoes not use the file path directly and instead expects raw JSON or bytes, adjust the test to:- open the file and pass
""or" \n\t "as the body, or - call the underlying
requests.putdirectly using the correct updateLayer URL and parameters.
- open the file and pass
Comment on lines
+40
to
+49
| @pytest.mark.layers | ||
| @pytest.mark.sanity | ||
| def test_delete_layer_success(cluster_type): | ||
| """ | ||
| Test Description: This test validates deleteLayer API successfully deletes an existing layer | ||
| """ | ||
| form_kruize_url(cluster_type) | ||
|
|
||
| input_json_file = layer_dir / "container-config.json" | ||
| with open(input_json_file, "r") as f: |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for invalid query parameters on deleteLayer
The new deleteLayer behavior validates query parameters against DELETE_LAYERS_QUERY_PARAMS_SUPPORTED and returns DeleteLayerAPI.INVALID_QUERY_PARAMS for extras, but current negative tests only cover invalid names. Please add a test calling /deleteLayer with name=foo&extra=bar that asserts a 400 response and the INVALID_QUERY_PARAMS message to cover this branch.
Suggested implementation:
# A. POSITIVE SCENARIOS - Delete Layer Successfully (4 tests)
# Note: We don't test sequential deletion of multiple layers separately because
# test_delete_layer_verify_removal already covers deletion behavior with multiple
# layers (verifying one is removed while others remain).
# =============================================================================
@pytest.mark.layers
@pytest.mark.sanity
def test_delete_layer_success(cluster_type):
"""
Test Description: This test validates deleteLayer API successfully deletes an existing layer
"""
form_kruize_url(cluster_type)
input_json_file = layer_dir / "container-config.json"
with open(input_json_file, "r") as f:
layer_data = json.load(f)
=======
# =============================================================================
# A. POSITIVE SCENARIOS - Delete Layer Successfully (4 tests)
# Note: We don't test sequential deletion of multiple layers separately because
# test_delete_layer_verify_removal already covers deletion behavior with multiple
# layers (verifying one is removed while others remain).
# =============================================================================
@pytest.mark.layers
@pytest.mark.sanity
def test_delete_layer_success(cluster_type):
"""
Test Description: This test validates deleteLayer API successfully deletes an existing layer
"""
form_kruize_url(cluster_type)
input_json_file = layer_dir / "container-config.json"
with open(input_json_file, "r") as f:
layer_data = json.load(f)
# =============================================================================
# B. NEGATIVE SCENARIOS - Delete Layer Failures (invalid input, missing layer, etc.)
# =============================================================================
@pytest.mark.layers
@pytest.mark.sanity
def test_delete_layer_invalid_query_params(cluster_type):
"""
Test Description: This test validates deleteLayer API rejects unsupported query parameters
and returns INVALID_QUERY_PARAMS with HTTP 400.
"""
form_kruize_url(cluster_type)
# Use a syntactically valid layer name and add an extra, unsupported query parameter
params = {
"name": "test-layer-invalid-query-param",
"extra": "unsupported",
}
response = requests.delete(
f"{get_kruize_url(cluster_type)}/deleteLayer",
params=params,
timeout=DEFAULT_TIMEOUT,
)
assert response.status_code == HTTPStatus.BAD_REQUEST
response_json = response.json()
# Ensure we are hitting the INVALID_QUERY_PARAMS branch introduced in deleteLayer
assert response_json.get("message") == DeleteLayerAPI.INVALID_QUERY_PARAMS.valueI assumed the presence of:
requests,HTTPStatus,DEFAULT_TIMEOUT,get_kruize_url, andDeleteLayerAPIalready being imported/used elsewhere in this test module (they are typically used in the existing deleteLayer tests). If any of these are not imported, add appropriate imports at the top of the file, reusing existing conventions from other REST API test files in this repo.- A section
B. NEGATIVE SCENARIOSdid not previously exist; if you already have negative deleteLayer tests, you may want to move this new test into that section instead of introducing a new header. - If the rest of the tests use a helper wrapper (e.g.,
delete_layer(...)) instead of callingrequests.deletedirectly, adapt this test to call that helper withparamscontaining an extra key so it still exercises theINVALID_QUERY_PARAMSbranch and asserts the same 400 + message.
Comment on lines
31
to
33
| @pytest.mark.layers | ||
| @pytest.mark.negative | ||
| def test_list_layers_when_no_layers_exist(cluster_type): |
Contributor
There was a problem hiding this comment.
nitpick (testing): Clarify the intent and stability of the performance-oriented listLayers test
This test currently measures response time but doesn’t assert on it, which keeps it stable. However, the “performance” framing in the name/docstring may lead others to add strict timing checks later and introduce flakiness. Please either clarify in the docstring that it only validates correctness/basic scalability (no latency SLO), or, if a loose upper bound is desired, add that timing assertion now so expectations are explicit.
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
1 participant
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
Please describe the issue or feature and the summary of changes made to fix this.
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 and enhanced validation for layer management APIs, along with comprehensive tests for create, list, update, and delete operations.
New Features:
Bug Fixes:
Enhancements:
Tests: