Add support for multiple datasource - #27
Open
khansaad wants to merge 4 commits into
Open
Conversation
Signed-off-by: Saad Khan <saakhan@ibm.com>
Reviewer's GuideAdds multi-datasource support to bulk experiment creation by introducing a list-based datasource configuration and wiring it through state resolution, payload construction, configuration, and deployment manifests while preserving backward compatibility with the legacy single-datasource setting. Sequence diagram for multi-datasource bulk experiment creation flowsequenceDiagram
participant BulkSchedulerService
participant KruizeStateService
participant BulkAPI
BulkSchedulerService->>KruizeStateService: refreshState()
KruizeStateService-->>BulkSchedulerService: (state refreshed)
BulkSchedulerService->>KruizeStateService: getDefaultDatasourceNames()
KruizeStateService-->>BulkSchedulerService: List<String> datasourceNames
BulkSchedulerService->>BulkSchedulerService: buildBulkPayload(targetLabels, datasourceNames, metadataProfile, metricProfile)
BulkSchedulerService->>BulkAPI: POST /bulk (payload with datasources)
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 2 issues, and left some high level feedback:
- In
buildBulkPayload, consider populating bothBulkSchedulerConstants.DATASOURCESand the legacyBulkSchedulerConstants.DATASOURCE(e.g., first entry) to preserve backward compatibility for any consumers still expecting the singulardatasourcefield. - In
getDefaultDatasourceNames(), you're trimming entries only for the newdefaultDatasourcesconfig but not for the legacydefaultDatasource; aligning this (e.g., trimmingdefaultDatasourcebefore comparison) would avoid subtle mismatches when users accidentally include whitespace in the legacy env var.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `buildBulkPayload`, consider populating both `BulkSchedulerConstants.DATASOURCES` and the legacy `BulkSchedulerConstants.DATASOURCE` (e.g., first entry) to preserve backward compatibility for any consumers still expecting the singular `datasource` field.
- In `getDefaultDatasourceNames()`, you're trimming entries only for the new `defaultDatasources` config but not for the legacy `defaultDatasource`; aligning this (e.g., trimming `defaultDatasource` before comparison) would avoid subtle mismatches when users accidentally include whitespace in the legacy env var.
## Individual Comments
### Comment 1
<location path="src/main/java/com/kruize/optimizer/service/BulkSchedulerService.java" line_range="215-216" />
<code_context>
- // Add datasource from global state
- payload.put(BulkSchedulerConstants.DATASOURCE, datasource);
+ // Add datasources list from global state
+ payload.put(BulkSchedulerConstants.DATASOURCES, datasources);
// Add metadata profile from global state
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider populating both `datasources` and deprecated `datasource` keys for backward compatibility
The constants still define `DATASOURCE` as deprecated for backward compatibility, but the payload now only sets `DATASOURCES`. Existing consumers that rely on the singular field may break. To avoid this, set both for now, e.g.:
```java
payload.put(BulkSchedulerConstants.DATASOURCES, datasources);
if (!datasources.isEmpty()) {
payload.put(BulkSchedulerConstants.DATASOURCE, datasources.get(0));
}
```
</issue_to_address>
### Comment 2
<location path="src/main/java/com/kruize/optimizer/service/KruizeStateService.java" line_range="175-166" />
<code_context>
- return cachedDatasources.stream()
- .findFirst()
- .map(Datasource::getName);
+ // Fall back to old single datasource config if new config is empty
+ if (result.isEmpty() && defaultDatasource != null && !defaultDatasource.trim().isEmpty()) {
+ boolean exists = cachedDatasources.stream()
+ .anyMatch(ds -> defaultDatasource.equals(ds.getName()));
+ if (exists) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Normalize `defaultDatasource` before comparison to avoid subtle mismatch cases
When falling back to the single `defaultDatasource`, you trim only for the emptiness check but compare the untrimmed value against `ds.getName()`. A value with leading/trailing spaces will pass the check but never match. Trim once and reuse the trimmed value for both checks and comparison, e.g.:
```java
String defaultName = defaultDatasource == null ? null : defaultDatasource.trim();
if (result.isEmpty() && defaultName != null && !defaultName.isEmpty()) {
boolean exists = cachedDatasources.stream()
.anyMatch(ds -> defaultName.equals(ds.getName()));
if (exists) {
result.add(defaultName);
}
}
```
</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
+215
to
+216
| // Add datasources list from global state | ||
| payload.put(BulkSchedulerConstants.DATASOURCES, datasources); |
There was a problem hiding this comment.
issue (bug_risk): Consider populating both datasources and deprecated datasource keys for backward compatibility
The constants still define DATASOURCE as deprecated for backward compatibility, but the payload now only sets DATASOURCES. Existing consumers that rely on the singular field may break. To avoid this, set both for now, e.g.:
payload.put(BulkSchedulerConstants.DATASOURCES, datasources);
if (!datasources.isEmpty()) {
payload.put(BulkSchedulerConstants.DATASOURCE, datasources.get(0));
}Signed-off-by: Saad Khan <saakhan@ibm.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
shekhar316
reviewed
Jun 16, 2026
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The configuration and comments are inconsistent between
KRUIZE_DATASOURCESandKRUIZE_DEFAULT_DATASOURCES(e.g.,application.ymlusesKRUIZE_DATASOURCES, but the base deployment setsKRUIZE_DEFAULT_DATASOURCES), which will result in the multi-datasource config not being picked up in some environments; align the env var name and references across code, YAML, and manifests. - BulkScheduler now only emits the
datasourcesfield in the payload whileBulkSchedulerConstantsstill exposesDATASOURCEfor backwards compatibility; if existing consumers expectdatasource, consider populating both keys (at least when a single datasource is configured) to avoid breaking older clients.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The configuration and comments are inconsistent between `KRUIZE_DATASOURCES` and `KRUIZE_DEFAULT_DATASOURCES` (e.g., `application.yml` uses `KRUIZE_DATASOURCES`, but the base deployment sets `KRUIZE_DEFAULT_DATASOURCES`), which will result in the multi-datasource config not being picked up in some environments; align the env var name and references across code, YAML, and manifests.
- BulkScheduler now only emits the `datasources` field in the payload while `BulkSchedulerConstants` still exposes `DATASOURCE` for backwards compatibility; if existing consumers expect `datasource`, consider populating both keys (at least when a single datasource is configured) to avoid breaking older clients.
## Individual Comments
### Comment 1
<location path="deployment/base/deployment.yaml" line_range="68-75" />
<code_context>
value: '{"kruize/autotune": "enabled"}'
# Default Configuration
+ # Default datasource name (deprecated - use KRUIZE_DEFAULT_DATASOURCES for multi-datasource support)
+ - name: KRUIZE_DEFAULT_DATASOURCE
+ value: "prometheus-1"
+
+ # Default datasources (comma-separated list for multi-datasource support)
+ # Takes precedence over single datasource config
+ # Example: "prometheus-1,prometheus-2"
+ - name: KRUIZE_DEFAULT_DATASOURCES
+ value: "prometheus-1"
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Align environment variable name with the one used in application.yml (`KRUIZE_DATASOURCES` vs `KRUIZE_DEFAULT_DATASOURCES`).
`application.yml` reads `kruize.defaults.datasources: ${KRUIZE_DATASOURCES:}`, but this deployment only defines `KRUIZE_DEFAULT_DATASOURCES`. As a result, the multi-datasource configuration here will never be used. Please either rename this env var to `KRUIZE_DATASOURCES` (and update the comment) or change the application config to read from `KRUIZE_DEFAULT_DATASOURCES`, depending on which name you intend to standardize on.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/kruize/optimizer/service/KruizeStateService.java" line_range="203-212" />
<code_context>
+ * Get the default datasource name (for operations that need a single datasource)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Behavior of `getDefaultDatasourceName` does not fully match the documented defaulting semantics.
The method Javadoc and `application.yml` comment say that when multiple datasources exist and no default is configured, the default should be `"prometheus-1"`. The current implementation instead uses the first entry in `allDatasources` when `defaultDatasource` is unset/blank, which may differ from `"prometheus-1"`. Either update the logic to explicitly prefer `"prometheus-1"` when present (falling back to the first element otherwise), or update the comments/config docs to reflect the actual behavior.
Suggested implementation:
```java
*
* Logic:
* - If KRUIZE_DATASOURCES has only one datasource, return that
* - If KRUIZE_DATASOURCES has multiple datasources:
* - If KRUIZE_DEFAULT_DATASOURCE is set, return that
* - If KRUIZE_DEFAULT_DATASOURCE is not set or blank, return the first configured datasource
* - If KRUIZE_DATASOURCES is not set:
* - If KRUIZE_DEFAULT_DATASOURCE is set, return that
* - If KRUIZE_DEFAULT_DATASOURCE is not set or blank, return empty
*
```
The current change updates the Javadoc to accurately describe the existing behavior (using the first configured datasource when multiple exist and no default is set).
If you instead choose to align the implementation with the documented `"prometheus-1"` behavior, you should:
1. Update `getDefaultDatasourceName()` implementation to:
- If multiple datasources exist and `KRUIZE_DEFAULT_DATASOURCE` is unset/blank, first look for a datasource named `"prometheus-1"` and return it if present.
- If `"prometheus-1"` is not present, fall back to the current behavior (first configured datasource).
2. Optionally, extract `"prometheus-1"` into a constant (e.g. `DEFAULT_DATASOURCE_NAME`) in your configuration or constants class and reference that instead of a string literal, to keep it consistent with `application.yml`.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/kruize/optimizer/service/BulkSchedulerService.java" line_range="215-216" />
<code_context>
- // Add datasource from global state
- payload.put(BulkSchedulerConstants.DATASOURCE, datasource);
+ // Add datasources list from global state
+ payload.put(BulkSchedulerConstants.DATASOURCES, datasources);
// Add metadata profile from global state
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider populating both `datasources` (new) and `datasource` (deprecated) in the payload for backward compatibility.
The new code only populates `DATASOURCES`, so the legacy `DATASOURCE` field is no longer sent even though the constant still exists. If any current consumers still depend on `datasource`, this will be a breaking change. Please consider populating both for now (e.g., set `DATASOURCE` from the primary entry in `datasources`) and deprecate `DATASOURCE` once all consumers are updated.
</issue_to_address>
### Comment 4
<location path="src/test/java/com/kruize/optimizer/service/BulkSchedulerServiceTest.java" line_range="100-103" />
<code_context>
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
- when(kruizeStateService.getDefaultDatasourceName()).thenReturn(Optional.of("prometheus-1"));
+ when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(List.of("prometheus-1"));
when(kruizeStateService.getDefaultMetadataProfileName()).thenReturn(Optional.of("cluster-metadata-local-monitoring"));
when(kruizeStateService.getDefaultMetricProfileName()).thenReturn(Optional.of("resource-optimization-local-monitoring"));
</code_context>
<issue_to_address>
**suggestion (testing):** Add a dedicated test case for multiple datasources to validate the new list-based behavior.
The current "happy path" test still only covers a single datasource. Since `BulkSchedulerService` now operates on a list, please add a separate test (e.g. `testScheduledBulkApiCall_MultipleDatasources`) where `getDefaultDatasourceNames()` returns multiple entries (e.g. `List.of("prometheus-1", "prometheus-2")`) and assert that:
- The payload contains the complete datasource list (and order, if relevant).
- The list passed to `kruizeClient.bulkCreateExperiments` matches what `kruizeStateService` returned.
This ensures the multi-datasource path is correctly wired and guards against regressions that only use the first datasource.
Suggested implementation:
```java
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.mockito.ArgumentCaptor;
```
```java
void testScheduledBulkApiCall_Success() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(List.of("prometheus-1"));
when(kruizeStateService.getDefaultMetadataProfileName()).thenReturn(Optional.of("cluster-metadata-local-monitoring"));
when(kruizeStateService.getDefaultMetricProfileName()).thenReturn(Optional.of("resource-optimization-local-monitoring"));
when(kruizeClient.bulkCreateExperiments(any())).thenReturn(mockBulkApiResponse);
assertNotNull(payload);
assertTrue(payload.containsKey("filter"));
assertTrue(payload.containsKey("datasources"));
assertTrue(payload.containsKey("metadata_profile"));
assertTrue(payload.containsKey("measurement_duration"));
assertEquals(List.of("prometheus-1"), payload.get("datasources"));
}
@Test
void testScheduledBulkApiCall_MultipleDatasources() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
List<String> datasources = List.of("prometheus-1", "prometheus-2");
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(datasources);
when(kruizeStateService.getDefaultMetadataProfileName())
.thenReturn(Optional.of("cluster-metadata-local-monitoring"));
when(kruizeStateService.getDefaultMetricProfileName())
.thenReturn(Optional.of("resource-optimization-local-monitoring"));
when(kruizeClient.bulkCreateExperiments(any())).thenReturn(mockBulkApiResponse);
// Act
bulkSchedulerService.scheduledBulkApiCall();
// Assert
ArgumentCaptor<Map<String, Object>> payloadCaptor = ArgumentCaptor.forClass(Map.class);
verify(kruizeClient).bulkCreateExperiments(payloadCaptor.capture());
Map<String, Object> payload = payloadCaptor.getValue();
assertNotNull(payload);
assertTrue(payload.containsKey("filter"));
assertTrue(payload.containsKey("datasources"));
assertTrue(payload.containsKey("metadata_profile"));
assertTrue(payload.containsKey("measurement_duration"));
// Ensure the full list of datasources is propagated and ordered correctly
assertEquals(datasources, payload.get("datasources"));
```
1. This new test assumes:
- `bulkSchedulerService` and `kruizeClient` are already initialized and mocked as in the existing tests.
- The `bulkCreateExperiments` method accepts a single payload `Map<String, Object>`; if it instead accepts a list or another type, adjust the `ArgumentCaptor` generic type and the verification accordingly.
2. If the test class does not already use JUnit 5, replace `@Test` with the appropriate annotation/import for your JUnit version.
3. If `payload` in the existing success test is produced via a shared helper or field rather than an `ArgumentCaptor`, you can align the new test with that pattern by reusing the same mechanism instead of introducing a new captor.
</issue_to_address>
### Comment 5
<location path="src/test/java/com/kruize/optimizer/service/BulkSchedulerServiceTest.java" line_range="161-164" />
<code_context>
+ * - Service logs error about missing datasources
*/
@Test
void testScheduledBulkApiCall_NoDatasource() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
- when(kruizeStateService.getDefaultDatasourceName()).thenReturn(Optional.empty());
+ when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(Collections.emptyList());
// Act
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting the new error message for missing datasources to fully validate the failure path.
Since the implementation now logs `MessageConstants.ERROR_NO_DATASOURCES_AVAILABLE`, please extend this test to assert that this specific log message is emitted (e.g., via a log appender or logger spy), in addition to the existing checks on `bulkCreateExperiments` and the job counter. This will ensure the test also validates the updated error messaging for the multi-datasource case.
Suggested implementation:
```java
import static com.kruize.optimizer.constants.MessageConstants.ERROR_NO_DATASOURCES_AVAILABLE;
* when no datasources are configured in Kruize.
*
* Expected Behavior:
* - Bulk API not called
* - Jobs counter not incremented
* - Service logs error about missing datasources
*/
@Test
void testScheduledBulkApiCall_NoDatasource() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(Collections.emptyList());
// Act
bulkSchedulerService.initialize();
// Assert
// - Bulk API not called
verify(kruizeClient, never()).bulkCreateExperiments(any());
// - Jobs counter not incremented
assertEquals(0, bulkSchedulerService.getJobsCounter());
// - Service logs error about missing datasources
// (ERROR_NO_DATASOURCES_AVAILABLE should be emitted as an ERROR log)
boolean hasMissingDatasourceErrorLog = logAppender.list.stream()
.anyMatch(event ->
event.getLevel() == ch.qos.logback.classic.Level.ERROR
&& event.getFormattedMessage().contains(ERROR_NO_DATASOURCES_AVAILABLE));
assertTrue(
hasMissingDatasourceErrorLog,
"Expected ERROR log with message: " + ERROR_NO_DATASOURCES_AVAILABLE
);
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(true);
doNothing().when(kruizeStateService).refreshState();
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(List.of("prometheus-1"));
when(kruizeStateService.getDefaultMetadataProfileName()).thenReturn(Optional.of("cluster-metadata-local-monitoring"));
when(kruizeStateService.getDefaultMetricProfileName()).thenReturn(Optional.of("resource-optimization-local-monitoring"));
when(kruizeClient.bulkCreateExperiments(any())).thenReturn(mockBulkApiResponse);
void testScheduledBulkApiCall_ExceptionHandling() {
```
To make this compile and work correctly, ensure the following in the rest of the test file:
1. **Log appender setup**
There must be a `logAppender` field capturing logs from `BulkSchedulerService`, e.g.:
```java
private ListAppender<ILoggingEvent> logAppender;
```
initialized in a `@BeforeEach` like:
```java
@BeforeEach
void setUp() {
Logger logger = (Logger) LoggerFactory.getLogger(BulkSchedulerService.class);
logAppender = new ListAppender<>();
logAppender.start();
logger.addAppender(logAppender);
// existing setup...
}
```
2. **Static imports / assertions**
Ensure `assertEquals` and `assertTrue` are statically imported (e.g., `import static org.junit.jupiter.api.Assertions.*;`) and `verify`, `never`, `any` are imported from Mockito if not already.
3. **Logback classes**
Make sure the file already uses Logback (`ListAppender`, `ILoggingEvent`, `ch.qos.logback.classic.Level`). If a different logging test utility is used in this file, adapt the `hasMissingDatasourceErrorLog` computation to that existing mechanism instead of introducing a new one.
</issue_to_address>
### Comment 6
<location path="src/test/java/com/kruize/optimizer/service/BulkSchedulerServiceTest.java" line_range="121-124" />
<code_context>
assertNotNull(payload);
assertTrue(payload.containsKey("filter"));
- assertTrue(payload.containsKey("datasource"));
+ assertTrue(payload.containsKey("datasources"));
assertTrue(payload.containsKey("metadata_profile"));
assertTrue(payload.containsKey("measurement_duration"));
- assertEquals("prometheus-1", payload.get("datasource"));
+ assertEquals(List.of("prometheus-1"), payload.get("datasources"));
assertEquals("cluster-metadata-local-monitoring", payload.get("metadata_profile"));
assertEquals(measurementDuration, payload.get("measurement_duration"));
</code_context>
<issue_to_address>
**nitpick:** Use `BulkSchedulerConstants.DATASOURCES` instead of hardcoded string keys in assertions.
Since this field now has a `BulkSchedulerConstants.DATASOURCES` constant, reference it in these assertions (e.g. `payload.containsKey(BulkSchedulerConstants.DATASOURCES)`) so the tests track the production contract if the key changes.
</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
+215
to
+216
| // Add datasources list from global state | ||
| payload.put(BulkSchedulerConstants.DATASOURCES, datasources); |
There was a problem hiding this comment.
suggestion (bug_risk): Consider populating both datasources (new) and datasource (deprecated) in the payload for backward compatibility.
The new code only populates DATASOURCES, so the legacy DATASOURCE field is no longer sent even though the constant still exists. If any current consumers still depend on datasource, this will be a breaking change. Please consider populating both for now (e.g., set DATASOURCE from the primary entry in datasources) and deprecate DATASOURCE once all consumers are updated.
Comment on lines
+100
to
103
| when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(List.of("prometheus-1")); | ||
| when(kruizeStateService.getDefaultMetadataProfileName()).thenReturn(Optional.of("cluster-metadata-local-monitoring")); | ||
| when(kruizeStateService.getDefaultMetricProfileName()).thenReturn(Optional.of("resource-optimization-local-monitoring")); | ||
| when(kruizeClient.bulkCreateExperiments(any())).thenReturn(mockBulkApiResponse); |
There was a problem hiding this comment.
suggestion (testing): Add a dedicated test case for multiple datasources to validate the new list-based behavior.
The current "happy path" test still only covers a single datasource. Since BulkSchedulerService now operates on a list, please add a separate test (e.g. testScheduledBulkApiCall_MultipleDatasources) where getDefaultDatasourceNames() returns multiple entries (e.g. List.of("prometheus-1", "prometheus-2")) and assert that:
- The payload contains the complete datasource list (and order, if relevant).
- The list passed to
kruizeClient.bulkCreateExperimentsmatches whatkruizeStateServicereturned.
This ensures the multi-datasource path is correctly wired and guards against regressions that only use the first datasource.
Suggested implementation:
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.mockito.ArgumentCaptor; void testScheduledBulkApiCall_Success() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(List.of("prometheus-1"));
when(kruizeStateService.getDefaultMetadataProfileName()).thenReturn(Optional.of("cluster-metadata-local-monitoring"));
when(kruizeStateService.getDefaultMetricProfileName()).thenReturn(Optional.of("resource-optimization-local-monitoring"));
when(kruizeClient.bulkCreateExperiments(any())).thenReturn(mockBulkApiResponse);
assertNotNull(payload);
assertTrue(payload.containsKey("filter"));
assertTrue(payload.containsKey("datasources"));
assertTrue(payload.containsKey("metadata_profile"));
assertTrue(payload.containsKey("measurement_duration"));
assertEquals(List.of("prometheus-1"), payload.get("datasources"));
}
@Test
void testScheduledBulkApiCall_MultipleDatasources() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
List<String> datasources = List.of("prometheus-1", "prometheus-2");
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(datasources);
when(kruizeStateService.getDefaultMetadataProfileName())
.thenReturn(Optional.of("cluster-metadata-local-monitoring"));
when(kruizeStateService.getDefaultMetricProfileName())
.thenReturn(Optional.of("resource-optimization-local-monitoring"));
when(kruizeClient.bulkCreateExperiments(any())).thenReturn(mockBulkApiResponse);
// Act
bulkSchedulerService.scheduledBulkApiCall();
// Assert
ArgumentCaptor<Map<String, Object>> payloadCaptor = ArgumentCaptor.forClass(Map.class);
verify(kruizeClient).bulkCreateExperiments(payloadCaptor.capture());
Map<String, Object> payload = payloadCaptor.getValue();
assertNotNull(payload);
assertTrue(payload.containsKey("filter"));
assertTrue(payload.containsKey("datasources"));
assertTrue(payload.containsKey("metadata_profile"));
assertTrue(payload.containsKey("measurement_duration"));
// Ensure the full list of datasources is propagated and ordered correctly
assertEquals(datasources, payload.get("datasources"));- This new test assumes:
bulkSchedulerServiceandkruizeClientare already initialized and mocked as in the existing tests.- The
bulkCreateExperimentsmethod accepts a single payloadMap<String, Object>; if it instead accepts a list or another type, adjust theArgumentCaptorgeneric type and the verification accordingly.
- If the test class does not already use JUnit 5, replace
@Testwith the appropriate annotation/import for your JUnit version. - If
payloadin the existing success test is produced via a shared helper or field rather than anArgumentCaptor, you can align the new test with that pattern by reusing the same mechanism instead of introducing a new captor.
Comment on lines
161
to
+164
| void testScheduledBulkApiCall_NoDatasource() { | ||
| // Arrange | ||
| when(kruizeStateService.isCacheEmpty()).thenReturn(false); | ||
| when(kruizeStateService.getDefaultDatasourceName()).thenReturn(Optional.empty()); | ||
| when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(Collections.emptyList()); |
There was a problem hiding this comment.
suggestion (testing): Consider asserting the new error message for missing datasources to fully validate the failure path.
Since the implementation now logs MessageConstants.ERROR_NO_DATASOURCES_AVAILABLE, please extend this test to assert that this specific log message is emitted (e.g., via a log appender or logger spy), in addition to the existing checks on bulkCreateExperiments and the job counter. This will ensure the test also validates the updated error messaging for the multi-datasource case.
Suggested implementation:
import static com.kruize.optimizer.constants.MessageConstants.ERROR_NO_DATASOURCES_AVAILABLE;
* when no datasources are configured in Kruize.
*
* Expected Behavior:
* - Bulk API not called
* - Jobs counter not incremented
* - Service logs error about missing datasources
*/
@Test
void testScheduledBulkApiCall_NoDatasource() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(Collections.emptyList());
// Act
bulkSchedulerService.initialize();
// Assert
// - Bulk API not called
verify(kruizeClient, never()).bulkCreateExperiments(any());
// - Jobs counter not incremented
assertEquals(0, bulkSchedulerService.getJobsCounter());
// - Service logs error about missing datasources
// (ERROR_NO_DATASOURCES_AVAILABLE should be emitted as an ERROR log)
boolean hasMissingDatasourceErrorLog = logAppender.list.stream()
.anyMatch(event ->
event.getLevel() == ch.qos.logback.classic.Level.ERROR
&& event.getFormattedMessage().contains(ERROR_NO_DATASOURCES_AVAILABLE));
assertTrue(
hasMissingDatasourceErrorLog,
"Expected ERROR log with message: " + ERROR_NO_DATASOURCES_AVAILABLE
);
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(true);
doNothing().when(kruizeStateService).refreshState();
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(List.of("prometheus-1"));
when(kruizeStateService.getDefaultMetadataProfileName()).thenReturn(Optional.of("cluster-metadata-local-monitoring"));
when(kruizeStateService.getDefaultMetricProfileName()).thenReturn(Optional.of("resource-optimization-local-monitoring"));
when(kruizeClient.bulkCreateExperiments(any())).thenReturn(mockBulkApiResponse);
void testScheduledBulkApiCall_ExceptionHandling() {To make this compile and work correctly, ensure the following in the rest of the test file:
-
Log appender setup
There must be alogAppenderfield capturing logs fromBulkSchedulerService, e.g.:private ListAppender<ILoggingEvent> logAppender;
initialized in a
@BeforeEachlike:@BeforeEach void setUp() { Logger logger = (Logger) LoggerFactory.getLogger(BulkSchedulerService.class); logAppender = new ListAppender<>(); logAppender.start(); logger.addAppender(logAppender); // existing setup... }
-
Static imports / assertions
EnsureassertEqualsandassertTrueare statically imported (e.g.,import static org.junit.jupiter.api.Assertions.*;) andverify,never,anyare imported from Mockito if not already. -
Logback classes
Make sure the file already uses Logback (ListAppender,ILoggingEvent,ch.qos.logback.classic.Level). If a different logging test utility is used in this file, adapt thehasMissingDatasourceErrorLogcomputation to that existing mechanism instead of introducing a new one.
Comment on lines
+121
to
+124
| assertTrue(payload.containsKey("datasources")); | ||
| assertTrue(payload.containsKey("metadata_profile")); | ||
| assertTrue(payload.containsKey("measurement_duration")); | ||
| assertEquals("prometheus-1", payload.get("datasource")); | ||
| assertEquals(List.of("prometheus-1"), payload.get("datasources")); |
There was a problem hiding this comment.
nitpick: Use BulkSchedulerConstants.DATASOURCES instead of hardcoded string keys in assertions.
Since this field now has a BulkSchedulerConstants.DATASOURCES constant, reference it in these assertions (e.g. payload.containsKey(BulkSchedulerConstants.DATASOURCES)) so the tests track the production contract if the key changes.
Signed-off-by: Saad Khan <saakhan@ibm.com>
khansaad
force-pushed
the
add-multi-ds-support
branch
from
June 18, 2026 15:13
63aba3e to
3919488
Compare
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The environment variable naming for multi-datasource support is inconsistent:
application.ymlmapskruize.defaults.datasourcestoKRUIZE_DATASOURCES, but the overlays setKRUIZE_DEFAULT_DATASOURCES, so those overlays will never affect the new setting—consider standardizing on a single env var name across base and overlays. - The new
getDefaultDatasourceNames()no longer validates the configured names againstcachedDatasources(unlike the previousgetDefaultDatasourceName()which fell back to the first available if the configured default was missing); if this fallback behavior is still desired, consider reintroducing validation and fallback for each configured name. - In
buildBulkPayload, the fallback branch that sets the deprecateddatasourcefield tonullwhen the list is empty is both unexpected and not used by callers; it may be clearer either to omit the key altogether or to throw/log an error instead of sending anullvalue.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The environment variable naming for multi-datasource support is inconsistent: `application.yml` maps `kruize.defaults.datasources` to `KRUIZE_DATASOURCES`, but the overlays set `KRUIZE_DEFAULT_DATASOURCES`, so those overlays will never affect the new setting—consider standardizing on a single env var name across base and overlays.
- The new `getDefaultDatasourceNames()` no longer validates the configured names against `cachedDatasources` (unlike the previous `getDefaultDatasourceName()` which fell back to the first available if the configured default was missing); if this fallback behavior is still desired, consider reintroducing validation and fallback for each configured name.
- In `buildBulkPayload`, the fallback branch that sets the deprecated `datasource` field to `null` when the list is empty is both unexpected and not used by callers; it may be clearer either to omit the key altogether or to throw/log an error instead of sending a `null` value.
## Individual Comments
### Comment 1
<location path="deployment/overlays/openshift/kustomization.yaml" line_range="24-26" />
<code_context>
+ # Deprecated - use KRUIZE_DEFAULT_DATASOURCES for multi-datasource support
- name: KRUIZE_DEFAULT_DATASOURCE
value: "prometheus-1"
+ # New - comma-separated list of datasources (takes precedence over single datasource)
+ # Example: "prometheus-1,prometheus-2"
+ - name: KRUIZE_DEFAULT_DATASOURCES
+ value: "prometheus-1"
target:
</code_context>
<issue_to_address>
**issue (bug_risk):** OpenShift overlay repeats the `KRUIZE_DEFAULT_DATASOURCES` vs `KRUIZE_DATASOURCES` mismatch.
This overlay sets `KRUIZE_DEFAULT_DATASOURCES`, but `application.yml` reads `KRUIZE_DATASOURCES`, so the configured list ("thanos-1,cryostat-2") will not affect `kruize.defaults.datasources`. Please align the env var name with `application.yml` to ensure the list is actually used.
</issue_to_address>
### Comment 2
<location path="src/test/java/com/kruize/optimizer/service/BulkSchedulerServiceTest.java" line_range="210-219" />
<code_context>
+ * - Service logs error about missing datasources (ERROR_NO_DATASOURCES_AVAILABLE)
*/
@Test
void testScheduledBulkApiCall_NoDatasource() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
- when(kruizeStateService.getDefaultDatasourceName()).thenReturn(Optional.empty());
+ when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(Collections.emptyList());
// Act
bulkSchedulerService.initialize();
bulkSchedulerService.scheduledBulkApiCall();
- // Assert - Should not call bulk API
+ // Assert
+ // - Bulk API not called
verify(kruizeClient, never()).bulkCreateExperiments(any());
+ // - Jobs counter not incremented
verify(jobsService, never()).incrementJobsTriggered();
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting that the correct error message is logged when no datasources are available.
The Javadoc for `testScheduledBulkApiCall_NoDatasource` documents that `ERROR_NO_DATASOURCES_AVAILABLE` should be logged, but this test only checks that the bulk API is not called and the jobs counter is not incremented. If you have a logging test helper or can inject a logger/appender, please also assert that `ERROR_NO_DATASOURCES_AVAILABLE` is emitted so the test fully verifies the documented behavior and use of the new constant.
Suggested implementation:
```java
import com.kruize.optimizer.client.KruizeClient;
import com.kruize.optimizer.util.MockResponseLoader;
import com.kruize.optimizer.utils.OptimizerConstants.BulkSchedulerConstants;
import io.quarkus.test.InjectMock;
import io.quarkus.test.junit.QuarkusTest;
import jakarta.inject.Inject;
import nl.altindag.log.LogCaptor;
import org.mockito.Mockito;
import java.io.IOException;
import java.util.Collections;
```
```java
*/
@Test
void testScheduledBulkApiCall_NoDatasource() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(Collections.emptyList());
LogCaptor logCaptor = LogCaptor.forClass(BulkSchedulerService.class);
// Act
bulkSchedulerService.initialize();
bulkSchedulerService.scheduledBulkApiCall();
// Assert
// - Bulk API not called
verify(kruizeClient, never()).bulkCreateExperiments(any());
// - Jobs counter not incremented
verify(jobsService, never()).incrementJobsTriggered();
// - Error about missing datasources is logged
assertTrue(
logCaptor.getErrorLogs()
.stream()
.anyMatch(message -> message.contains(BulkSchedulerConstants.ERROR_NO_DATASOURCES_AVAILABLE)),
"Expected ERROR_NO_DATASOURCES_AVAILABLE to be logged when no datasources are available"
);
}
```
1. This change assumes the presence of the `BulkSchedulerService` class in the same package; ensure the test can reference `BulkSchedulerService.class` (add an import if needed: `import com.kruize.optimizer.service.BulkSchedulerService;`).
2. The code uses `LogCaptor` from `nl.altindag.log:log-captor`. If this dependency is not yet present in your test classpath, add it to your build (e.g., in Maven: `test`-scoped dependency `nl.altindag:log-captor`).
3. The test uses `assertTrue`; if this static import is not already present, add `import static org.junit.jupiter.api.Assertions.assertTrue;` to the test file.
4. If your project already has a different logging test helper or pattern, you may prefer to swap `LogCaptor` usage for that helper while keeping the same assertion semantics: verifying that an error log contains `BulkSchedulerConstants.ERROR_NO_DATASOURCES_AVAILABLE`.
</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
+24
to
+26
| # New - comma-separated list of datasources (takes precedence over single datasource) | ||
| # Example: "thanos-1,prometheus-1" | ||
| - name: KRUIZE_DEFAULT_DATASOURCES |
There was a problem hiding this comment.
issue (bug_risk): OpenShift overlay repeats the KRUIZE_DEFAULT_DATASOURCES vs KRUIZE_DATASOURCES mismatch.
This overlay sets KRUIZE_DEFAULT_DATASOURCES, but application.yml reads KRUIZE_DATASOURCES, so the configured list ("thanos-1,cryostat-2") will not affect kruize.defaults.datasources. Please align the env var name with application.yml to ensure the list is actually used.
Comment on lines
210
to
+219
| void testScheduledBulkApiCall_NoDatasource() { | ||
| // Arrange | ||
| when(kruizeStateService.isCacheEmpty()).thenReturn(false); | ||
| when(kruizeStateService.getDefaultDatasourceName()).thenReturn(Optional.empty()); | ||
| when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(Collections.emptyList()); | ||
|
|
||
| // Act | ||
| bulkSchedulerService.initialize(); | ||
| bulkSchedulerService.scheduledBulkApiCall(); | ||
|
|
||
| // Assert - Should not call bulk API | ||
| // Assert |
There was a problem hiding this comment.
suggestion (testing): Consider asserting that the correct error message is logged when no datasources are available.
The Javadoc for testScheduledBulkApiCall_NoDatasource documents that ERROR_NO_DATASOURCES_AVAILABLE should be logged, but this test only checks that the bulk API is not called and the jobs counter is not incremented. If you have a logging test helper or can inject a logger/appender, please also assert that ERROR_NO_DATASOURCES_AVAILABLE is emitted so the test fully verifies the documented behavior and use of the new constant.
Suggested implementation:
import com.kruize.optimizer.client.KruizeClient;
import com.kruize.optimizer.util.MockResponseLoader;
import com.kruize.optimizer.utils.OptimizerConstants.BulkSchedulerConstants;
import io.quarkus.test.InjectMock;
import io.quarkus.test.junit.QuarkusTest;
import jakarta.inject.Inject;
import nl.altindag.log.LogCaptor;
import org.mockito.Mockito;
import java.io.IOException;
import java.util.Collections; */
@Test
void testScheduledBulkApiCall_NoDatasource() {
// Arrange
when(kruizeStateService.isCacheEmpty()).thenReturn(false);
when(kruizeStateService.getDefaultDatasourceNames()).thenReturn(Collections.emptyList());
LogCaptor logCaptor = LogCaptor.forClass(BulkSchedulerService.class);
// Act
bulkSchedulerService.initialize();
bulkSchedulerService.scheduledBulkApiCall();
// Assert
// - Bulk API not called
verify(kruizeClient, never()).bulkCreateExperiments(any());
// - Jobs counter not incremented
verify(jobsService, never()).incrementJobsTriggered();
// - Error about missing datasources is logged
assertTrue(
logCaptor.getErrorLogs()
.stream()
.anyMatch(message -> message.contains(BulkSchedulerConstants.ERROR_NO_DATASOURCES_AVAILABLE)),
"Expected ERROR_NO_DATASOURCES_AVAILABLE to be logged when no datasources are available"
);
}- This change assumes the presence of the
BulkSchedulerServiceclass in the same package; ensure the test can referenceBulkSchedulerService.class(add an import if needed:import com.kruize.optimizer.service.BulkSchedulerService;). - The code uses
LogCaptorfromnl.altindag.log:log-captor. If this dependency is not yet present in your test classpath, add it to your build (e.g., in Maven:test-scoped dependencynl.altindag:log-captor). - The test uses
assertTrue; if this static import is not already present, addimport static org.junit.jupiter.api.Assertions.assertTrue;to the test file. - If your project already has a different logging test helper or pattern, you may prefer to swap
LogCaptorusage for that helper while keeping the same assertion semantics: verifying that an error log containsBulkSchedulerConstants.ERROR_NO_DATASOURCES_AVAILABLE.
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
2 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 adds support for adding multiple datasources as part of experiment creation.
Summary by Sourcery
Add multi-datasource support to the bulk experiment scheduler while preserving backward compatibility with the existing single-datasource configuration.
New Features:
Enhancements:
Build:
Tests: