Add Bulk Scheduler & Webhook updates - #36
Conversation
Signed-off-by: bharathappali <abharath@redhat.com>
Reviewer's GuideIntroduce profile-based bulk scheduling driven by Kruize bulk profiles, including per-profile timers, new profile/webhook handling paths, and basic per-profile job metrics, while keeping the existing fixed-interval scheduler as a fallback mode. Sequence diagram for profile-based bulk scheduling and executionsequenceDiagram
participant BulkSchedulerService
participant ProfileTimerManager
participant BulkProfileService
participant KruizeClient
participant Scheduler
participant JobsService
BulkSchedulerService->>ProfileTimerManager: initializeProfiles()
ProfileTimerManager->>BulkProfileService: getEnabledProfiles()
BulkProfileService->>KruizeClient: getBulkProfiles()
KruizeClient-->>BulkProfileService: JSON BulkProfile[]
BulkProfileService-->>ProfileTimerManager: List<BulkProfile>
loop for each BulkProfile
ProfileTimerManager->>ProfileTimerManager: scheduleProfile(profile)
ProfileTimerManager->>Scheduler: scheduleAtFixedRate(executeProfileJob)
end
rect rgb(230,230,230)
Scheduler->>ProfileTimerManager: executeProfileJob(profile)
ProfileTimerManager->>BulkProfileService: convertProfileToBulkJob(profile)
BulkProfileService-->>ProfileTimerManager: Map bulkJob
ProfileTimerManager->>KruizeClient: bulkCreateExperiments(bulkJob)
KruizeClient-->>ProfileTimerManager: response
ProfileTimerManager->>JobsService: incrementJobsTriggered(profileName)
end
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 2 issues, and left some high level feedback:
- In
BulkProfileService.convertProfileToBulkJob()andProfileTimerManager.scheduleProfile()/updateProfileTimer(), consider adding defensive null checks forrecommendationSettingsand its fields (e.g.,getScheduling(),getMeasurementDuration()), since these come from an external API and can currently cause NPEs orIllegalArgumentExceptions that abort initialization or webhook handling. - The logic in
ProfileTimerManager.updateProfileTimer()for handling interval changes can be simplified: both branches cancel and reschedule the timer, so you can remove thedelayMillis > newInterval.toMillis()condition unless you plan to treat shorter vs. longer intervals differently. - In
ProfileTimerManager, theScheduledExecutorServiceis created with a hardcoded pool size of 10; consider either making this configurable or using a container-managed executor so the thread usage can be tuned for different deployment environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `BulkProfileService.convertProfileToBulkJob()` and `ProfileTimerManager.scheduleProfile()/updateProfileTimer()`, consider adding defensive null checks for `recommendationSettings` and its fields (e.g., `getScheduling()`, `getMeasurementDuration()`), since these come from an external API and can currently cause NPEs or `IllegalArgumentException`s that abort initialization or webhook handling.
- The logic in `ProfileTimerManager.updateProfileTimer()` for handling interval changes can be simplified: both branches cancel and reschedule the timer, so you can remove the `delayMillis > newInterval.toMillis()` condition unless you plan to treat shorter vs. longer intervals differently.
- In `ProfileTimerManager`, the `ScheduledExecutorService` is created with a hardcoded pool size of 10; consider either making this configurable or using a container-managed executor so the thread usage can be tuned for different deployment environments.
## Individual Comments
### Comment 1
<location path="src/main/java/com/kruize/optimizer/service/BulkProfileService.java" line_range="177-178" />
<code_context>
+ bulkJob.put("metadata_profile", cluster.getMetadataProfile());
+ }
+
+ // 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 (bug_risk):** Guard against null recommendation settings when building bulk job requests
This dereferences `profile.getRecommendationSettings()` without a null check. If Kruize returns a profile without `recommendation_settings`, this will throw a `NullPointerException` and prevent bulk job creation for that profile. Add a null check before calling `getMeasurementDuration()`, and consider logging and skipping `measurement_duration` when settings are missing.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/kruize/optimizer/service/BulkProfileService.java" line_range="92-101" />
<code_context>
+ public Duration parseScheduling(String scheduling) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider making parseScheduling more robust to unexpected formats to avoid bringing down callers
`parseScheduling` currently throws `IllegalArgumentException` for null/empty/invalid strings, and callers like `scheduleProfile`/`updateProfileTimer` don’t catch it. A single bad value can therefore abort timer initialization or fail a webhook. Consider either handling these exceptions at the call sites (e.g., log and skip scheduling for that profile) or changing this method to return a default/optional value so callers can decide how to recover.
Suggested implementation:
```java
import java.time.Duration;
import java.util.Optional;
```
```java
* @param scheduling Scheduling string
* @return Optional Duration object, empty if the scheduling string is null/empty/invalid
*/
public Optional<Duration> parseScheduling(String scheduling) {
if (scheduling == null || scheduling.trim().isEmpty()) {
LOG.warn("Skipping scheduling: scheduling string is null or empty");
return Optional.empty();
}
Matcher matcher = SCHEDULING_PATTERN.matcher(scheduling.trim().toLowerCase());
if (!matcher.matches()) {
LOG.warn("Skipping scheduling: invalid scheduling format '{}'", scheduling);
return Optional.empty();
}
int value = Integer.parseInt(matcher.group(1));
```
To fully implement the suggestion and avoid unexpected exceptions propagating to callers, you should also:
1. Ensure a logger is available in this class:
- If not already present, add something like `private static final Logger LOG = LoggerFactory.getLogger(BulkProfileService.class);` and the corresponding SLF4J imports.
2. Update all call sites in this file (and others) from:
- `Duration duration = parseScheduling(profile.getScheduling());`
- to something that safely handles missing/invalid values, for example:
- `Optional<Duration> maybeDuration = parseScheduling(profile.getScheduling());`
- `maybeDuration.ifPresent(duration -> scheduleProfile(profile, duration));`
- or provide a fallback/default duration where appropriate.
3. Adjust method signatures for `scheduleProfile` / `updateProfileTimer` (or similar) if they currently assume a non-null `Duration`, so they can either:
- Accept an `Optional<Duration>`, or
- Simply not be called when `parseScheduling` returns `Optional.empty()`.
4. If this class is part of a public API, update any interfaces or tests that reference `parseScheduling` to use `Optional<Duration>` and assert on the empty case instead of expecting exceptions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Add measurement duration from recommendation settings | ||
| String measurementDuration = profile.getRecommendationSettings().getMeasurementDuration(); |
There was a problem hiding this comment.
issue (bug_risk): Guard against null recommendation settings when building bulk job requests
This dereferences profile.getRecommendationSettings() without a null check. If Kruize returns a profile without recommendation_settings, this will throw a NullPointerException and prevent bulk job creation for that profile. Add a null check before calling getMeasurementDuration(), and consider logging and skipping measurement_duration when settings are missing.
| public Duration parseScheduling(String scheduling) { | ||
| if (scheduling == null || scheduling.trim().isEmpty()) { | ||
| 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); | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion (bug_risk): Consider making parseScheduling more robust to unexpected formats to avoid bringing down callers
parseScheduling currently throws IllegalArgumentException for null/empty/invalid strings, and callers like scheduleProfile/updateProfileTimer don’t catch it. A single bad value can therefore abort timer initialization or fail a webhook. Consider either handling these exceptions at the call sites (e.g., log and skip scheduling for that profile) or changing this method to return a default/optional value so callers can decide how to recover.
Suggested implementation:
import java.time.Duration;
import java.util.Optional; * @param scheduling Scheduling string
* @return Optional Duration object, empty if the scheduling string is null/empty/invalid
*/
public Optional<Duration> parseScheduling(String scheduling) {
if (scheduling == null || scheduling.trim().isEmpty()) {
LOG.warn("Skipping scheduling: scheduling string is null or empty");
return Optional.empty();
}
Matcher matcher = SCHEDULING_PATTERN.matcher(scheduling.trim().toLowerCase());
if (!matcher.matches()) {
LOG.warn("Skipping scheduling: invalid scheduling format '{}'", scheduling);
return Optional.empty();
}
int value = Integer.parseInt(matcher.group(1));To fully implement the suggestion and avoid unexpected exceptions propagating to callers, you should also:
- Ensure a logger is available in this class:
- If not already present, add something like
private static final Logger LOG = LoggerFactory.getLogger(BulkProfileService.class);and the corresponding SLF4J imports.
- If not already present, add something like
- Update all call sites in this file (and others) from:
Duration duration = parseScheduling(profile.getScheduling());- to something that safely handles missing/invalid values, for example:
Optional<Duration> maybeDuration = parseScheduling(profile.getScheduling());maybeDuration.ifPresent(duration -> scheduleProfile(profile, duration));- or provide a fallback/default duration where appropriate.
- Adjust method signatures for
scheduleProfile/updateProfileTimer(or similar) if they currently assume a non-nullDuration, so they can either:- Accept an
Optional<Duration>, or - Simply not be called when
parseSchedulingreturnsOptional.empty().
- Accept an
- If this class is part of a public API, update any interfaces or tests that reference
parseSchedulingto useOptional<Duration>and assert on the empty case instead of expecting exceptions.
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>
d84d929 to
145fed3
Compare
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
This PR is built on top of #35
#35 needs to be merged before merging this PR
This PR adds the bulk scheduler changes
Summary by Sourcery
Introduce profile-based bulk scheduling driven by Kruize bulk profiles and integrate it with existing bulk scheduler and webhook handling.
New Features:
Enhancements: