Unified ElasticSearch Utility Module#1288
Conversation
… use flexible index options.
- Mock InstructionEventGenerator to fix test timeout - Upgrade Jackson to 2.14.3 to fix dependency mismatch - Fix assertions in UserOwnershipTransferActorTest
|
There was a problem hiding this comment.
Pull request overview
This PR consolidates ElasticSearch utility logic from disparate implementations across microservices into a single unified module core/sunbird-es-utils. The refactoring removes the legacy es-utils module and updates all dependent services to use the new centralized implementation.
Changes:
- Created new
core/sunbird-es-utilsmodule with REST high-level client implementation, connection management, and query building utilities - Removed legacy
core/es-utilsmodule including all source files, tests, and resources - Updated dependency management with centralized version properties in core/pom.xml and migrated cassandra-utils to use shared properties
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| core/sunbird-es-utils/pom.xml | New module POM with ES 7.10.2 dependencies and managed HTTP component versions |
| core/sunbird-es-utils/src/main/java/org/sunbird/helper/ConnectionManager.java | Thread-safe singleton connection manager for ES REST client with environment-based configuration |
| core/sunbird-es-utils/src/main/java/org/sunbird/common/inf/ElasticSearchService.java | Service interface defining async ES operations (CRUD, search, bulk, health check) |
| core/sunbird-es-utils/src/main/java/org/sunbird/common/factory/EsClientFactory.java | Factory for creating ElasticSearchService instances with singleton pattern |
| core/sunbird-es-utils/src/main/java/org/sunbird/common/ElasticSearchRestHighImpl.java | Complete REST client implementation with async operations using Scala Futures |
| core/sunbird-es-utils/src/main/java/org/sunbird/common/ElasticSearchHelper.java | Query building utilities with improved documentation and corrected typos |
| core/sunbird-es-utils/src/main/java/org/sunbird/dto/SearchDTO.java | Enhanced DTO with better documentation and increased default limit |
| core/platform-common/src/main/java/org/sunbird/util/ProjectUtil.java | Added isNotNull helper and new ES index types (course, courseBatch) |
| core/platform-common/src/main/java/org/sunbird/keys/JsonKey.java | Added ES_COURSE_INDEX and ES_COURSE_BATCH_INDEX constants |
| core/platform-common/src/main/java/org/sunbird/exception/ResponseCode.java | Added invalidData response code for ES validation errors |
| service/src/test/java/org/sunbird/actor/user/UserOwnershipTransferActorTest.java | Updated error codes from UOS_UOWNTRANS0028 to UOS_UOWNTRANS0029 |
| service/src/test/java/org/sunbird/actor/user/UserDeletionBackgroundJobActorTest.java | Added InstructionEventGenerator to PowerMock setup |
| service/pom.xml | Updated dependency artifact ID from es-utils to sunbird-es-utils |
| core/pom.xml | Centralized version properties and updated module reference |
| core/sunbird-cassandra-utils/pom.xml | Migrated to use centralized version properties from parent POM |
| core/es-utils/* | Complete removal of legacy module (source, tests, resources, config) |
Comments suppressed due to low confidence (5)
core/sunbird-es-utils/src/main/java/org/sunbird/dto/SearchDTO.java:41
- The default limit has been changed from 250 to 1000, which is a 4x increase. This change could have significant performance implications:
- It may cause slower queries and increased memory usage
- It could lead to timeouts on large datasets
- There's no mention of this change in the PR description
This appears to be an unintended change or should be documented as a deliberate behavioral modification with justification for why the limit was increased.
core/sunbird-es-utils/src/main/java/org/sunbird/common/ElasticSearchHelper.java:766
- Method naming typo: "getBasicBuiders" should be "getBasicBuilders". This typo appears in both the method definition and its call.
core/sunbird-es-utils/src/main/java/org/sunbird/common/ElasticSearchHelper.java:269 - Method naming typo in several methods: "createFilterESOpperation" should be "createFilterESOperation", "createNestedFilterESOpperation" should be "createNestedFilterESOperation", "createESOpperation" should be "createESOperation", and "createNestedESOpperation" should be "createNestedESOperation". The typo "Opperation" appears in multiple method names throughout the file.
core/sunbird-es-utils/src/main/java/org/sunbird/dto/SearchDTO.java:226 - getGroupQuery exposes the internal representation stored in field groupQuery. The value may be modified after this call to getGroupQuery.
core/sunbird-es-utils/src/main/java/org/sunbird/dto/SearchDTO.java:134 - getAdditionalProperties exposes the internal representation stored in field additionalProperties. The value may be modified after this call to getAdditionalProperties.
getAdditionalProperties exposes the internal representation stored in field additionalProperties. The value may be modified after this call to getAdditionalProperties.
getAdditionalProperties exposes the internal representation stored in field additionalProperties. The value may be modified after this call to getAdditionalProperties.
getAdditionalProperties exposes the internal representation stored in field additionalProperties. The value may be modified after this call to getAdditionalProperties.
getAdditionalProperties exposes the internal representation stored in field additionalProperties. The value may be modified after this call to getAdditionalProperties.
getAdditionalProperties exposes the internal representation stored in field additionalProperties. The value may be modified after this call to getAdditionalProperties.
getAdditionalProperties exposes the internal representation stored in field additionalProperties. The value may be modified after this call to getAdditionalProperties.
getAdditionalProperties exposes the internal representation stored in field additionalProperties. The value may be modified after this call to getAdditionalProperties.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Build HttpHost array for all configured hosts | ||
| HttpHost[] httpHosts = new HttpHost[hostList.size()]; | ||
| for (int i = 0; i < hostList.size(); i++) { | ||
| httpHosts[i] = new HttpHost(hostList.get(i), 9200, "http"); |
There was a problem hiding this comment.
The port number is hardcoded to 9200 in the createRestClient method. However, the code reads the port configuration from the environment variable SUNBIRD_ES_PORT (lines 56, 77-87), parses it, and stores it in the ports list, but then completely ignores it and uses the hardcoded value 9200.
This creates a confusing situation where:
- The environment variable SUNBIRD_ES_PORT is read but not used
- Users cannot configure the port through environment variables
- The ports list is populated but never used
Either use the configured port values from the environment or remove the unused port configuration parsing logic.
| } | ||
|
|
||
| public static boolean isNotNull(Object obj) { | ||
| return null != obj ? true : false; |
There was a problem hiding this comment.
The isNotNull method uses a ternary operator that returns true or false based on a boolean condition, which is redundant. The method can be simplified to directly return the boolean expression.
The current implementation return null != obj ? true : false; is unnecessarily verbose and can be simplified to return obj != null;.
| return null != obj ? true : false; | |
| return obj != null; |
| } | ||
|
|
||
| // Parse comma-separated hosts | ||
| String[] splitedHost = hostName.split(","); |
There was a problem hiding this comment.
The typo in variable name "splitedHost" should be corrected to "splitHost". The same typo exists in "splitedPort" on line 77 which should be "splitPort". This maintains consistency with standard Java naming conventions and improves code readability.
| } | ||
|
|
||
| // Parse comma-separated ports (currently stored but not used) | ||
| String[] splitedPort = port.split(","); |
There was a problem hiding this comment.
The typo in variable name "splitedPort" should be corrected to "splitPort" for consistency with standard Java naming conventions.
| static { | ||
| // Prevent Netty from using runtime available processors check | ||
| System.setProperty("es.set.netty.runtime.available.processors", "false"); | ||
|
|
||
| // Initialize connection on class loading | ||
| initialiseRestClientConnection(); | ||
|
|
||
| // Register shutdown hook for cleanup | ||
| registerShutDownHook(); | ||
| } |
There was a problem hiding this comment.
The static initializer block performs connection initialization during class loading, which can cause issues:
- Class loading failures: If ElasticSearch is unavailable during application startup, the class cannot be loaded at all
- Difficult to test: Static initialization makes unit testing challenging
- Long startup times: Connection initialization happens synchronously during class loading
- No recovery mechanism: If initialization fails, the application cannot retry without restarting
Consider using lazy initialization in getRestClient() method instead of eager initialization in the static block, or at minimum handle failures more gracefully to allow the application to start even if ES is temporarily unavailable.
| private static final List<String> hosts = new ArrayList<>(); | ||
|
|
||
| /** List of Elasticsearch ports (populated from env but currently unused). */ | ||
| private static final List<Integer> ports = new ArrayList<>(); |
There was a problem hiding this comment.
The contents of this container are never accessed.
Summary
Re-architected and rewrote the ElasticSearch utility layer into a single, unified
core/sunbird-es-utilsmodule. This new implementation consolidates disparate ElasticSearch handling logic from across all microservices into one central library.Key Changes
core/sunbird-es-utilsas the single source of truth for all ElasticSearch operations.es-utilsmodule and refactored all dependent services (Actors, DAOs) to use the new unified implementation.Impact
Eliminates code duplication across microservices, enforces a standardized database access pattern, and simplifies future maintenance and upgrades.