Layers -05 Add DAO and Converter support for List Layers functionality#1757
Merged
dinogun merged 12 commits intokruize:mvp_demofrom Feb 5, 2026
Merged
Conversation
Contributor
Reviewer's GuideImplements backend support to list layers from the database by adding DAO queries, DB↔domain converters, a service method to load all layers into a map, and associated Prometheus timing metrics and validations. Sequence diagram for loadAllLayers service and DAO interactionsequenceDiagram
actor Caller
participant ExperimentDBService
participant ExperimentDAO as ExperimentDAO_ExperimentDAOImpl
participant HibernateSessionFactory as KruizeHibernateUtil
participant Session
participant Database as DB
participant DBHelpers as KruizeObjectConverters
participant MetricsConfig
Caller->>ExperimentDBService: loadAllLayers(layerMap)
alt layerMap is null
ExperimentDBService-->>Caller: return
else layerMap not null
ExperimentDBService->>ExperimentDAO: loadAllLayers()
activate ExperimentDAO
ExperimentDAO->>MetricsConfig: meterRegistry()
MetricsConfig-->>ExperimentDAO: MeterRegistry
ExperimentDAO->>MetricsConfig: start Timer.Sample for loadAllLayers
ExperimentDAO->>HibernateSessionFactory: getSessionFactory().openSession()
HibernateSessionFactory-->>ExperimentDAO: Session
activate Session
ExperimentDAO->>Session: createQuery(SELECT_FROM_LAYER)
Session->>DB: execute HQL query
DB-->>Session: List<KruizeLMLayerEntry>
Session-->>ExperimentDAO: entries
deactivate Session
ExperimentDAO->>MetricsConfig: register timerLoadAllLayers with status=success
ExperimentDAO->>MetricsConfig: stop Timer.Sample
ExperimentDAO-->>ExperimentDBService: List<KruizeLMLayerEntry>
deactivate ExperimentDAO
alt entries not null and not empty
ExperimentDBService->>DBHelpers: convertLayerEntryToLayerObject(entries)
activate DBHelpers
loop for each KruizeLMLayerEntry
DBHelpers->>DBHelpers: convertLayerDBObjToLayerObject(entry)
alt conversion fails
DBHelpers-->>ExperimentDBService: LayerConversionException
ExperimentDBService-->>Caller: propagate exception
else conversion ok
DBHelpers-->>ExperimentDBService: KruizeLayer
end
end
deactivate DBHelpers
ExperimentDBService->>ExperimentDBService: populate layerMap with layer.getLayerName()
end
ExperimentDBService-->>Caller: return
end
Sequence diagram for LayerPresenceQuery dataSource validationsequenceDiagram
actor Caller
participant LayerPresenceQuery
participant KruizeSupportedTypes
Caller->>LayerPresenceQuery: setDataSource(dataSource)
alt dataSource is not null and in MONITORING_AGENTS_SUPPORTED
LayerPresenceQuery->>KruizeSupportedTypes: MONITORING_AGENTS_SUPPORTED.contains(dataSource.toLowerCase())
KruizeSupportedTypes-->>LayerPresenceQuery: true
LayerPresenceQuery->>LayerPresenceQuery: this.dataSource = dataSource
else dataSource invalid or null
LayerPresenceQuery->>KruizeSupportedTypes: MONITORING_AGENTS_SUPPORTED.contains(dataSource.toLowerCase())
KruizeSupportedTypes-->>LayerPresenceQuery: false
LayerPresenceQuery->>LayerPresenceQuery: do not modify dataSource
end
LayerPresenceQuery-->>Caller: return
Class diagram for new layer loading and conversion flowclassDiagram
class ExperimentDBService {
+void loadAllLayers(Map~String,KruizeLayer~ layerMap) throws Exception
+ValidationOutputData addLayerToDB(KruizeLayer kruizeLayer)
-ExperimentDAO experimentDAO
}
class ExperimentDAO {
<<interface>>
+ValidationOutputData addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry)
+List~KruizeLMLayerEntry~ loadAllLayers() throws Exception
+List~KruizeLMLayerEntry~ loadLayerByName(String layerName) throws Exception
}
class ExperimentDAOImpl {
+List~KruizeLMLayerEntry~ loadAllLayers() throws Exception
+List~KruizeLMLayerEntry~ loadLayerByName(String layerName) throws Exception
+List~KruizeLMMetadataProfileEntry~ loadAllMetadataProfiles() throws Exception
-Logger LOGGER
}
class DBHelpers {
}
class Converters {
}
class KruizeObjectConverters {
+KruizeLMLayerEntry convertLayerObjectToLayerDBObj(KruizeLayer kruizeLayer) throws Exception
+KruizeLayer convertLayerDBObjToLayerObject(KruizeLMLayerEntry entry) throws LayerConversionException
+List~KruizeLayer~ convertLayerEntryToLayerObject(List~KruizeLMLayerEntry~ kruizeLayerEntryList) throws LayerConversionException
}
class KruizeLMLayerEntry {
+String getApi_version()
+String getKind()
+String getLayer_name()
+Integer getLayer_level()
+Object getDetails()
+Object getMetadata()
+Object getLayer_presence()
+Object getTunables()
}
class KruizeLayer {
+void setApiVersion(String apiVersion)
+void setKind(String kind)
+void setLayerName(String layerName)
+void setLayerLevel(Integer layerLevel)
+void setDetails(Object details)
+void setMetadata(LayerMetadata metadata)
+void setLayerPresence(LayerPresence layerPresence)
+void setTunables(List~Tunable~ tunables)
+String getLayerName()
}
class LayerMetadata {
}
class LayerPresence {
}
class Tunable {
}
class LayerConversionException {
+LayerConversionException(AnalyzerConstants.LayerConversionSection section, String message, Throwable cause)
+AnalyzerConstants.LayerConversionSection getSection()
-AnalyzerConstants.LayerConversionSection section
}
class AnalyzerConstants {
}
class LayerConversionSection {
<<enum>>
BASIC_FIELDS
METADATA
LAYER_PRESENCE
TUNABLES
}
class MetricsConfig {
+static Timer timerAddLayerDB
+static Timer timerLoadAllLayers
+static Timer timerLoadLayerByName
+static Timer.Builder timerBAddLayerDB
+static Timer.Builder timerBLoadAllLayers
+static Timer.Builder timerBLoadLayerByName
+static MeterRegistry meterRegistry()
}
class LayerPresenceQuery {
-String dataSource
+String getDataSource()
+void setDataSource(String dataSource)
}
class KruizeSupportedTypes {
+static Set~String~ MONITORING_AGENTS_SUPPORTED
}
ExperimentDBService --> ExperimentDAO : uses
ExperimentDAOImpl ..|> ExperimentDAO
ExperimentDBService --> KruizeObjectConverters : uses
ExperimentDAOImpl --> KruizeLMLayerEntry : queries
ExperimentDAOImpl --> DBConstants : uses HQL constants
ExperimentDAOImpl --> MetricsConfig : records timers
KruizeObjectConverters --> KruizeLMLayerEntry : converts from
KruizeObjectConverters --> KruizeLayer : converts to
KruizeObjectConverters --> LayerConversionException : throws
KruizeObjectConverters --> LayerMetadata
KruizeObjectConverters --> LayerPresence
KruizeObjectConverters --> Tunable
LayerConversionException --> AnalyzerConstants : uses
AnalyzerConstants --> LayerConversionSection
LayerPresenceQuery --> KruizeSupportedTypes : validates dataSource
DBHelpers *-- Converters
Converters *-- KruizeObjectConverters
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 layer conversion helpers in DBHelpers mix ObjectMapper and Gson, throw/catch generic Exceptions, and call printStackTrace; consider standardizing on a single JSON library and surfacing more specific checked/unchecked exceptions instead of swallowing details at this low level.
- The DAO methods for layers (addLayerToDB/loadAllLayers/loadLayerByName) re-register timers on every call using the builder; this can lead to metric registration churn and tag-cardinality issues—prefer reusing pre-registered Timer instances and only tagging via Sample where needed.
- ExperimentDBService.addLayerToDB logs exceptions but always returns a ValidationOutputData object whose message is often null; consider setting an explicit error message or propagating the exception so callers can distinguish failure reasons more reliably.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The layer conversion helpers in DBHelpers mix ObjectMapper and Gson, throw/catch generic Exceptions, and call printStackTrace; consider standardizing on a single JSON library and surfacing more specific checked/unchecked exceptions instead of swallowing details at this low level.
- The DAO methods for layers (addLayerToDB/loadAllLayers/loadLayerByName) re-register timers on every call using the builder; this can lead to metric registration churn and tag-cardinality issues—prefer reusing pre-registered Timer instances and only tagging via Sample where needed.
- ExperimentDBService.addLayerToDB logs exceptions but always returns a ValidationOutputData object whose message is often null; consider setting an explicit error message or propagating the exception so callers can distinguish failure reasons more reliably.
## Individual Comments
### Comment 1
<location> `src/main/java/com/autotune/database/service/ExperimentDBService.java:473-474` </location>
<code_context>
+ Timer.Sample timerAddLayerDB = Timer.start(MetricsConfig.meterRegistry());
+ Transaction tx = null;
+ try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {
+ try {
+ tx = session.beginTransaction();
+ session.persist(kruizeLayerEntry);
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle possible null return from convertLayerObjectToLayerDBObj before calling DAO
convertLayerObjectToLayerDBObj may return null on conversion failure, but its result is used here without a check. If null is passed to experimentDAO.addLayerToDB, this could trigger a runtime exception (e.g., from Hibernate). Add a null guard and, on failure, return a failed ValidationOutputData with a clear error message instead of calling the DAO.
</issue_to_address>
### Comment 2
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1611-1612` </location>
<code_context>
+ validationOutputData.setSuccess(false);
+ validationOutputData.setMessage(e.getMessage());
+ }
+ } catch (Exception e) {
+ LOGGER.error("Not able to save layer due to {}", e.getMessage());
+ validationOutputData.setMessage(e.getMessage());
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid swallowing conversion exceptions and only logging the message
By catching `Exception` and only logging `e.getMessage()`, callers lose the original exception type, stack trace, and context, and the method may proceed with invalid or partially initialized data. Prefer either letting the specific exception propagate or wrapping it in a domain-specific exception while preserving the cause, and log with `LOGGER.error(..., e)` instead of using `printStackTrace()` or message-only logging.
</issue_to_address>
### Comment 3
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1683-1692` </location>
<code_context>
+ public static List<KruizeLayer> convertLayerEntryToLayerObject(List<KruizeLMLayerEntry> kruizeLayerEntryList) throws Exception {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Preserve original exception cause instead of throwing a new generic Exception
In the outer catch, the original exception is logged but then replaced with `new Exception(e.getMessage())`, which discards its type and stack trace. Instead, either rethrow the original exception or wrap it with `throw new Exception("...", e);` so callers retain access to the root cause.
Suggested implementation:
```java
throw new Exception("Error while converting layer entries to layer objects", e);
```
If you prefer to keep the original exception type entirely, you can instead replace the line with `throw e;` so callers get the exact original exception without wrapping.
</issue_to_address>
### Comment 4
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/presence/LayerPresenceQuery.java:39-48` </location>
<code_context>
+ public LayerPresenceQuery() {
+ }
+
+ public LayerPresenceQuery(String datasource,
+ String layerPresenceQuery,
+ String layerPresenceKey) throws MonitoringAgentNotSupportedException {
+
+ if (KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED.contains(datasource)) {
+ this.dataSource = datasource;
+ } else {
+ throw new MonitoringAgentNotSupportedException(
+ "Unsupported monitoring agent datasource: '" + datasource + "'. " +
+ "Supported datasources: " + KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED
+ );
+ }
+ this.layerPresenceQuery = layerPresenceQuery;
+ this.layerPresenceKey = layerPresenceKey;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Constructor validates datasource, but setters allow bypassing that validation
The arg-based constructor validates `datasource` against `KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED`, but other code paths (e.g., default constructor + setter / deserialization) can assign an unchecked value. To preserve the constraint that only supported monitoring agents are used, either apply the same validation in the setter or make `dataSource` immutable and settable only via the validating constructor.
</issue_to_address>
### Comment 5
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/LayerPresence.java:46-55` </location>
<code_context>
+ * @return LayerPresenceDetector implementation
+ */
+ public LayerPresenceDetector getDetector() {
+ // Check for query-based presence
+ if (queries != null && !queries.isEmpty()) {
+ return new QueryBasedPresence(queries);
+ }
+
+ // Check for label-based presence
+ if (label != null && !label.isEmpty()) {
+ return new LabelBasedPresence(label);
+ }
+
+ // Default to presence always
+ if (presence != null) {
+ return new PresenceAlways(presence);
+ }
+
+ // If nothing is set, default to "always"
+ return new PresenceAlways("always");
+ }
</code_context>
<issue_to_address>
**suggestion:** Clarify or enforce precedence rules between presence, queries, and labels
This method prioritizes queries over labels and only falls back to `presence` (or "always") when both are unset. If a config sets queries together with labels or presence, the extra settings are silently ignored, which may confuse users. Consider either enforcing that only one mechanism is configured, or at least logging a warning when multiple are set but only one is used.
Suggested implementation:
```java
/**
* Factory method to get the appropriate LayerPresenceDetector implementation
* based on what fields are populated.
* <p>
* Precedence rules when multiple mechanisms are configured:
* queries > label > presence
*
* @return LayerPresenceDetector implementation
*/
public LayerPresenceDetector getDetector() {
boolean hasQueries = (queries != null && !queries.isEmpty());
boolean hasLabel = (label != null && !label.isEmpty());
boolean hasPresence = (presence != null);
int configuredMechanisms = 0;
if (hasQueries) {
configuredMechanisms++;
}
if (hasLabel) {
configuredMechanisms++;
}
if (hasPresence) {
configuredMechanisms++;
}
if (configuredMechanisms > 1) {
// Multiple presence mechanisms configured; warn and rely on documented precedence.
LOGGER.warn(
"Multiple presence mechanisms configured for LayerPresence (queries: {}, label: {}, presence: {}). " +
"Using precedence: queries > label > presence.",
hasQueries, hasLabel, hasPresence
);
}
// Check for query-based presence (highest precedence)
if (hasQueries) {
return new QueryBasedPresence(queries);
}
// Check for label-based presence
if (hasLabel) {
return new LabelBasedPresence(label);
}
// Default to presence always
if (hasPresence) {
return new PresenceAlways(presence);
}
// If nothing is set, default to "always"
return new PresenceAlways("always");
```
1. Ensure a logger is available in this class. If it is not already defined, add near the top of `LayerPresence`:
- Import:
`import org.slf4j.Logger;`
`import org.slf4j.LoggerFactory;`
- Field:
`private static final Logger LOGGER = LoggerFactory.getLogger(LayerPresence.class);`
2. If the project uses a different logging convention (e.g., `Log`, `Logger LOG`, or a custom logging wrapper), adjust the `LOGGER.warn(...)` usage to match the existing pattern.
3. If there are unit tests around `LayerPresence.getDetector()`, consider adding tests that:
- Configure only `queries`, only `label`, only `presence`, and combinations thereof.
- Assert that the correct detector type is chosen in each case.
- Optionally verify that a warning is logged when multiple mechanisms are configured.
</issue_to_address>
### Comment 6
<location> `src/main/java/com/autotune/analyzer/utils/AnalyzerConstants.java:645-635` </location>
<code_context>
+ /**
+ * Enum for different types of layer presence detection
+ */
+ public enum PresenceType {
+ ALWAYS,
+ QUERY,
+ LABEL
+ }
+
+ public static final String DEFAULT_PRESENCE = "always";
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating PresenceType enum that already exists in LayerPresenceDetector
This enum duplicates `LayerPresenceDetector.PresenceType`, which risks them diverging over time. Prefer reusing `LayerPresenceDetector.PresenceType`, or refactor so there is a single source of truth that the other references or derives from.
Suggested implementation:
```java
/**
* Contains constants related to KruizeLayer presence detection
*/
public static final class LayerConstants {
// Default layer presence type, derived from LayerPresenceDetector.PresenceType
public static final String DEFAULT_PRESENCE =
LayerPresenceDetector.PresenceType.ALWAYS.name().toLowerCase();
private LayerConstants() {
}
}
```
1. Add an import for `LayerPresenceDetector` at the top of `AnalyzerConstants.java`, using its correct package (for example: `import com.autotune.analyzer.layer.LayerPresenceDetector;`), matching the actual location in your codebase.
2. If your codebase standardizes lowercase constants via a specific utility or uses `Locale.ROOT` with `toLowerCase`, you may want to adjust `toLowerCase()` accordingly:
`LayerPresenceDetector.PresenceType.ALWAYS.name().toLowerCase(Locale.ROOT);` and import `java.util.Locale`.
3. Update any usages of `AnalyzerConstants.LayerConstants.PresenceType` (if any were added elsewhere in the PR) to use `LayerPresenceDetector.PresenceType` directly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/com/autotune/analyzer/kruizeLayer/presence/LayerPresenceQuery.java
Show resolved
Hide resolved
bhanvimenghani
force-pushed
the
layer-dao-list-implementation
branch
2 times, most recently
from
ccc24a1 to
2b88bb8
Compare
Contributor
|
@bhanvimenghani Pls rebase to resolve conflicts |
bharathappali
force-pushed
the
layer-dao-list-implementation
branch
from
2b88bb8 to
af54139
Compare
Member
|
@dinogun I have rebased on behalf of @bhanvimenghani |
bharathappali
requested changes
Jan 30, 2026
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Member
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
LayerPresenceQuery.setDataSource, callingdataSource.toLowerCase()without a null check can throw an NPE and silently ignoring unsupported values (leaving the previous value unchanged) may be surprising to callers; consider validating/null-checking and failing fast or logging when an unsupported value is provided. - The new DB timer usage in
ExperimentDAOImpl.loadAllLayersandloadLayerByNamere-registers the timers on every call and overwrites the static timer fields; it would be cleaner to register the timers once inMetricsConfigand just start/stop them in the DAO methods without reassigning the static variables. - In
ExperimentDAOImpland the new converters, you wrap and rethrow exceptions with only the message, and inconvertLayerDBObjToLayerObjectyou log the message and callprintStackTrace()while swallowing failures and returning null; consider consistently propagating the original cause (new Exception(msg, e)) and avoidingprintStackTrace()/silent nulls so callers can handle conversion/DB errors more predictably.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `LayerPresenceQuery.setDataSource`, calling `dataSource.toLowerCase()` without a null check can throw an NPE and silently ignoring unsupported values (leaving the previous value unchanged) may be surprising to callers; consider validating/null-checking and failing fast or logging when an unsupported value is provided.
- The new DB timer usage in `ExperimentDAOImpl.loadAllLayers` and `loadLayerByName` re-registers the timers on every call and overwrites the static timer fields; it would be cleaner to register the timers once in `MetricsConfig` and just start/stop them in the DAO methods without reassigning the static variables.
- In `ExperimentDAOImpl` and the new converters, you wrap and rethrow exceptions with only the message, and in `convertLayerDBObjToLayerObject` you log the message and call `printStackTrace()` while swallowing failures and returning null; consider consistently propagating the original cause (`new Exception(msg, e)`) and avoiding `printStackTrace()`/silent nulls so callers can handle conversion/DB errors more predictably.
## Individual Comments
### Comment 1
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/LayerPresenceQuery.java:60-61` </location>
<code_context>
public void setDataSource(String dataSource) {
- this.dataSource = dataSource;
+ if (KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED.contains(dataSource.toLowerCase()))
+ this.dataSource = dataSource;
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against null and consider behavior when dataSource is unsupported
`dataSource.toLowerCase()` will throw an NPE if `dataSource` is null. Also, unsupported values are silently ignored, leaving the previous `this.dataSource` and making behavior harder to trace. Consider adding a null check and explicitly handling unsupported values (e.g., early return with a log or exception).
</issue_to_address>
### Comment 2
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1619-1628` </location>
<code_context>
+ public static KruizeLayer convertLayerDBObjToLayerObject(KruizeLMLayerEntry entry) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Exception handling here both swallows the error and uses e.printStackTrace
This method logs and returns `null` on any exception, so callers can’t distinguish a real `null` from a conversion failure. It also uses `e.printStackTrace()`, which isn’t appropriate for production. Prefer propagating the exception or using a result type (e.g., `Optional<KruizeLayer>`) that makes failures explicit, and rely on structured logging instead of `printStackTrace()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/com/autotune/analyzer/kruizeLayer/LayerPresenceQuery.java
Outdated
Show resolved
Hide resolved
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Member
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the layer conversion helpers,
convertLayerEntryToLayerObjectcurrently declares and throws a genericExceptionand then wrapsLayerConversionExceptioninto a plainException, which discards the structured section information and stack trace; consider narrowing the method signature toLayerConversionException(or including it) and rethrowing the original cause to preserve context. - In the new DAO methods (
loadAllLayers,loadLayerByName), you catch a genericExceptionand wrap it in a newExceptionwith only the message, which loses the original cause and stack trace; it would be better to chain the cause (e.g.throw new Exception("...", e)) or introduce a dedicated data-access exception type. - The metrics wiring in
loadAllLayersandloadLayerByNamere-registersTimerinstances on every call viatimerBLoadAllLayers.tag(...).register(...), which can create many time series and be expensive; consider registering the timers once (e.g. statically or during initialization) and only tagging/stopping samples per call.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the layer conversion helpers, `convertLayerEntryToLayerObject` currently declares and throws a generic `Exception` and then wraps `LayerConversionException` into a plain `Exception`, which discards the structured section information and stack trace; consider narrowing the method signature to `LayerConversionException` (or including it) and rethrowing the original cause to preserve context.
- In the new DAO methods (`loadAllLayers`, `loadLayerByName`), you catch a generic `Exception` and wrap it in a new `Exception` with only the message, which loses the original cause and stack trace; it would be better to chain the cause (e.g. `throw new Exception("...", e)`) or introduce a dedicated data-access exception type.
- The metrics wiring in `loadAllLayers` and `loadLayerByName` re-registers `Timer` instances on every call via `timerBLoadAllLayers.tag(...).register(...)`, which can create many time series and be expensive; consider registering the timers once (e.g. statically or during initialization) and only tagging/stopping samples per call.
## Individual Comments
### Comment 1
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1693-1702` </location>
<code_context>
+ public static List<KruizeLayer> convertLayerEntryToLayerObject(List<KruizeLMLayerEntry> kruizeLayerEntryList) throws Exception {
</code_context>
<issue_to_address>
**suggestion:** Preserve LayerConversionException details and avoid re-wrapping generic Exceptions in the conversion loop.
The method signature and handling are overly generic: it declares `throws Exception` and rewraps all errors (including `LayerConversionException`) into a new `Exception` while logging only `e.getMessage()`, which discards structured section data and the original stack trace. Narrow the catch to `LayerConversionException` (plus an optional catch for unexpected throwables), log the full exception, and either rethrow the original or wrap it as the cause (e.g., `new Exception("...", e)`). Update the method signature to `throws LayerConversionException` to better reflect the expected failure mode.
Suggested implementation:
```java
* @param kruizeLayerEntryList List of KruizeLMLayerEntry database objects to be converted
* @return List of KruizeLayer domain objects
* @throws LayerConversionException if any KruizeLMLayerEntry cannot be converted to a KruizeLayer
*/
public static List<KruizeLayer> convertLayerEntryToLayerObject(List<KruizeLMLayerEntry> kruizeLayerEntryList) throws LayerConversionException {
```
Because the snippet does not show the full method body, you’ll need to adjust the exception handling inside the conversion loop. Concretely:
1. Replace the generic catch with a typed catch for `LayerConversionException` and an optional fallback catch for unexpected exceptions. For example, inside the `for (KruizeLMLayerEntry entry : kruizeLayerEntryList) { ... }` loop:
```java
try {
if (null == entry)
continue;
KruizeLayer kruizeLayer = convertLayerDBObjToLayerObject(entry);
kruizeLayerList.add(kruizeLayer);
} catch (LayerConversionException e) {
// Preserve full context and stack trace
LOGGER.error("Layer conversion failed for entry: {}", entry, e);
throw e;
} catch (Exception e) {
// Handle unexpected issues but preserve as cause
LOGGER.error("Unexpected error during layer conversion for entry: {}", entry, e);
throw new LayerConversionException("Unexpected error during layer conversion", e);
}
```
2. If the file does not already import `LayerConversionException`, add at the top of `DBHelpers.java`:
```java
import com.autotune.common.exceptions.LayerConversionException;
```
3. Ensure that any other callers of `convertLayerEntryToLayerObject(...)` are updated to handle `LayerConversionException` (either by declaring it in their own `throws` clause or by catching it).
</issue_to_address>
### Comment 2
<location> `src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java:1452-1455` </location>
<code_context>
+ try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {
+ entries = session.createQuery(DBConstants.SQLQUERY.SELECT_FROM_LAYER, KruizeLMLayerEntry.class).list();
+ statusValue = "success";
+ } catch (Exception e) {
+ LOGGER.error("Not able to load Layers due to {}", e.getMessage());
+ throw new Exception("Error while loading existing Layers from database due to : " + e.getMessage());
+ } finally {
+ if (null != timerLoadAllLayers) {
</code_context>
<issue_to_address>
**suggestion:** Retain original exception as cause and log full stack trace when loadAllLayers fails.
The catch block wraps the original `Exception` using only `e.getMessage()`, which loses the stack trace and concrete exception type, and logs only the message. Use `LOGGER.error("Not able to load Layers", e);` and preserve the cause when rethrowing, e.g. `throw new Exception("Error while loading existing Layers from database", e);` to keep full context for debugging.
```suggestion
} catch (Exception e) {
LOGGER.error("Not able to load Layers", e);
throw new Exception("Error while loading existing Layers from database", e);
} finally {
```
</issue_to_address>
### Comment 3
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/LayerPresenceQuery.java:59-63` </location>
<code_context>
public void setDataSource(String dataSource) {
- this.dataSource = dataSource;
+ if (null != dataSource &&
+ KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED.contains(dataSource.toLowerCase()))
+ this.dataSource = dataSource;
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Setter silently ignores unsupported dataSource values, which may hide configuration errors.
Because the setter only updates `this.dataSource` when the value is non-null and in `MONITORING_AGENTS_SUPPORTED`, calls with unsupported values become silent no-ops and leave the previous value (or null) in place. This can mislead callers into believing the data source was changed. Consider either resetting `this.dataSource` to null for unsupported values, or throwing an `IllegalArgumentException` to fail fast on invalid input.
```suggestion
public void setDataSource(String dataSource) {
if (dataSource == null) {
this.dataSource = null;
return;
}
if (!KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED.contains(dataSource.toLowerCase())) {
throw new IllegalArgumentException(
"Unsupported dataSource: " + dataSource + ". Supported values are: "
+ KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED);
}
this.dataSource = dataSource;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Member
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The DAO method
loadLayerByNamereturns aList<KruizeLMLayerEntry>even though it is logically expected to return at most a single record; consider tightening this to return anOptional<KruizeLMLayerEntry>(or throw if multiple found) so callers don't have to handle an unexpected collection. - In
ExperimentDAOImpl.loadAllLayersandloadLayerByName, timers are re-registered on each call (MetricsConfig.timerBLoadAllLayers.tag(...).register(...)), which can create many distinct meters; it would be cleaner to register the timers once and only useTimer.SamplewithTimerinstances at call time. - In
ExperimentDBService.loadAllLayers, the method silently returns whenlayerMapis null; consider either enforcing non-null via a precondition (e.g.,Objects.requireNonNull) or documenting and avoiding this silent no-op to make incorrect usage fail fast.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The DAO method `loadLayerByName` returns a `List<KruizeLMLayerEntry>` even though it is logically expected to return at most a single record; consider tightening this to return an `Optional<KruizeLMLayerEntry>` (or throw if multiple found) so callers don't have to handle an unexpected collection.
- In `ExperimentDAOImpl.loadAllLayers` and `loadLayerByName`, timers are re-registered on each call (`MetricsConfig.timerBLoadAllLayers.tag(...).register(...)`), which can create many distinct meters; it would be cleaner to register the timers once and only use `Timer.Sample` with `Timer` instances at call time.
- In `ExperimentDBService.loadAllLayers`, the method silently returns when `layerMap` is null; consider either enforcing non-null via a precondition (e.g., `Objects.requireNonNull`) or documenting and avoiding this silent no-op to make incorrect usage fail fast.
## Individual Comments
### Comment 1
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1622` </location>
<code_context>
+ */
+ public static KruizeLayer convertLayerDBObjToLayerObject(KruizeLMLayerEntry entry) throws LayerConversionException {
+ KruizeLayer kruizeLayer = new KruizeLayer();
+ Gson gson = new Gson();
+
+ try {
</code_context>
<issue_to_address>
**suggestion (performance):** Consider reusing a single Gson instance instead of creating a new one per conversion for efficiency.
`convertLayerDBObjToLayerObject` creates a new `Gson` each time, which is unnecessary overhead when loading many layers. Since `Gson` is thread-safe after configuration, consider a static/shared instance in this helper to reduce allocations and GC pressure in bulk operations (e.g. `loadAllLayers`).
Suggested implementation:
```java
public static KruizeLayer convertLayerDBObjToLayerObject(KruizeLMLayerEntry entry) throws LayerConversionException {
KruizeLayer kruizeLayer = new KruizeLayer();
Gson gson = GSON;
```
To fully implement the optimization, also:
1. Add a shared `Gson` instance to the `DBHelpers` class, e.g.:
- `private static final Gson GSON = new Gson();`
Place this near other static fields in `DBHelpers`.
2. Ensure `com.google.gson.Gson` is imported at the top of the file if it is not already.
This will allow all conversions (including `convertLayerDBObjToLayerObject`) to reuse the same thread-safe `Gson` instance.
</issue_to_address>
### Comment 2
<location> `src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java:1452-1461` </location>
<code_context>
+ entries = session.createQuery(DBConstants.SQLQUERY.SELECT_FROM_LAYER_BY_NAME, KruizeLMLayerEntry.class)
+ .setParameter("layerName", layerName).list();
+ statusValue = "success";
+ } catch (Exception e) {
+ LOGGER.error("Not able to load Layer {} due to {}", layerName, e.getMessage());
+ throw new Exception("Error while loading Layer from database due to : " + e.getMessage());
+ } finally {
+ if (null != timerLoadLayerByName) {
</code_context>
<issue_to_address>
**issue:** Preserve the original exception as the cause when rethrowing to avoid losing stack trace and context.
In `loadLayerByName`, the rethrown `Exception` currently uses only `e.getMessage()` and drops the original cause, while `loadAllLayers` correctly chains the exception with `throw new Exception("Error while loading existing Layers from database", e);`. For consistency and better diagnosability, apply the same pattern here:
```java
throw new Exception("Error while loading Layer from database", e);
```
</issue_to_address>
### Comment 3
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/LayerPresenceQuery.java:60-61` </location>
<code_context>
public void setDataSource(String dataSource) {
- this.dataSource = dataSource;
+ if (null != dataSource &&
+ KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED.contains(dataSource.toLowerCase()))
+ this.dataSource = dataSource;
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silently ignoring unsupported dataSource values may make misconfigurations harder to detect.
The setter now only updates `this.dataSource` when the input is in `MONITORING_AGENTS_SUPPORTED`; otherwise it does nothing. This diverges from the previous behavior and can make invalid inputs appear to succeed. Please either fail fast (e.g., throw), return a status, or at least log unsupported values so configuration issues are detectable.
Suggested implementation:
```java
public void setDataSource(String dataSource) {
if (dataSource == null) {
LOGGER.warn("LayerPresenceQuery: dataSource is null; resetting to null");
this.dataSource = null;
return;
}
String normalizedDataSource = dataSource.toLowerCase();
if (KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED.contains(normalizedDataSource)) {
this.dataSource = dataSource;
} else {
LOGGER.warn(
"LayerPresenceQuery: Unsupported dataSource '{}'. Supported values are: {}",
dataSource,
KruizeSupportedTypes.MONITORING_AGENTS_SUPPORTED
);
}
}
```
To make the above compile and log correctly, the following are also needed in `LayerPresenceQuery.java`:
1. Ensure a logger is defined in the class, e.g., using SLF4J:
- At the top of the file (with other imports), add:
`import org.slf4j.Logger;`
`import org.slf4j.LoggerFactory;`
- Inside the class, add:
`private static final Logger LOGGER = LoggerFactory.getLogger(LayerPresenceQuery.class);`
2. If a different logging framework is used in this project (e.g., `java.util.logging` or a custom logger), adjust the logger type, factory method, and `warn` calls to match the existing conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
@dinogun Please kindly review this PR |
dinogun
reviewed
Feb 3, 2026
dinogun
reviewed
Feb 3, 2026
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Member
|
@dinogun I have addressed your review comments and have also raised an issue for optimising the |
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
3 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
Implements backend data layer for listing layers from database
Changes
loadAllLayers()andloadLayerByName()methods with transaction handling and metricsKruizeLMLayerEntry(DB) ↔KruizeLayer(domain)loadAllLayers()to orchestrate DB access and conversionFiles Modified
ExperimentDAO.java- Interface definitionsExperimentDAOImpl.java- DAO implementation with Hibernate queriesDBHelpers.java- Data conversion logic (DB ↔ Domain)ExperimentDBService.java- Service layer orchestrationDBConstants.java- HQL query constantsMetricsConfig.java- Performance metrics configurationFixes # (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
Add support for loading and converting layer entities between the database and domain model, with metrics instrumentation for the new DAO operations.
New Features:
Enhancements: