Add Profile Timer Manager - #35
Open
bharathappali wants to merge 2 commits into
Open
Conversation
Reviewer's GuideIntroduce a Profile Timer Manager and supporting bulk profile infrastructure to periodically trigger bulk jobs per profile, including REST client endpoints, data models, and job accounting. Sequence diagram for scheduled bulk profile job executionsequenceDiagram
participant ProfileTimerManager
participant BulkProfileService
participant KruizeClient
participant JobsService
ProfileTimerManager->>BulkProfileService: getEnabledProfiles()
BulkProfileService-->>ProfileTimerManager: List<BulkProfile>
loop for each BulkProfile
ProfileTimerManager->>BulkProfileService: parseScheduling(scheduling)
BulkProfileService-->>ProfileTimerManager: Duration
ProfileTimerManager->>ProfileTimerManager: scheduleProfile(profile)
end
ProfileTimerManager->>ProfileTimerManager: executeProfileJob(profile)
ProfileTimerManager->>BulkProfileService: convertProfileToBulkJob(profile)
BulkProfileService-->>ProfileTimerManager: Map bulkJob
ProfileTimerManager->>KruizeClient: bulkCreateExperiments(bulkJob)
KruizeClient-->>ProfileTimerManager: String response
ProfileTimerManager->>JobsService: incrementJobsTriggered(profileName)
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}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- ProfileTimerManager currently always schedules with an initial delay of 0; consider using the parsed scheduling interval (or a configurable value) for the initial delay to avoid immediate execution on startup or after updates if that’s not desired behavior.
- BulkProfileService.convertProfileToBulkJob only uses the first ClusterConfig and silently ignores additional clusters; if multiple clusters are expected, you may want to either aggregate them or explicitly validate and fail fast to avoid surprising partial behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ProfileTimerManager currently always schedules with an initial delay of 0; consider using the parsed scheduling interval (or a configurable value) for the initial delay to avoid immediate execution on startup or after updates if that’s not desired behavior.
- BulkProfileService.convertProfileToBulkJob only uses the first ClusterConfig and silently ignores additional clusters; if multiple clusters are expected, you may want to either aggregate them or explicitly validate and fail fast to avoid surprising partial behavior.
## Individual Comments
### Comment 1
<location path="src/main/java/com/kruize/optimizer/service/ProfileTimerManager.java" line_range="67-68" />
<code_context>
+ cancelProfileTimer(profileName);
+
+ // Parse scheduling interval
+ Duration interval = bulkProfileService.parseScheduling(
+ profile.getRecommendationSettings().getScheduling()
+ );
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against null `recommendationSettings` when parsing scheduling.
If `profile.getRecommendationSettings()` can be null, this line will cause a `NullPointerException` and stop `scheduleProfile` / `initializeProfiles` from scheduling timers. Please add a null check and either use a safe default interval or fail fast with a clear error that includes the profile name.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/kruize/optimizer/service/BulkProfileService.java" line_range="174-175" />
<code_context>
+ }
+
+ // Add measurement duration from recommendation settings
+ String measurementDuration = profile.getRecommendationSettings().getMeasurementDuration();
+ if (measurementDuration != null && !measurementDuration.isEmpty()) {
+ bulkJob.put("measurement_duration", measurementDuration);
+ }
</code_context>
<issue_to_address>
**issue:** Handle possible null `recommendationSettings` when building the bulk job.
`profile.getRecommendationSettings()` can be null, leading to a `NullPointerException` when calling `getMeasurementDuration()`. Add a null check or validate the presence of `recommendationSettings` before this call, and define the behavior when it’s missing (e.g., omit `measurement_duration` or use a default).
</issue_to_address>
### Comment 3
<location path="src/main/java/com/kruize/optimizer/service/BulkProfileService.java" line_range="93-95" />
<code_context>
+ throw new IllegalArgumentException("Scheduling string cannot be null or empty");
+ }
+
+ Matcher matcher = SCHEDULING_PATTERN.matcher(scheduling.trim().toLowerCase());
+ if (!matcher.matches()) {
+ throw new IllegalArgumentException("Invalid scheduling format: " + scheduling);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Invalid scheduling formats currently cause an unchecked exception that may disrupt timer initialization.
Because `scheduleProfile` and `initializeProfiles` both call this logic, a single malformed `scheduling` string can prevent all timers from being initialized. Consider handling the error locally (e.g., log and skip the offending profile or fall back to a safe default interval) instead of letting the `IllegalArgumentException` propagate.
Suggested implementation:
```java
public Duration parseScheduling(String scheduling) {
// Use a safe default so that a single malformed profile does not block timer initialization
Duration defaultDuration = Duration.ofMinutes(5);
if (scheduling == null || scheduling.trim().isEmpty()) {
LOGGER.warn("Scheduling string is null or empty. Falling back to default interval: " + defaultDuration);
return defaultDuration;
}
```
```java
Matcher matcher = SCHEDULING_PATTERN.matcher(scheduling.trim().toLowerCase());
if (!matcher.matches()) {
LOGGER.warn("Invalid scheduling format '" + scheduling + "'. Falling back to default interval: " + defaultDuration);
return defaultDuration;
}
```
1. Ensure `BulkProfileService` has a logger instance, e.g. `private static final Logger LOGGER = Logger.getLogger(BulkProfileService.class);`.
2. If other parts of the code rely on `IllegalArgumentException` from `parseScheduling` (e.g., tests or callers doing explicit error handling), update them to work with the new behavior (using the returned default duration instead of catching exceptions).
3. Optionally, extract `Duration.ofMinutes(5)` into a named constant (e.g., `private static final Duration DEFAULT_SCHEDULING_INTERVAL = Duration.ofMinutes(5);`) and reuse it across the class, including `scheduleProfile` / `initializeProfiles` if they need the same fallback logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+67
to
+68
| Duration interval = bulkProfileService.parseScheduling( | ||
| profile.getRecommendationSettings().getScheduling() |
There was a problem hiding this comment.
issue (bug_risk): Guard against null recommendationSettings when parsing scheduling.
If profile.getRecommendationSettings() can be null, this line will cause a NullPointerException and stop scheduleProfile / initializeProfiles from scheduling timers. Please add a null check and either use a safe default interval or fail fast with a clear error that includes the profile name.
Comment on lines
+93
to
+95
| Matcher matcher = SCHEDULING_PATTERN.matcher(scheduling.trim().toLowerCase()); | ||
| if (!matcher.matches()) { | ||
| throw new IllegalArgumentException("Invalid scheduling format: " + scheduling); |
There was a problem hiding this comment.
suggestion (bug_risk): Invalid scheduling formats currently cause an unchecked exception that may disrupt timer initialization.
Because scheduleProfile and initializeProfiles both call this logic, a single malformed scheduling string can prevent all timers from being initialized. Consider handling the error locally (e.g., log and skip the offending profile or fall back to a safe default interval) instead of letting the IllegalArgumentException propagate.
Suggested implementation:
public Duration parseScheduling(String scheduling) {
// Use a safe default so that a single malformed profile does not block timer initialization
Duration defaultDuration = Duration.ofMinutes(5);
if (scheduling == null || scheduling.trim().isEmpty()) {
LOGGER.warn("Scheduling string is null or empty. Falling back to default interval: " + defaultDuration);
return defaultDuration;
} Matcher matcher = SCHEDULING_PATTERN.matcher(scheduling.trim().toLowerCase());
if (!matcher.matches()) {
LOGGER.warn("Invalid scheduling format '" + scheduling + "'. Falling back to default interval: " + defaultDuration);
return defaultDuration;
}- Ensure
BulkProfileServicehas a logger instance, e.g.private static final Logger LOGGER = Logger.getLogger(BulkProfileService.class);. - If other parts of the code rely on
IllegalArgumentExceptionfromparseScheduling(e.g., tests or callers doing explicit error handling), update them to work with the new behavior (using the returned default duration instead of catching exceptions). - Optionally, extract
Duration.ofMinutes(5)into a named constant (e.g.,private static final Duration DEFAULT_SCHEDULING_INTERVAL = Duration.ofMinutes(5);) and reuse it across the class, includingscheduleProfile/initializeProfilesif they need the same fallback logic.
bharathappali
force-pushed
the
feat-bulk-prof-5
branch
from
July 9, 2026 07:56
72d9070 to
3250d0d
Compare
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
khansaad
force-pushed
the
feat-bulk-prof-5
branch
from
August 28, 2026 06:33
3250d0d to
3c5f169
Compare
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.
This PR is built on top of #34
#34 needs to be merged before this PR
This PR adds the Profile Timer Manager
Summary by Sourcery
Manage recurring bulk profile jobs with configuration-specific scheduling and job tracking.
New Features:
Enhancements: