fix: return Future.failed() instead of throw in DataNode recoverWith blocks#1180
Open
pallakartheekreddy wants to merge 13 commits intocloud-store-refactoringfrom
Open
fix: return Future.failed() instead of throw in DataNode recoverWith blocks#1180pallakartheekreddy wants to merge 13 commits intocloud-store-refactoringfrom
pallakartheekreddy wants to merge 13 commits intocloud-store-refactoringfrom
Conversation
…blocks
In all recoverWith { case e: CompletionException => throw e.getCause }
blocks, throwing inside recoverWith breaks the Future monad: the thrown
exception escapes the Future, making it uncatchable by downstream
Future combinators (.map, .flatMap, .recover, etc.).
The correct pattern is to return Future.failed(e.getCause), which keeps
the failure inside the Future chain so callers can handle it via
.recoverWith or .failed.foreach as expected.
Affects create(), update(), read(), search(), and enrichHierarchyAndUpdate()
in DataNode — 5 sites updated.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SonarCloud Analysis Results 🔍Quality Gate Results for Services:Please review the analysis results for each service. Ensure all quality gates are passing before merging. |
- QuestionActor.scala: remove redundant 'import scala.collection.JavaConverters'
(without wildcard) which is entirely subsumed by the subsequent
'import scala.collection.JavaConverters._'
- QuestionSetActor.scala: same JavaConverters duplicate as QuestionActor
- LockActor.scala: remove 'import scala.collection.immutable.{List, Map}'
— both types are already in scope via scala.Predef and importing them
explicitly creates a confusing apparent shadow of the builtins
Co-authored-by: Claude <claude@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
new FileOutputStream(csvFile) was passed directly into the OutputStreamWriter constructor. If the OutputStreamWriter constructor threw (e.g. charset encoding error), the FileOutputStream was never assigned to a variable reachable by the finally block, so it leaked. Fix: assign the FileOutputStream to a dedicated variable (fos) first, then pass it to OutputStreamWriter. The finally block now closes fos explicitly, ensuring the file handle is released even when the higher-level writer was never successfully constructed. Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace inline magic strings and numbers with named constants to improve
readability and make future changes easier:
- ChannelConstants: add COMPOSITE_SEARCH_URL_CONFIG_KEY and
COMPOSITE_SEARCH_URL_DEFAULT; ChannelManager uses them in all 4 call
sites instead of repeating the raw string literals
- DIALConstants: add DEFAULT_ERROR_CORRECTION_LEVEL ("H"),
DEFAULT_PIXELS_PER_BLOCK (2), DEFAULT_QR_CODE_MARGIN (3); DIALManager
references them instead of inline literals in defaultConfig map
- ContentConstants: add MAX_FILE_PATH_SIZE (100); ContentActor uses it
for the pre-signed URL file-path length guard and the error message
Co-authored-by: Claude <claude@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
… leak (#1185) KafkaProducer and KafkaConsumer were never closed, causing open connections and thread leaks when services shut down. This adds: - close(): flushes producer, then closes both producer and consumer with individual try/catch so a failure in one doesn't skip closing the other - registerShutdownHook(): registers a JVM shutdown hook to call close() automatically on normal or signal-based process exit Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…dexed access (#1184) LinkedList provides O(n) get(i) which degrades performance when fields and sortOrder are accessed by index during query construction. ArrayList provides O(1) random access with no change to the List<T> interface. All three instantiation sites updated, and the now-unused LinkedList import removed. Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
The directory and package declaration contained a typo ('mangers' instead
of 'managers'). This caused the file to sit in the wrong package path,
making it inconsistent with the surrounding codebase.
Changes:
- Moved FrameworkManager.scala from sunbird/mangers/ to sunbird/managers/
- Updated package declaration in FrameworkManager.scala
- Updated import in FrameworkActor.scala
- Updated import in CategoryActor.scala
- Updated import in FrameworkManagerTest.scala
Co-authored-by: Claude <claude@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ook (#1188) Two resource management issues fixed: 1. returnConnection() used jedisPool.returnResource() which has been deprecated since Jedis 3.0. Replaced with jedis.close() which correctly returns healthy connections to the pool and closes broken ones. 2. JedisPool was never closed, leaving open connections and background threads after process exit. Added closePool() and a JVM shutdown hook to ensure the pool is always properly released on shutdown. Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
#1191) * refactor: replace println/System.out.println with TelemetryManager logging Replace all raw println and System.out.println calls in business logic with structured TelemetryManager calls (debug/error/warn) so that log output is captured by the platform's logging pipeline and not silently dropped in production. Files changed: - ContentActor.scala: warn for missing default licence - ChannelManager.scala: error for category fetch failure (added import) - NodeValidator.scala: debug for singular identifier lookup (added import) - DataSubGraph.scala: debug for subgraph traversal diagnostics (added import) - StorageService.scala: error for getUri exception (added import) - CollectionTOCUtil.scala: error for validateDIALCodes exception - ObjectCategoryDefinitionActor.scala: debug for fallback _all lookup (added import) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Apply suggestion from @maheshkumargangula Co-authored-by: Mahesh Kumar Gangula <6985261+maheshkumargangula@users.noreply.github.com> * Apply suggestion from @maheshkumargangula Co-authored-by: Mahesh Kumar Gangula <6985261+maheshkumargangula@users.noreply.github.com> --------- Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Mahesh Kumar Gangula <6985261+maheshkumargangula@users.noreply.github.com>
… in CassandraConnector (#1183) Two issues fixed: 1. prepareSession() called registerShutdownHook() on every invocation. If getSession() triggered re-preparation (stale/closed session), multiple hooks were registered for the same operation. Added a shutdownHookRegistered boolean guard to ensure exactly one hook is registered for the lifetime of the process. 2. e.printStackTrace() in the catch block sent stack traces to stderr without context. Replaced with TelemetryManager.error() for consistent, structured logging. 3. close() now iterates sessions individually with per-session error handling so a failure on one session does not abort closing others, and clears the map after shutdown. Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ex to TaxonomyUtil (#1193) TermActor and CategoryInstanceActor each contained identical private methods: - generateIdentifier(scopeId, code): slugifies scopeId_code - getIndex / getCategoryIndex: computes next SEQUENCE_MEMBERSHIP index from a node Extracted both to a new TaxonomyUtil object in the utils/taxonomy package. Updated all call sites in both actors to use TaxonomyUtil.generateIdentifier() and TaxonomyUtil.getNextSequenceIndex(). Removed the now-redundant private method definitions and the no-longer-needed Slug / RelationTypes imports. Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* refactor: replace .flatMap(f => f) anti-pattern with .flatten .flatMap(f => f) is a verbose identity flatMap — the idiomatic Scala equivalent is simply .flatten. Applied mechanically across 31 files (115 occurrences) to improve readability with no behaviour change. Affected modules: taxonomy-actors, FrameworkManager, FrameworkActor, CategoryInstanceActor, ObjectCategoryDefinitionActor, ContentActor, HierarchyManager, UpdateHierarchyManager, DataNode, DefinitionNode, RelationValidator, VersioningNode, VersionKeyValidator, FrameworkValidator, CopyManager, FlagManager, AcceptFlagManager, AssetCopyManager, DiscardManager, DIALManager, PublishManager, ReviewManager, UploadManager, ItemSetActor, QuestionActor, QuestionSetActor, assessment CopyManager, parseq Task, CollectionMimeTypeMgrImpl. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: replace non-existent TelemetryManager.debug with TelemetryManager.log to resolve build errors * Replace JavaConverters with jdk.CollectionConverters Switch scala.collection.JavaConverters to scala.jdk.CollectionConverters and simplify conversions by replacing JavaConverters.seqAsJavaListConverter(...).asJava with .toList.asJava in QuestionActor and QuestionSetActor. This removes deprecated API usage (Scala 2.13+ compatibility) and keeps runtime behavior unchanged. --------- Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Mahesh Kumar Gangula <mahesh@sanketika.in>
|
❌ The last analysis has failed. |
SonarCloud Analysis Results 🔍Quality Gate Results for Services:Please review the analysis results for each service. Ensure all quality gates are passing before merging. |
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.
In all recoverWith { case e: CompletionException => throw e.getCause } blocks, throwing inside recoverWith breaks the Future monad: the thrown exception escapes the Future, making it uncatchable by downstream Future combinators (.map, .flatMap, .recover, etc.).
The correct pattern is to return Future.failed(e.getCause), which keeps the failure inside the Future chain so callers can handle it via .recoverWith or .failed.foreach as expected.
Affects create(), update(), read(), search(), and enrichHierarchyAndUpdate() in DataNode — 5 sites updated.
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please choose appropriate options.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes in the below checkboxes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: