Adding Scheduler to Create Bulk Jobs#6
Open
shekhar316 wants to merge 7 commits intokruize:mvp_demofrom
Open
Conversation
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Reviewer's GuideIntroduces a new Quarkus-based "kruize-optimizer" microservice with REST endpoints and support classes to interact with the Kruize API (datasources, profiles, layers, status), adds Maven wrapper and build config, centralizes constants and error handling, and wires application startup; it lays the plumbing for scheduled/bulk operations and webhook handling though those specific schedulers/endpoints are likely in a dependent module. Sequence diagram for listing metadata profiles via REST endpointsequenceDiagram
actor ApiClient
participant MetadataProfileResource
participant ProfileService
participant KruizeClient
participant KruizeAPI
ApiClient->>MetadataProfileResource: HTTP GET /kruize/metadataProfiles/list?verbose={v}
MetadataProfileResource->>ProfileService: getMetadataProfiles(verbose)
ProfileService->>KruizeClient: getMetadataProfiles(verbose)
KruizeClient->>KruizeAPI: GET /listMetadataProfiles?verbose={v}
KruizeAPI-->>KruizeClient: 200 OK, List<KruizeProfile>
KruizeClient-->>ProfileService: List<KruizeProfile>
ProfileService-->>MetadataProfileResource: List<KruizeProfile>
MetadataProfileResource-->>ApiClient: 200 OK, ApiResponse{status="success", data=List<KruizeProfile>}
Class diagram for new Kruize Optimizer microservice domain, services, resources, and clientclassDiagram
%% Domain models
class ApiResponse~T~ {
- String status
- String message
- T data
- List~String~ errors
+ ApiResponse()
+ ApiResponse(status String, message String)
+ ApiResponse(status String, message String, data T)
+ static success(message String, data T) ApiResponse~T~
+ static success(message String) ApiResponse~T~
+ static error(message String, errors List~String~) ApiResponse~T~
+ static error(message String) ApiResponse~T~
+ getStatus() String
+ setStatus(status String) void
+ getMessage() String
+ setMessage(message String) void
+ getData() T
+ setData(data T) void
+ getErrors() List~String~
+ setErrors(errors List~String~) void
}
class Datasource {
- String name
- String provider
- String url
- String serviceName
- String namespace
+ Datasource()
+ Datasource(name String, provider String, url String)
+ getName() String
+ setName(name String) void
+ getProvider() String
+ setProvider(provider String) void
+ getUrl() String
+ setUrl(url String) void
+ getServiceName() String
+ setServiceName(serviceName String) void
+ getNamespace() String
+ setNamespace(namespace String) void
}
class KruizeProfile {
- String name
- Metadata metadata
- String profileVersion
+ KruizeProfile()
+ KruizeProfile(name String, profileVersion String)
+ getName() String
+ setName(name String) void
+ getMetadata() Metadata
+ setMetadata(metadata Metadata) void
+ getProfileVersion() String
+ setProfileVersion(profileVersion String) void
}
class Metadata {
- String name
+ Metadata()
+ Metadata(name String)
+ getName() String
+ setName(name String) void
}
KruizeProfile o-- Metadata
class KruizeStatus {
- DatasourceStatus datasources
- ProfileStatus metadataProfiles
- ProfileStatus metricProfiles
- ProfileStatus layers
- List~String~ alerts
+ KruizeStatus()
+ getDatasources() DatasourceStatus
+ setDatasources(datasources DatasourceStatus) void
+ getMetadataProfiles() ProfileStatus
+ setMetadataProfiles(metadataProfiles ProfileStatus) void
+ getMetricProfiles() ProfileStatus
+ setMetricProfiles(metricProfiles ProfileStatus) void
+ getLayers() ProfileStatus
+ setLayers(layers ProfileStatus) void
+ getAlerts() List~String~
+ setAlerts(alerts List~String~) void
+ addAlert(alert String) void
}
class DatasourceStatus {
- int count
- List~Datasource~ list
+ DatasourceStatus()
+ DatasourceStatus(count int, list List~Datasource~)
+ getCount() int
+ setCount(count int) void
+ getList() List~Datasource~
+ setList(list List~Datasource~) void
}
class ProfileStatus {
- int count
- List~ProfileInfo~ installed
+ ProfileStatus()
+ ProfileStatus(count int, installed List~ProfileInfo~)
+ getCount() int
+ setCount(count int) void
+ getInstalled() List~ProfileInfo~
+ setInstalled(installed List~ProfileInfo~) void
}
class ProfileInfo {
- String name
- String profileVersion
+ ProfileInfo()
+ ProfileInfo(name String, profileVersion String)
+ getName() String
+ setName(name String) void
+ getProfileVersion() String
+ setProfileVersion(profileVersion String) void
}
KruizeStatus o-- DatasourceStatus
KruizeStatus o-- ProfileStatus
DatasourceStatus o-- Datasource
ProfileStatus o-- ProfileInfo
class DatasourceListResponse {
- List~Datasource~ datasources
+ DatasourceListResponse()
+ DatasourceListResponse(datasources List~Datasource~)
+ getDatasources() List~Datasource~
+ setDatasources(datasources List~Datasource~) void
}
%% Client
class KruizeClient {
<<interface>>
+ getDatasources() DatasourceListResponse
+ getMetadataProfiles(verbose boolean) List~KruizeProfile~
+ getMetricProfiles(verbose boolean) List~KruizeProfile~
+ getLayers() List~KruizeProfile~
+ createMetadataProfile(profileDefinition Object) String
+ createMetricProfile(profileDefinition Object) String
+ createLayer(layerDefinition Object) String
}
%% Services
class DatasourceService {
- KruizeClient kruizeClient
+ getDatasources() List~Datasource~
+ getDatasourceCount() int
+ isKruizeAvailable() boolean
}
class ProfileService {
- KruizeClient kruizeClient
- String profileBaseUrl
- ObjectMapper objectMapper
+ getMetadataProfiles(verbose boolean) List~KruizeProfile~
+ getMetricProfiles(verbose boolean) List~KruizeProfile~
+ getLayers() List~KruizeProfile~
+ installMissingProfiles(profileType String) List~String~
- installProfile(profileType String, profileName String) void
- loadProfileFromLocal(profileType String, profileName String) Object
- getResourcePath(profileType String, profileName String) String
- getAvailableProfilesFromLocal(profileType String) List~String~
- getInstalledProfiles(profileType String) List~KruizeProfile~
}
class StatusService {
- DatasourceService datasourceService
- ProfileService profileService
+ getSystemStatus() KruizeStatus
- convertToProfileInfo(profiles List~KruizeProfile~) List~ProfileInfo~
+ isKruizeHealthy() boolean
}
DatasourceService ..> KruizeClient
ProfileService ..> KruizeClient
StatusService ..> DatasourceService
StatusService ..> ProfileService
%% REST resources
class DatasourceResource {
- DatasourceService datasourceService
+ listDatasources() Response
}
class MetadataProfileResource {
- ProfileService profileService
+ listMetadataProfiles(verbose boolean) Response
+ installMetadataProfiles() Response
}
class MetricProfileResource {
- ProfileService profileService
+ listMetricProfiles(verbose boolean) Response
+ installMetricProfiles() Response
}
class LayerResource {
- ProfileService profileService
+ listLayers() Response
+ installLayers() Response
}
class StatusResource {
- StatusService statusService
+ getStatus() Response
}
DatasourceResource ..> DatasourceService
MetadataProfileResource ..> ProfileService
MetricProfileResource ..> ProfileService
LayerResource ..> ProfileService
StatusResource ..> StatusService
%% Exceptions and startup
class KruizeServiceException {
- int statusCode
+ KruizeServiceException(message String)
+ KruizeServiceException(message String, statusCode int)
+ KruizeServiceException(message String, cause Throwable)
+ KruizeServiceException(message String, cause Throwable, statusCode int)
+ getStatusCode() int
}
class GlobalExceptionMapper {
+ toResponse(exception Exception) Response
}
GlobalExceptionMapper ..> ApiResponse
KruizeServiceException --|> RuntimeException
class Startup {
+ onStart(ev StartupEvent) void
}
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 4 issues, and left some high level feedback:
- Several configuration fields/constants are currently unused (e.g.,
ProfileService.profileBaseUrl,GenericOptimizerConstants.STARTUP_MESSAGE); either wire them into the implementation (for profile retrieval/logging) or remove them to avoid confusion. - Service layers frequently wrap all exceptions in generic
RuntimeExceptionwhile you also introducedKruizeServiceExceptionand aGlobalExceptionMapper; consider standardizing on the custom exception and letting the mapper handle translation to HTTP responses to reduce duplicated try/catch and make error handling more consistent. - The hardcoded profile names in
ProfileService.getAvailableProfilesFromLocalwill require code changes whenever you add or rename profile files; consider discovering available profiles from the classpath/resource tree instead so it stays in sync with the actual JSON definitions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several configuration fields/constants are currently unused (e.g., `ProfileService.profileBaseUrl`, `GenericOptimizerConstants.STARTUP_MESSAGE`); either wire them into the implementation (for profile retrieval/logging) or remove them to avoid confusion.
- Service layers frequently wrap all exceptions in generic `RuntimeException` while you also introduced `KruizeServiceException` and a `GlobalExceptionMapper`; consider standardizing on the custom exception and letting the mapper handle translation to HTTP responses to reduce duplicated try/catch and make error handling more consistent.
- The hardcoded profile names in `ProfileService.getAvailableProfilesFromLocal` will require code changes whenever you add or rename profile files; consider discovering available profiles from the classpath/resource tree instead so it stays in sync with the actual JSON definitions.
## Individual Comments
### Comment 1
<location path="src/main/java/com/kruize/optimizer/service/ProfileService.java" line_range="106" />
<code_context>
+ * @param profileType type of profile (metadata, metric, layer)
+ * @return List of installation results
+ */
+ public List<String> installMissingProfiles(String profileType) {
+ List<String> results = new ArrayList<>();
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use a stronger type than raw String for `profileType` or validate early to avoid silent misconfiguration.
Because `profileType` is a free-form `String`, unexpected values just produce empty lists from `getInstalledProfiles()` and `getAvailableProfilesFromLocal()`, so `installMissingProfiles()` returns an empty `results` without any error. `installProfile()` only throws for bad types when there is an available profile, which won’t happen if the list is empty. Consider using an enum (or the existing `ProfileType` constants) or adding an explicit validation branch that fails fast on unknown types to surface misconfigurations instead of silently ignoring them.
Suggested implementation:
```java
/**
* Install missing profiles from local repository
*
* @param profileType type of profile (see {@link ProfileType})
* @return List of installation results
*/
```
```java
public List<String> installMissingProfiles(ProfileType profileType) {
```
1. Update all call sites of `installMissingProfiles(String profileType)` to pass a `ProfileType` value instead of a `String`, e.g. `installMissingProfiles(ProfileType.METADATA)`.
2. If the method body currently relies on `String` operations (e.g. `equalsIgnoreCase("metadata")`), replace them with `switch`/`if` on `ProfileType` enum values or direct comparisons (e.g. `profileType == ProfileType.METADATA`).
3. If any external interfaces (REST endpoints, config, etc.) provide a string profile type, convert/validate those strings at the boundary (e.g. `ProfileType.valueOf(...)` with appropriate error handling) before calling this method, so misconfigurations surface clearly.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/kruize/optimizer/service/ProfileService.java" line_range="45-46" />
<code_context>
+ @RestClient
+ KruizeClient kruizeClient;
+
+ @ConfigProperty(name = "kruize.profile.git.base-url", defaultValue = "local")
+ String profileBaseUrl;
+
+ private final ObjectMapper objectMapper = new ObjectMapper();
</code_context>
<issue_to_address>
**suggestion:** The `profileBaseUrl` config is defined but never used, which can be confusing for operators.
Right now profiles are always loaded via `loadProfileFromLocal()` (with hardcoded `configs/v1.0/...` paths) and `getAvailableProfilesFromLocal()` (with fixed names), so `kruize.profile.git.base-url` / `profileBaseUrl` never affect behavior. Please either integrate `profileBaseUrl` into profile path/discovery, or remove this config until it’s actually used to avoid a no-op setting.
Suggested implementation:
```java
private final ObjectMapper objectMapper = new ObjectMapper();
```
1. In `ProfileService.java`, remove the now-unused `org.eclipse.microprofile.config.inject.ConfigProperty` import if present.
2. Search the codebase for `"kruize.profile.git.base-url"` and remove any corresponding entries from configuration files (e.g., `application.properties`, `application.yml`) and documentation, or update them later when you actually wire this setting into profile discovery.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/kruize/optimizer/service/DatasourceService.java" line_range="57-59" />
<code_context>
+ datasources
+ )).build();
+
+ } catch (Exception e) {
+ LOG.error("Error fetching datasources", e);
+ return Response.status(Response.Status.INTERNAL_SERVER_ERROR)
</code_context>
<issue_to_address>
**suggestion:** Consider throwing `KruizeServiceException` instead of a raw `RuntimeException` for consistency with the global exception mapper.
These services (and `ProfileService`) currently throw generic `RuntimeException`s, which end up as default 500s with a generic message. Using `KruizeServiceException` (or a shared hierarchy) from service methods would let you set both status code and client-facing message explicitly, making error handling more predictable and consistent across the API.
</issue_to_address>
### Comment 4
<location path="README.md" line_range="46" />
<code_context>
+./mvnw package -Dnative
+```
+
+Or, if you don't have GraalVM installed, you can run the native executable build in a container using:
+
+```shell script
</code_context>
<issue_to_address>
**suggestion (typo):** Fix the grammatical issue in "native executable build".
Consider rephrasing to something like “run the native executable built in a container” or “run the build of the native executable in a container” to fix the grammar around “build/built.”
Suggested implementation:
```
Or, if you don't have GraalVM installed, you can run the native executable built in a container using:
```
From the provided snippet, it looks like some markdown code fences around the `./mvnw quarkus:dev` command might be misaligned or missing. You may want to review that section in the full README to ensure all ```shell script fences are correctly opened and closed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/com/kruize/optimizer/service/DatasourceService.java
Outdated
Show resolved
Hide resolved
| ./mvnw package -Dnative | ||
| ``` | ||
|
|
||
| Or, if you don't have GraalVM installed, you can run the native executable build in a container using: |
There was a problem hiding this comment.
suggestion (typo): Fix the grammatical issue in "native executable build".
Consider rephrasing to something like “run the native executable built in a container” or “run the build of the native executable in a container” to fix the grammar around “build/built.”
Suggested implementation:
Or, if you don't have GraalVM installed, you can run the native executable built in a container using:
From the provided snippet, it looks like some markdown code fences around the ./mvnw quarkus:dev command might be misaligned or missing. You may want to review that section in the full README to ensure all ```shell script fences are correctly opened and closed.
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
shekhar316
requested review from
bharathappali,
khansaad,
mbvreddy and
shreyabiradar07
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
3 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.
This PR -
Note: This PR is based on profile manager module, that needs to be merged first.
Summary by Sourcery
Introduce a Quarkus-based Kruize Optimizer microservice with REST endpoints for managing Kruize datasources, profiles, and system status, backed by a Maven build and runtime configuration.
New Features:
Enhancements:
Build:
Documentation: