Fixes #27062: Fix silent exception swallowing in FeedRepository and resource leaks in SchemaFieldExtractor#27063
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
Pull request overview
This PR addresses two backend code-quality issues in openmetadata-service to improve operational debuggability and prevent resource leaks during JSON schema loading and entity cleanup.
Changes:
- Wraps schema resource
InputStreamusage inSchemaFieldExtractorwith try-with-resources to ensure streams are closed. - Stops silently swallowing exceptions in
FeedRepository.deleteByAbout()by emitting a WARN log when thread deletion fails.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
openmetadata-service/src/main/java/org/openmetadata/service/util/SchemaFieldExtractor.java |
Ensures schema-loading InputStreams are closed via try-with-resources; refactors exception handling around schema load. |
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java |
Logs failures when deleting threads during entity cleanup instead of swallowing exceptions silently. |
Comments suppressed due to low confidence (2)
openmetadata-service/src/main/java/org/openmetadata/service/util/SchemaFieldExtractor.java:192
- In this catch-all branch, the log message drops the stack trace (only logs
e.getMessage()), and the newSchemaProcessingExceptionis created without the originalThrowableas its cause. This makes schema load failures much harder to debug. Consider logging the exception itself and using theSchemaProcessingException(String message, Throwable cause, ErrorType)constructor so the root cause is preserved.
} catch (Exception e) {
LOG.error("Error loading schema '{}': {}", schemaPath, e.getMessage());
throw new SchemaProcessingException(
"Error loading schema '" + schemaPath + "': " + e.getMessage(),
SchemaProcessingException.ErrorType.OTHER);
openmetadata-service/src/main/java/org/openmetadata/service/util/SchemaFieldExtractor.java:444
- Same as above: this logs only
e.getMessage()and wraps the error without setting the original exception as the cause. To retain actionable diagnostics, log the exception (stack trace) and passeinto theSchemaProcessingExceptionconstructor that accepts a cause.
} catch (Exception e) {
LOG.error("Error loading schema '{}': {}", schemaPath, e.getMessage());
throw new SchemaProcessingException(
"Error loading schema '" + schemaPath + "': " + e.getMessage(),
SchemaProcessingException.ErrorType.OTHER);
…in SchemaFieldExtractor - FeedRepository.deleteByAbout(): Replace empty catch block with LOG.warn to log failed thread deletions during entity cleanup with thread ID, entity ID, and exception details - SchemaFieldExtractor.loadMainSchema(): Wrap InputStream in try-with-resources to prevent file descriptor leak - SchemaFieldExtractor.loadSchema(): Same try-with-resources fix, consistent with getAllEntityTypes() which already does this correctly
d429cdd to
64624a0
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ ApprovedFixes silent exception swallowing in FeedRepository and resource leaks in SchemaFieldExtractor to improve error visibility and prevent resource exhaustion. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
Describe your changes:
Fixes #27062
Two related code quality fixes in
openmetadata-servicethat improve debuggability and resource management:Fix 1: FeedRepository.deleteByAbout() — silent exception swallowing
The
deleteByAbout()method had a completely empty catch block that silently swallowed all exceptions during thread deletion:The class already uses
@Slf4jwithLOGthroughout (6 existing usages), making this empty catch block a clear oversight. When thread deletion fails during entity cleanup, operators now get actionable diagnostic information instead of silent data orphaning.Fix 2: SchemaFieldExtractor.loadMainSchema() & loadSchema() — unclosed InputStreams
Both methods opened
InputStreamviagetClassLoader().getResourceAsStream()but never closed them, leaking file descriptors on each schema load. Fixed by wrapping with try-with-resources:Notably,
getAllEntityTypes()in the same file already uses try-with-resources correctly — this fix makes the file internally consistent.Testing: Both fixes are behavioral no-ops (logging-only change + resource lifecycle change) with no functional changes to control flow. Validated with
mvn spotless:check.Type of change:
Checklist:
Fixes <issue-number>: <short explanation>