PR#8 Add support for experiment_types in Bulk API - #2002
Open
khansaad wants to merge 5 commits into
Open
Conversation
Contributor
Reviewer's GuideExtends Bulk API processing with validated, single-type experiment selection, including namespace-level experiment creation while preserving container defaults. Bulk-level cluster and recommendation settings are propagated to generated experiments, with supporting dependency and documentation updates. Sequence diagram for Bulk API experiment type processingsequenceDiagram
participant Client
participant BulkServiceValidation
participant BulkJobManager
participant Metadata
participant CreateExperimentAPI
Client->>BulkServiceValidation: validate(payload, jobID)
BulkServiceValidation->>BulkServiceValidation: validateExperimentTypes(experiment_types)
BulkServiceValidation-->>BulkJobManager: valid request
BulkJobManager->>BulkJobManager: resolveExperimentType(experiment_types)
BulkJobManager->>Metadata: getDatasources()
alt experiment type is namespace
BulkJobManager->>BulkJobManager: frameNamespaceExperimentName(labelString, dataSourceCluster, namespace)
BulkJobManager->>BulkJobManager: prepareNamespaceExperimentJSONInput(dsc, namespace, experiment_name, objects)
else absent or container type
BulkJobManager->>BulkJobManager: frameExperimentName(labelString, clusterName, namespace, workload, container)
BulkJobManager->>BulkJobManager: prepareCreateExperimentJSONInput(container, clusterName, workload, namespace, experiment_name, objects)
end
BulkJobManager->>CreateExperimentAPI: create experiments with recommendation settings
Flow diagram for Bulk API experiment type selectionflowchart TD
A[Bulk API payload] --> B{experiment_types provided?}
B -->|No or empty| C[Resolve CONTAINER]
B -->|One container value| C
B -->|One namespace value| D[Resolve NAMESPACE]
B -->|More than one or invalid value| E[Return validation error]
C --> F[Create container experiments]
D --> G[Create one namespace experiment per namespace]
F --> H[Propagate cluster_name, model_settings, term_settings]
G --> H
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.
khansaad
changed the title
khansaad
force-pushed
the
bulk-exp-type-support
branch
from
July 9, 2026 09:48
340e665 to
e3d290b
Compare
khansaad
force-pushed
the
bulk-exp-type-support
branch
8 times, most recently
from
August 3, 2026 03:27
3270b6e to
0bfff4f
Compare
bharathappali
requested changes
Aug 3, 2026
kusumachalasani
requested changes
Aug 10, 2026
Contributor
|
@khansaad Please update bulk documentation that we are supporting only one experiment_type for now. As it is a list, it might be confusing to users. We can mention multiple types are supported in future. |
Contributor
Author
Done |
bharathappali
requested changes
Aug 19, 2026
mbvreddy
reviewed
Aug 27, 2026
| } | ||
|
|
||
| public void setExperiment_types(List<String> experiment_types) { | ||
| if (experiment_types != null && !experiment_types.isEmpty()) { |
Contributor
There was a problem hiding this comment.
We can leverage apache commons lang utility function https://commons.apache.org/proper/commons-collections/javadocs/api-3.2.2/org/apache/commons/collections/CollectionUtils.html#isEmpty(java.util.Collection) here
mbvreddy
reviewed
Aug 27, 2026
| } | ||
| // Validation guarantees exactly one entry; get(0) is intentional. | ||
| try { | ||
| return AnalyzerConstants.ExperimentType.valueOf(experimentTypes.get(0).trim().toUpperCase()); |
Contributor
There was a problem hiding this comment.
If I understand correctly, this is for now. In future, we support multiple types. Correct me.
mbvreddy
reviewed
Aug 27, 2026
| * If provided, only experiments of the specified type(s) will be created. | ||
| * If not provided or empty, defaults to container experiments. | ||
| */ | ||
| private List<String> experiment_types; |
Contributor
There was a problem hiding this comment.
We can store experiment_types as list of enum instead of String. This helps in upfront validation and avoid converting it to enum at later point in time via resolveExperimentType.
Contributor
Author
There was a problem hiding this comment.
Made sense.
Updated now
mbvreddy
reviewed
Aug 27, 2026
| * @return an error message if validation fails; otherwise an empty string | ||
| */ | ||
| public static String validateExperimentTypes(List<String> experimentTypes) { | ||
| if (experimentTypes == null || experimentTypes.isEmpty()) { |
Contributor
There was a problem hiding this comment.
Here also, replace with CollectionUtils method.
mbvreddy
requested changes
Aug 27, 2026
khansaad
force-pushed
the
bulk-exp-type-support
branch
from
August 27, 2026 11:58
336e9cc to
2007fbb
Compare
- Add experiment_types field to BulkInput (container/namespace) - Add resolveExperimentType and namespace experiment creation path (prepareNamespaceExperimentJSONInput, frameNamespaceExperimentName) - Add validateExperimentTypes to BulkServiceValidation - Add BULK_INVALID_EXPERIMENT_TYPES error constant
Signed-off-by: Saad Khan <saakhan@ibm.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
khansaad
force-pushed
the
bulk-exp-type-support
branch
from
August 27, 2026 12:04
2007fbb to
f57ba94
Compare
mbvreddy
reviewed
Aug 28, 2026
| * (null, empty, or exactly one recognized value) | ||
| * @return the resolved ExperimentType | ||
| */ | ||
| private AnalyzerConstants.ExperimentType resolveExperimentType(List<AnalyzerConstants.ExperimentType> experimentTypes) { |
Contributor
There was a problem hiding this comment.
Now that we have started storing experiment_types as ENUM, I don't think the keyword resolve in method name is apt. There is nothing to resolve. Do we need this method?
mbvreddy
reviewed
Aug 28, 2026
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-collections4</artifactId> | ||
| <version>${commons-collections4-version}</version> | ||
| </dependency> |
Contributor
There was a problem hiding this comment.
If it requires to add this additional library, lets not do that only for couple of instances. This will create additional overhead in patching this time to time.
Lets go back to check for null and empty explicitly and remove this additional dependency.
mbvreddy
approved these changes
Aug 28, 2026
mbvreddy
requested changes
Aug 28, 2026
mbvreddy
left a comment
mbvreddy
left a comment
Contributor
There was a problem hiding this comment.
Posted few more comments. Please check.
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/utils/AnalyzerConstants.java" line_range="332-336" />
<code_context>
- WORKLOAD // For application-specific experiments
+ WORKLOAD; // For application-specific experiments
+
+ @JsonCreator
+ public static ExperimentType fromString(String value) {
+ if (value == null) return null;
+ return ExperimentType.valueOf(value.trim().toUpperCase());
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** `ExperimentType.fromString` throws `IllegalArgumentException` for an invalid or empty `experiment_types` value, so `ObjectMapper.readValue` fails before `BulkServiceValidation.validateExperimentTypes` runs and the API cannot return the intended `BULK_INVALID_EXPERIMENT_TYPES` validation response.
**Triggers:** When a request contains an unsupported, empty, or otherwise malformed experiment type.
**Suggested fix:** Return a nullable/invalid marker that validation can inspect, or catch the enum conversion exception in the request layer and map it to the bulk validation error.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/analyzer/workerimpl/BulkJobManager.java" line_range="869-873" />
<code_context>
+ String clusterName = dataSourceCluster.getDataSourceClusterName();
+ String namespaceName = namespace.getNamespace();
+
+ // Namespace experiment name: datasource|clustername|namespace
+ String experimentName = KruizeDeploymentInfo.namespace_experiment_name_format
+ .replace("%datasource%", datasource)
+ .replace("%clustername%", clusterName)
+ .replace("%namespace%", namespaceName);
+
+ if (null != labelString) {
</code_context>
<issue_to_address>
**issue (broader_impact):** Namespace experiments use the datasource metadata cluster in their experiment name, while `prepareNamespaceExperimentJSONInput` uses the trimmed `bulkInput.cluster_name` in the experiment payload. With a cluster override, the created experiment name identifies one cluster but its `cluster_name` field identifies another, breaking the name/cluster identity invariant and causing inconsistent lookups or duplicate experiments.
**Triggers:** When a namespace bulk request supplies `cluster_name` that differs from the cluster name in datasource metadata.
**Suggested fix:** Resolve and trim the cluster name once, pass it to `frameNamespaceExperimentName`, and use that same value for both the experiment name and payload.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if the type resolution or namespace payload is wrong, the bulk job can create the wrong set of persisted experiments and associated recommendation data; reverting the code will not remove those records, though the impact is bounded and can be repaired by cleanup and rerunning the job.
Blocking findings: src/main/java/com/autotune/analyzer/utils/AnalyzerConstants.java:336, src/main/java/com/autotune/analyzer/workerimpl/BulkJobManager.java:873
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+332
to
+336
| @JsonCreator | ||
| public static ExperimentType fromString(String value) { | ||
| if (value == null) return null; | ||
| return ExperimentType.valueOf(value.trim().toUpperCase()); | ||
| } |
Contributor
There was a problem hiding this comment.
issue (bug_risk): ExperimentType.fromString throws IllegalArgumentException for an invalid or empty experiment_types value, so ObjectMapper.readValue fails before BulkServiceValidation.validateExperimentTypes runs and the API cannot return the intended BULK_INVALID_EXPERIMENT_TYPES validation response.
Triggers: When a request contains an unsupported, empty, or otherwise malformed experiment type.
Suggested fix: Return a nullable/invalid marker that validation can inspect, or catch the enum conversion exception in the request layer and map it to the bulk validation error.
Comment on lines
+869
to
+873
| // Namespace experiment name: datasource|clustername|namespace | ||
| String experimentName = KruizeDeploymentInfo.namespace_experiment_name_format | ||
| .replace("%datasource%", datasource) | ||
| .replace("%clustername%", clusterName) | ||
| .replace("%namespace%", namespaceName); |
Contributor
There was a problem hiding this comment.
issue (broader_impact): Namespace experiments use the datasource metadata cluster in their experiment name, while prepareNamespaceExperimentJSONInput uses the trimmed bulkInput.cluster_name in the experiment payload. With a cluster override, the created experiment name identifies one cluster but its cluster_name field identifies another, breaking the name/cluster identity invariant and causing inconsistent lookups or duplicate experiments.
Triggers: When a namespace bulk request supplies cluster_name that differs from the cluster name in datasource metadata.
Suggested fix: Resolve and trim the cluster name once, pass it to frameNamespaceExperimentName, and use that same value for both the experiment name and payload.
Signed-off-by: Saad Khan <saakhan@ibm.com>
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
5 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
Enable Bulk API jobs to create either container or namespace experiments through a validated experiment type selection.
New Features:
Enhancements:
Build:
Documentation: