Remove unused APIs from Optimizer - #26
Open
gulati-aakriti wants to merge 4 commits into
Open
Conversation
Reviewer's GuideThis PR removes unused REST API resources and related status infrastructure, cleans up unused service methods and constants, and replaces resource-level tests with focused service-level tests for datasources and profiles. 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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The repeated mocking of
bulkSchedulerService.initialize()andkruizeStateService.refreshStateAndInstallProfiles()in bothDatasourceServiceTestandProfileServiceTestcould be extracted into a shared test utility or base class to reduce duplication and keep future changes to initialization behavior centralized. - The
// Made with Bobfooter comments in the new test classes are non-standard and may not align with the project's style; consider removing them to keep test files focused on their purpose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated mocking of `bulkSchedulerService.initialize()` and `kruizeStateService.refreshStateAndInstallProfiles()` in both `DatasourceServiceTest` and `ProfileServiceTest` could be extracted into a shared test utility or base class to reduce duplication and keep future changes to initialization behavior centralized.
- The `// Made with Bob` footer comments in the new test classes are non-standard and may not align with the project's style; consider removing them to keep test files focused on their purpose.
## Individual Comments
### Comment 1
<location path="src/test/java/com/kruize/optimizer/service/ProfileServiceTest.java" line_range="117" />
<code_context>
+ * - Returns empty list
+ */
+ @Test
+ void testGetMetadataProfiles_EmptyList() {
+ // Arrange
+ Response mockResponse = Response.status(400)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for non-400 `ClientWebApplicationException` to ensure they are handled as true errors
`testGetMetadataProfiles_EmptyList` covers the 400 "no profiles" case. Please also add a test where `kruizeClient.getMetadataProfiles(true)` throws a `ClientWebApplicationException` with a non-400 status (e.g., 500 or 503) and assert that a `KruizeServiceException` is thrown. This will verify and document the generic error-handling behavior for Kruize API HTTP errors.
Suggested implementation:
```java
// Assert
assertNotNull(profiles);
```
Please add the following test method to `ProfileServiceTest` (at class scope, alongside the existing `testGetMetadataProfiles_EmptyList`), and ensure the necessary imports are present:
```java
/**
* Test getMetadataProfiles when Kruize returns a non-400 HTTP error
*
* Test Description: Verifies that when Kruize returns a 5xx error,
* the service treats it as a true error and throws KruizeServiceException.
*
* Expected Output:
* - KruizeServiceException is thrown
*/
@Test
void testGetMetadataProfiles_Non400ClientWebApplicationException() {
// Arrange
Response mockResponse = Response.status(500)
.entity("Internal Server Error from Kruize")
.build();
ClientWebApplicationException exception = new ClientWebApplicationException(mockResponse);
when(kruizeClient.getMetadataProfiles(true)).thenThrow(exception);
// Act & Assert
assertThrows(KruizeServiceException.class, () -> profileService.getMetadataProfiles());
}
```
You may need to ensure the following imports exist (or equivalent, depending on your project conventions):
```java
import javax.ws.rs.core.Response;
import javax.ws.rs.ClientErrorException;
import javax.ws.rs.ClientWebApplicationException;
import static org.junit.jupiter.api.Assertions.assertThrows;
```
Adjust the imports to match whatever is already in `ProfileServiceTest` (e.g., if `ClientWebApplicationException` or `assertThrows` is already imported, do not duplicate them). This new test will ensure that non-400 `ClientWebApplicationException`s are surfaced as `KruizeServiceException` and will document the generic error-handling behavior for Kruize API HTTP errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
Key Changes
Removed 5 unused REST resource classes, eliminating redundant API endpoints for:
Datasource management
Layer configuration
Metadata profiles
Metric profiles
Status reporting
Service Layer Refactoring
Removed StatusService.java entirely
Cleaned up unused methods in DatasourceService, JobsService, and ProfileService
Removed KruizeStatus model class
Test Strategy Shift
Removed: 4 resource-level test files (839 lines)
Added: 2 service-level test files (502 lines)
Result: More focused testing at the service layer with better coverage
Constants Cleanup
Removed 22 lines of unused constants from OptimizerConstants.java
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 🎯
Summary by Sourcery
Remove unused optimizer REST resources and related status model while shifting tests from removed resource endpoints to the underlying service layer.
Bug Fixes:
Enhancements:
Tests: