[BULK-PROFILE]-8: Add the BulkProfileService servlet - #1994
#1994[BULK-PROFILE]-8: Add the BulkProfileService servlet#1994bharathappali wants to merge 21 commits intokruize:runtimes-iirjkruize/autotune:runtimes-iirjfrom bharathappali:feat-bulk-prof-8bharathappali/autotune:feat-bulk-prof-8Copy head branch name to clipboard
Conversation
Reviewer's GuideAdds a new BulkProfile REST servlet, service-layer DTOs, and DB entity/DAO support to manage bulk profile configurations with CRUD operations and webhook notifications. Sequence diagram for BulkProfileService update with webhooksequenceDiagram
actor Client
participant BulkProfileService
participant ExperimentDAO
participant KruizeBulkProfileEntry
participant GenericRestApiClient
participant WebhookEndpoint
Client->>BulkProfileService: doPut(profile_name)
BulkProfileService->>ExperimentDAO: loadBulkProfileByName(profileName)
ExperimentDAO-->>BulkProfileService: KruizeBulkProfileEntry
BulkProfileService->>KruizeBulkProfileEntry: toBulkProfile()
BulkProfileService->>BulkProfileService: apply BulkProfileUpdateRequest
BulkProfileService->>KruizeBulkProfileEntry: fromBulkProfile(BulkProfile)
BulkProfileService->>ExperimentDAO: updateBulkProfileToDB(KruizeBulkProfileEntry)
ExperimentDAO-->>BulkProfileService: ValidationOutputData
alt [update successful]
BulkProfileService->>GenericRestApiClient: callKruizeAPI(payload)
GenericRestApiClient->>WebhookEndpoint: POST payload
WebhookEndpoint-->>GenericRestApiClient: 200 OK
GenericRestApiClient-->>BulkProfileService: HttpResponseWrapper
end
BulkProfileService-->>Client: 200 OK with BulkProfile
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The HQL constants for bulk profiles mix entity field names and column names (e.g.
LOAD_ALL_ENABLED_BULK_PROFILESandDELETE_BULK_PROFILE_BY_NAMEuseprofile_nameinstead of theKruizeBulkProfileEntryfieldprofileName), which will break the queries; they should consistently use entity property names in JPQL. - The
BulkProfile.descriptionfield is not mapped toKruizeBulkProfileEntryor thekruize_bulk_profiletable, so descriptions will be silently dropped on persistence and reload; add a column and mapping if description is meant to round-trip. - The DB migration defines
recommendation_settings JSONB NOT NULL, butBulkProfileServicedoes not validate thatrecommendation_settingsis present andKruizeBulkProfileEntry.fromBulkProfileonly sets it when non-null, so inserts/updates can violate the NOT NULL constraint; either relax the schema or enforce/normalize a non-null value in the service layer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The HQL constants for bulk profiles mix entity field names and column names (e.g. `LOAD_ALL_ENABLED_BULK_PROFILES` and `DELETE_BULK_PROFILE_BY_NAME` use `profile_name` instead of the `KruizeBulkProfileEntry` field `profileName`), which will break the queries; they should consistently use entity property names in JPQL.
- The `BulkProfile.description` field is not mapped to `KruizeBulkProfileEntry` or the `kruize_bulk_profile` table, so descriptions will be silently dropped on persistence and reload; add a column and mapping if description is meant to round-trip.
- The DB migration defines `recommendation_settings JSONB NOT NULL`, but `BulkProfileService` does not validate that `recommendation_settings` is present and `KruizeBulkProfileEntry.fromBulkProfile` only sets it when non-null, so inserts/updates can violate the NOT NULL constraint; either relax the schema or enforce/normalize a non-null value in the service layer.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/services/BulkProfileService.java" line_range="41-50" />
<code_context>
+ this.updatedAt = updatedAt;
+ }
+
+ /**
+ * Cluster configuration within a bulk profile
+ */
</code_context>
<issue_to_address>
**suggestion (typo):** REST path documentation does not match the registered servlet context
The Javadoc mentions `/bulkProfile`, but `ServerContext.BULK_PROFILE_SERVICE` is registered at `ROOT_CONTEXT + "bulkProfiles"` (plural). Please align the documented path with the actual servlet mapping.
Suggested implementation:
```java
* REST path: {@code /bulkProfiles}
```
If the Javadoc mentions `/bulkProfile` in multiple places (e.g., examples, descriptions, or method-level documentation), apply the same string replacement there as well to ensure all documented paths consistently use `/bulkProfiles`, matching `ServerContext.BULK_PROFILE_SERVICE` at `ROOT_CONTEXT + "bulkProfiles"`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /** | ||
| * REST API service for Bulk Profile management | ||
| * Provides CRUD operations for bulk profiles and webhook notifications | ||
| */ | ||
| @WebServlet(asyncSupported = true) | ||
| public class BulkProfileService extends HttpServlet { | ||
| private static final long serialVersionUID = 1L; | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(BulkProfileService.class); | ||
| private static final ObjectMapper objectMapper = new ObjectMapper(); | ||
| private ExperimentDAO experimentDAO; |
There was a problem hiding this comment.
suggestion (typo): REST path documentation does not match the registered servlet context
The Javadoc mentions /bulkProfile, but ServerContext.BULK_PROFILE_SERVICE is registered at ROOT_CONTEXT + "bulkProfiles" (plural). Please align the documented path with the actual servlet mapping.
Suggested implementation:
* REST path: {@code /bulkProfiles}If the Javadoc mentions /bulkProfile in multiple places (e.g., examples, descriptions, or method-level documentation), apply the same string replacement there as well to ensure all documented paths consistently use /bulkProfiles, matching ServerContext.BULK_PROFILE_SERVICE at ROOT_CONTEXT + "bulkProfiles".
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
…ulk profile Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
5ee445e to
92b7b82
Compare
Description
This PR is built on top of #1992
#1992 should be merged before this PR
This PR adds the servlet for exposing the CRUD API's for the bulk profile service
Fixes #1977
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 end-to-end support for managing bulk profiles, from REST API exposure through DAO and Hibernate integration down to database schema migration.
New Features:
Enhancements:
Build: