PR#8 Add support for experiment_types in Bulk API - #2002
Open
khansaad wants to merge 16 commits into
Open
Conversation
Contributor
Reviewer's GuideAdds new Bulk API capabilities for experiment type selection, cluster/model/term customization, and cluster-aware datasources, along with validation, notifications, DB schema support, and comprehensive tests. Sequence diagram for Bulk API experiment_type handlingsequenceDiagram
actor Client
participant BulkService as BulkServiceValidation
participant BulkJob as BulkJobManager
participant BulkInput as BulkInput
Client->>BulkService: validate(payload, jobID)
BulkService->>BulkService: validateExperimentTypes(payload.getExperiment_types())
BulkService-->>Client: ValidationOutputData (success)
Client->>BulkJob: startBulkJob(BulkInput)
BulkJob->>BulkJob: getExperimentMap(labelString, jobData, metadataInfo, datasource)
BulkJob->>BulkJob: resolveExperimentType(BulkInput.getExperiment_types())
alt ExperimentType.NAMESPACE
BulkJob->>BulkJob: frameNamespaceExperimentName(...)
BulkJob->>BulkJob: prepareNamespaceExperimentJSONInput(...)
else ExperimentType.CONTAINER (default)
BulkJob->>BulkJob: frameExperimentName(...)
BulkJob->>BulkJob: prepareCreateExperimentJSONInput(...)
end
BulkJob-->>Client: Experiments created for selected experiment_type
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The new
experiment_typeshandling inBulkServiceValidationallows a list, butBulkJobManager.resolveExperimentTypeonly uses the first entry and silently ignores the rest; either enforce a single element at validation time or update the job manager to actually support multiple experiment types as implied by the API and PR title. - In
DataSourceInfoAdapter, the authentication field is serialized asauthenticationConfig, whereas the existing datasources design and JSON examples use theauthenticationkey – align the serialized field name with the rest of the API to avoid breaking clients consuming/listDatasources.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `experiment_types` handling in `BulkServiceValidation` allows a list, but `BulkJobManager.resolveExperimentType` only uses the first entry and silently ignores the rest; either enforce a single element at validation time or update the job manager to actually support multiple experiment types as implied by the API and PR title.
- In `DataSourceInfoAdapter`, the authentication field is serialized as `authenticationConfig`, whereas the existing datasources design and JSON examples use the `authentication` key – align the serialized field name with the rest of the API to avoid breaking clients consuming `/listDatasources`.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/common/bulk/BulkServiceValidation.java" line_range="35-36" />
<code_context>
+import java.lang.reflect.Type;
+import java.util.List;
+
+/**
+ * Custom Gson serializer for DataSourceInfo to conditionally exclude empty clusters field
+ */
</code_context>
<issue_to_address>
**issue:** JavaDoc for validateExperimentTypes mentions duplicate checks that are not implemented in the method body.
The method currently only checks for non-empty values and membership in VALID_EXPERIMENT_TYPES, but not duplicates as documented. Please either add duplicate detection (e.g., track lowercased values in a Set and fail on repeats) or update the JavaDoc so it accurately describes the existing behavior.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/analyzer/workerimpl/BulkJobManager.java" line_range="699-701" />
<code_context>
+ * @param experimentTypes the raw list from BulkInput.experiment_types
+ * @return the resolved ExperimentType
+ */
+ private AnalyzerConstants.ExperimentType resolveExperimentType(List<String> experimentTypes) {
+ if (experimentTypes == null || experimentTypes.isEmpty()) {
+ return AnalyzerConstants.ExperimentType.CONTAINER;
+ }
+ try {
</code_context>
<issue_to_address>
**issue (bug_risk):** resolveExperimentType ignores all but the first experiment_types entry, which may conflict with expectations from the BulkInput contract.
BulkInput exposes experiment_types as a List<String> and BulkServiceValidation permits multiple entries, but this method always uses experimentTypes.get(0). If a caller passes both "container" and "namespace", only the first is used with no warning. Either enforce and document a single allowed experiment type at validation time, or update orchestration to support multiple experiment types instead of silently discarding the rest.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/autotune/common/datasource/DataSourceInfo.java" line_range="165-166" />
<code_context>
+ *
+ * @return list of cluster names; empty list if no clusters were provided
+ */
+ public List<String> getClusters() {
+ return clusters;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** getClusters exposes the internal mutable list, which can be modified by callers and break the constructor invariant.
Because this returns the internal mutable list, callers can change DataSourceInfo’s state after construction, violating the “set once at construction time” contract. To preserve the invariant and prevent accidental mutation, return an unmodifiable view (e.g., Collections.unmodifiableList(clusters)) or a defensive copy instead.
Suggested implementation:
```java
/**
* Returns the list of cluster names associated with this datasource.
* The {@code clusters} field is final and set once at construction time.
* To preserve the immutability contract and prevent callers from mutating
* the internal state, this method returns an unmodifiable view of the list.
*
* @return unmodifiable list of cluster names; empty list if no clusters were provided
*/
public List<String> getClusters() {
return Collections.unmodifiableList(clusters);
}
```
```java
import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
```
</issue_to_address>
### Comment 4
<location path="src/test/java/com/autotune/analyzer/workerimpl/BulkJobManagerPassthroughTest.java" line_range="96-105" />
<code_context>
+ @DisplayName("Backward Compatibility Tests")
+ class BackwardCompatibilityTests {
+
+ @Test
+ @DisplayName("Should maintain existing behavior when cluster name not provided")
+ void shouldMaintainExistingBehaviorWhenClusterNameNotProvided() {
+ // Given - Old-style bulk input without cluster name
+ when(bulkInput.getCluster_name()).thenReturn(null);
+
+ // When
+ String experimentName = bulkJobManager.frameExperimentName(
+ null, cluster, namespace, workload, container
+ );
+
+ // Then
+ assertTrue(experimentName.contains("metadata-cluster"),
+ "Should use metadata cluster when bulk payload cluster is not provided");
+ assertEquals("prometheus-metadata-cluster-default-test-app-deployment-app-container",
+ experimentName,
+ "Experiment name should follow existing format");
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen BulkJobManagerPassthroughTest by asserting the actual experiment payload uses the overridden cluster_name and experiment_type.
Current tests only read `bulkInput.getCluster_name()` or `frameExperimentName` and never exercise `getExperimentMap`/`prepareCreateExperimentJSONInput`. As a result, they don’t verify that `CreateExperimentAPIObject` actually uses the trimmed `cluster_name` override or the resolved `experiment_type` (`NAMESPACE` vs `CONTAINER`). To validate the new behavior, drive `BulkJobManager` end‑to‑end with a minimal `metadataInfo` (cluster/namespace/workload/container), call `getExperimentMap`, and assert the resulting experiment’s `clusterName`, `experimentType`, and `kubernetesObjects` fields.
Suggested implementation:
```java
import com.autotune.analyzer.serviceObjects.BulkInput;
import com.autotune.analyzer.experiment.CreateExperimentAPIObject;
```
```java
import com.autotune.operator.KruizeDeploymentInfo;
import org.junit.jupiter.api.AfterEach;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
```
```java
@Nested
@DisplayName("Cluster Name Passthrough Tests")
class ClusterNamePassthroughTests {
@Test
@DisplayName("Should propagate trimmed cluster name and resolved experiment type into experiment payload")
void shouldPropagateClusterNameAndExperimentTypeIntoExperimentPayload() {
// Given - bulk payload overrides cluster_name with surrounding whitespace
when(bulkInput.getCluster_name()).thenReturn(" my-metadata-cluster ");
// Let BulkJobManager resolve the experiment type from metadata (e.g. NAMESPACE/CONTAINER)
when(bulkInput.getExperiment_type()).thenReturn(null);
Map<String, Object> metadataInfo = new HashMap<>();
metadataInfo.put("cluster_name", "my-metadata-cluster");
metadataInfo.put("namespace", "default");
metadataInfo.put("workload_name", "test-app-deployment");
metadataInfo.put("workload_type", "deployment");
metadataInfo.put("container_name", "app-container");
// When - drive BulkJobManager end-to-end through getExperimentMap
Map<String, CreateExperimentAPIObject> experimentMap =
bulkJobManager.getExperimentMap(Collections.singletonList(metadataInfo), bulkInput);
// Then
assertEquals(1, experimentMap.size(), "Exactly one experiment should be generated from the metadata");
CreateExperimentAPIObject experiment = experimentMap.values().iterator().next();
// clusterName should use the trimmed bulkInput cluster_name override
assertEquals(
"my-metadata-cluster",
experiment.getClusterName(),
"Experiment clusterName should use trimmed bulk cluster_name override"
);
// experimentType in payload should match the resolved experiment type used by BulkJobManager
String resolvedExperimentType = bulkJobManager.getExperimentType(metadataInfo, bulkInput);
assertEquals(
resolvedExperimentType,
experiment.getExperimentType(),
"Experiment experimentType should match resolved experiment type"
);
// kubernetesObjects should reflect metadataInfo fields
assertFalse(
experiment.getKubernetesObjects().isEmpty(),
"Experiment payload should contain kubernetesObjects"
);
CreateExperimentAPIObject.KubernetesObject ko = experiment.getKubernetesObjects().get(0);
assertEquals("default", ko.getNamespace(), "Namespace should match metadataInfo");
assertEquals("test-app-deployment", ko.getWorkloadName(), "Workload name should match metadataInfo");
assertEquals("deployment", ko.getWorkloadType(), "Workload type should match metadataInfo");
assertEquals("app-container", ko.getContainerName(), "Container name should match metadataInfo");
}
```
Depending on the existing test fixture in `BulkJobManagerPassthroughTest`, you may need to:
1. Ensure `bulkJobManager` and `bulkInput` are already initialized/mocked in a `@BeforeEach` method and accessible from the nested class (e.g. as fields in the outer test class). If they are defined with narrower scope, move them to fields or adjust visibility.
2. If the actual experiment payload type is not `com.autotune.analyzer.experiment.CreateExperimentAPIObject` or uses different accessor names (`getClusterName`, `getExperimentType`, `getKubernetesObjects`, `getNamespace`, `getWorkloadName`, `getWorkloadType`, `getContainerName`), update the import and method calls accordingly.
3. If `BulkJobManager` exposes a different API for deriving experiment type than `getExperimentType(metadataInfo, bulkInput)`, replace that call with the appropriate method or inline the expected value (e.g. `"NAMESPACE"` or `"CONTAINER"`) based on how your production code resolves it.
4. If `getExperimentMap` returns a different generic type (e.g. `Map<String, KruizeObject>` or a wrapper object before reaching `CreateExperimentAPIObject`), adapt the test to extract the `CreateExperimentAPIObject` from that structure before asserting on `clusterName`, `experimentType`, and `kubernetesObjects`.
</issue_to_address>
### Comment 5
<location path="design/BulkAPI.md" line_range="107-108" />
<code_context>
- **metadata_profile:** Name of the metadata profile to import the cluster metadata. This is a mandatory field `metadata_profile`
should be installed / created before invoking bulk API.
-- **measurement_duration:** The historic data duration to fetch the cluster metadata. This is an optional field, if not
+- **measurement_duration:** The historic data duration to fetch the cluster metadata. This is an optional field, if not
specified `15min` as default measurement_duration value is considered.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider rephrasing the measurement_duration description for clearer grammar and wording.
"Historic data duration" reads a bit awkward; "historical data duration" is more standard here. Also, the sentence currently uses a comma splice—consider something like: "This is an optional field; if not specified, `15min` is used as the default measurement_duration value."
```suggestion
- **measurement_duration:** The historical data duration used to fetch the cluster metadata. This is an optional field; if not
specified, `15min` is used as the default measurement_duration value.
```
</issue_to_address>
### Comment 6
<location path="design/NotificationCodes.md" line_range="92" />
<code_context>
+| 324006 | NOTICE | MEMORY_REQUESTS_OVER_PROVISIONED | Specifies that the workload is over-provisioned for Memory requests | Workload is over-provisioned for Memory. Kruize recommends reducing Memory allocation to optimize costs. | DATA USER |
</code_context>
<issue_to_address>
**nitpick (typo):** Spelling of "optimize" is inconsistent with existing "optimised" in the table.
This entry uses “optimize” while others in the table use “optimised” (e.g., “Workload is optimised wrt CPU REQUESTS”). Please align the spelling within the table, either by changing this to “optimised” or updating the earlier entries to “optimized.”
```suggestion
| 324006 | NOTICE | MEMORY_REQUESTS_OVER_PROVISIONED | Specifies that the workload is over-provisioned for Memory requests | Workload is over-provisioned for Memory. Kruize recommends reducing Memory allocation to optimise costs. | DATA USER |
```
</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
35
to
36
| /** | ||
| * Utility class that performs validation for bulk service requests. |
Contributor
There was a problem hiding this comment.
issue: JavaDoc for validateExperimentTypes mentions duplicate checks that are not implemented in the method body.
The method currently only checks for non-empty values and membership in VALID_EXPERIMENT_TYPES, but not duplicates as documented. Please either add duplicate detection (e.g., track lowercased values in a Set and fail on repeats) or update the JavaDoc so it accurately describes the existing behavior.
Comment on lines
+699
to
+701
| private AnalyzerConstants.ExperimentType resolveExperimentType(List<String> experimentTypes) { | ||
| if (experimentTypes == null || experimentTypes.isEmpty()) { | ||
| return AnalyzerConstants.ExperimentType.CONTAINER; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): resolveExperimentType ignores all but the first experiment_types entry, which may conflict with expectations from the BulkInput contract.
BulkInput exposes experiment_types as a List and BulkServiceValidation permits multiple entries, but this method always uses experimentTypes.get(0). If a caller passes both "container" and "namespace", only the first is used with no warning. Either enforce and document a single allowed experiment type at validation time, or update orchestration to support multiple experiment types instead of silently discarding the rest.
Comment on lines
+165
to
+166
| public List<String> getClusters() { | ||
| return clusters; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): getClusters exposes the internal mutable list, which can be modified by callers and break the constructor invariant.
Because this returns the internal mutable list, callers can change DataSourceInfo’s state after construction, violating the “set once at construction time” contract. To preserve the invariant and prevent accidental mutation, return an unmodifiable view (e.g., Collections.unmodifiableList(clusters)) or a defensive copy instead.
Suggested implementation:
/**
* Returns the list of cluster names associated with this datasource.
* The {@code clusters} field is final and set once at construction time.
* To preserve the immutability contract and prevent callers from mutating
* the internal state, this method returns an unmodifiable view of the list.
*
* @return unmodifiable list of cluster names; empty list if no clusters were provided
*/
public List<String> getClusters() {
return Collections.unmodifiableList(clusters);
}import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Comment on lines
+96
to
+105
| @Test | ||
| @DisplayName("Should return cluster name from bulk payload when provided") | ||
| void shouldReturnClusterNameFromBulkPayload() { | ||
| // Given | ||
| when(bulkInput.getCluster_name()).thenReturn("custom-cluster"); | ||
|
|
||
| // When | ||
| String clusterName = bulkInput.getCluster_name(); | ||
|
|
||
| // Then |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen BulkJobManagerPassthroughTest by asserting the actual experiment payload uses the overridden cluster_name and experiment_type.
Current tests only read bulkInput.getCluster_name() or frameExperimentName and never exercise getExperimentMap/prepareCreateExperimentJSONInput. As a result, they don’t verify that CreateExperimentAPIObject actually uses the trimmed cluster_name override or the resolved experiment_type (NAMESPACE vs CONTAINER). To validate the new behavior, drive BulkJobManager end‑to‑end with a minimal metadataInfo (cluster/namespace/workload/container), call getExperimentMap, and assert the resulting experiment’s clusterName, experimentType, and kubernetesObjects fields.
Suggested implementation:
import com.autotune.analyzer.serviceObjects.BulkInput;
import com.autotune.analyzer.experiment.CreateExperimentAPIObject;import com.autotune.operator.KruizeDeploymentInfo;
import org.junit.jupiter.api.AfterEach;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map; @Nested
@DisplayName("Cluster Name Passthrough Tests")
class ClusterNamePassthroughTests {
@Test
@DisplayName("Should propagate trimmed cluster name and resolved experiment type into experiment payload")
void shouldPropagateClusterNameAndExperimentTypeIntoExperimentPayload() {
// Given - bulk payload overrides cluster_name with surrounding whitespace
when(bulkInput.getCluster_name()).thenReturn(" my-metadata-cluster ");
// Let BulkJobManager resolve the experiment type from metadata (e.g. NAMESPACE/CONTAINER)
when(bulkInput.getExperiment_type()).thenReturn(null);
Map<String, Object> metadataInfo = new HashMap<>();
metadataInfo.put("cluster_name", "my-metadata-cluster");
metadataInfo.put("namespace", "default");
metadataInfo.put("workload_name", "test-app-deployment");
metadataInfo.put("workload_type", "deployment");
metadataInfo.put("container_name", "app-container");
// When - drive BulkJobManager end-to-end through getExperimentMap
Map<String, CreateExperimentAPIObject> experimentMap =
bulkJobManager.getExperimentMap(Collections.singletonList(metadataInfo), bulkInput);
// Then
assertEquals(1, experimentMap.size(), "Exactly one experiment should be generated from the metadata");
CreateExperimentAPIObject experiment = experimentMap.values().iterator().next();
// clusterName should use the trimmed bulkInput cluster_name override
assertEquals(
"my-metadata-cluster",
experiment.getClusterName(),
"Experiment clusterName should use trimmed bulk cluster_name override"
);
// experimentType in payload should match the resolved experiment type used by BulkJobManager
String resolvedExperimentType = bulkJobManager.getExperimentType(metadataInfo, bulkInput);
assertEquals(
resolvedExperimentType,
experiment.getExperimentType(),
"Experiment experimentType should match resolved experiment type"
);
// kubernetesObjects should reflect metadataInfo fields
assertFalse(
experiment.getKubernetesObjects().isEmpty(),
"Experiment payload should contain kubernetesObjects"
);
CreateExperimentAPIObject.KubernetesObject ko = experiment.getKubernetesObjects().get(0);
assertEquals("default", ko.getNamespace(), "Namespace should match metadataInfo");
assertEquals("test-app-deployment", ko.getWorkloadName(), "Workload name should match metadataInfo");
assertEquals("deployment", ko.getWorkloadType(), "Workload type should match metadataInfo");
assertEquals("app-container", ko.getContainerName(), "Container name should match metadataInfo");
}Depending on the existing test fixture in BulkJobManagerPassthroughTest, you may need to:
- Ensure
bulkJobManagerandbulkInputare already initialized/mocked in a@BeforeEachmethod and accessible from the nested class (e.g. as fields in the outer test class). If they are defined with narrower scope, move them to fields or adjust visibility. - If the actual experiment payload type is not
com.autotune.analyzer.experiment.CreateExperimentAPIObjector uses different accessor names (getClusterName,getExperimentType,getKubernetesObjects,getNamespace,getWorkloadName,getWorkloadType,getContainerName), update the import and method calls accordingly. - If
BulkJobManagerexposes a different API for deriving experiment type thangetExperimentType(metadataInfo, bulkInput), replace that call with the appropriate method or inline the expected value (e.g."NAMESPACE"or"CONTAINER") based on how your production code resolves it. - If
getExperimentMapreturns a different generic type (e.g.Map<String, KruizeObject>or a wrapper object before reachingCreateExperimentAPIObject), adapt the test to extract theCreateExperimentAPIObjectfrom that structure before asserting onclusterName,experimentType, andkubernetesObjects.
Comment on lines
+107
to
108
| - **measurement_duration:** The historic data duration to fetch the cluster metadata. This is an optional field, if not | ||
| specified `15min` as default measurement_duration value is considered. |
Contributor
There was a problem hiding this comment.
suggestion (typo): Consider rephrasing the measurement_duration description for clearer grammar and wording.
"Historic data duration" reads a bit awkward; "historical data duration" is more standard here. Also, the sentence currently uses a comma splice—consider something like: "This is an optional field; if not specified, 15min is used as the default measurement_duration value."
Suggested change
| - **measurement_duration:** The historic data duration to fetch the cluster metadata. This is an optional field, if not | |
| specified `15min` as default measurement_duration value is considered. | |
| - **measurement_duration:** The historical data duration used to fetch the cluster metadata. This is an optional field; if not | |
| specified, `15min` is used as the default measurement_duration value. |
| | 324003 | NOTICE | MEMORY_REQUESTS_OPTIMISED | Specifies that the workload is optimised wrt MEMORY REQUESTS | Workload is optimised wrt MEMORY REQUESTS, no changes needed | DATA USER | | ||
| | 324004 | NOTICE | MEMORY_LIMITS_OPTIMISED | Specifies that the workload is optimised wrt MEMORY LIMITS | Workload is optimised wrt MEMORY LIMITS, no changes needed | DATA USER | | ||
| | 324005 | NOTICE | MEMORY_REQUESTS_UNDER_PROVISIONED | Specifies that the workload is under-provisioned for Memory requests | Workload is under-provisioned for Memory. Kruize recommends increasing Memory allocation. | DATA USER | | ||
| | 324006 | NOTICE | MEMORY_REQUESTS_OVER_PROVISIONED | Specifies that the workload is over-provisioned for Memory requests | Workload is over-provisioned for Memory. Kruize recommends reducing Memory allocation to optimize costs. | DATA USER | |
Contributor
There was a problem hiding this comment.
nitpick (typo): Spelling of "optimize" is inconsistent with existing "optimised" in the table.
This entry uses “optimize” while others in the table use “optimised” (e.g., “Workload is optimised wrt CPU REQUESTS”). Please align the spelling within the table, either by changing this to “optimised” or updating the earlier entries to “optimized.”
Suggested change
| | 324006 | NOTICE | MEMORY_REQUESTS_OVER_PROVISIONED | Specifies that the workload is over-provisioned for Memory requests | Workload is over-provisioned for Memory. Kruize recommends reducing Memory allocation to optimize costs. | DATA USER | | |
| | 324006 | NOTICE | MEMORY_REQUESTS_OVER_PROVISIONED | Specifies that the workload is over-provisioned for Memory requests | Workload is over-provisioned for Memory. Kruize recommends reducing Memory allocation to optimise costs. | DATA USER | |
khansaad
changed the title
- Add cluster_name, model_settings, term_settings fields to BulkInput - Implement passthrough logic in BulkJobManager - Add comprehensive validation for new fields - Maintain backward compatibility (all fields optional) Enables Optimizer to specify: - Cluster name for experiments - Custom recommendation models (performance, cost) - Custom recommendation terms (short, medium, long) Signed-off-by: Saad Khan <saakhan@ibm.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
This reverts commit 4e9325a.
This reverts commit e0fbcef.
This reverts commit f3186c8.
…Bulk API" This reverts commit da2343a.
- Add DataSourceInfoAdapter for JSON serialization with cluster list - Update ListDatasources to use adapter for proper JSON output - Add cluster validation and filtering in DataSourceCollection - Add null check in DBHelpers to prevent NPE when URL is null - Change log level from INFO to DEBUG to reduce noise This PR integrates cluster_name support into the datasource layer, enabling datasources to be associated with multiple clusters.
- BulkInput: added cluster_name field with getter/setter; updated isEmpty() - BulkJobManager: use cluster_name from bulk payload if provided (trimmed), falling back to datasource metadata cluster name - BulkServiceValidation: added validateClusterName() with same rules as the deleted ClusterNameUtils.validateClusterName (null=valid, blank=error, >253 chars=error); removed ClusterNameUtils dependency
- Add BulkServiceValidationTest for cluster_name validation - Add KruizeDataSourceEntryTest for cluster list operations and filtering - Update test_bulkAPI.py with cluster_name and model_settings validation - Remove DNS-1123 validation tests, keep only length/emptiness checks - Add tests for filtering invalid cluster names (empty strings, too long) - Complete model_settings validation test with API call and assertions This PR provides comprehensive test coverage for cluster name validation and filtering behavior across Java and Python test suites.
- BulkAPI.md: document cluster_name field (optional, overrides metadata cluster, max 253 chars) - KruizeDatasource.md: document clusters field as JSONB array, add Cluster Configuration section with single/multi cluster examples, behavior, use cases and API response sample - Updated clusters field type description to JSONB to match DB schema - Updated cluster_name validation description to match actual implementation (non-empty, max 253 chars)
- BulkInput: added model_settings and term_settings fields with getters/setters; updated isEmpty() to include new fields - BulkJobManager: pass model_settings and term_settings from bulk payload to RecommendationSettings - BulkServiceValidation: added validateModelSettings() and validateTermSettings() with case-insensitive validation; no ClusterNameUtils dependency - test_bulkAPI.py: comprehensive parametrized tests for model_settings and term_settings including whitespace, omitted fields, invalid names - BulkAPI.md: document model_settings and term_settings parameters
- RecommendationConstants: added NOTICE_CPU/MEMORY_REQUESTS_UNDER/OVER_PROVISIONED notification codes (323006, 323007, 324005, 324006) and message constants - RecommendationEngine: emit under/over-provisioned notifications when recommendation threshold is exceeded for CPU and memory requests - NotificationCodes.md: document new notification codes - test_bulkAPI.py: fix cluster_name test expectations to match actual validation (length/empty only, not DNS format); add backward compatibility test; improve combined settings test
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
khansaad
force-pushed
the
bulk-exp-type-support
branch
from
July 9, 2026 09:48
340e665 to
e3d290b
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
4 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.
Description
This PR adds changes to support multiple experiment types in bulk.
Fixes # (issue)
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
Extend Bulk API and datasource handling to support configurable experiment types, cluster scoping, and recommendation settings while enriching validation, notifications, and documentation.
New Features:
Enhancements:
Documentation:
Tests: