Unified ElasticSearch Utility Module#644
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request consolidates the ElasticSearch utility layer by moving it from course-mw/sunbird-util/sunbird-es-utils to a new unified core/sunbird-es-utils module. This architectural change standardizes ElasticSearch access patterns across all microservices and eliminates code duplication.
Changes:
- Created new
core/sunbird-es-utilsmodule with improved implementation and documentation - Removed legacy
course-mw/sunbird-util/sunbird-es-utilsmodule completely - Updated ElasticSearchService interface with reordered parameters (RequestContext moved to last position)
- Refactored all dependent services (actors, DAOs, tests) to use new method signatures
- Centralized dependency version management in parent POM
Reviewed changes
Copilot reviewed 78 out of 80 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/sunbird-es-utils/* | New unified ES utils module with improved connection management, comprehensive documentation, and better error handling |
| core/pom.xml | Added new module and centralized all dependency version properties |
| course-mw/sunbird-util/pom.xml | Removed reference to old sunbird-es-utils module |
| course-mw/sunbird-util/sunbird-es-utils/* | Complete removal of legacy ES utils implementation |
| course-mw/**/JsonKey.java | Added new constants: SEARCH_FUZZY and REST |
| course-mw/**/Actor.java | Updated all ES service method calls to new signature order |
| course-mw/**/Test.java | Updated all mock expectations to match new method signatures |
Comments suppressed due to low confidence (2)
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.
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.
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.
| long startTime = System.currentTimeMillis(); | ||
| Promise<String> promise = Futures.promise(); | ||
|
|
||
| logger.debug(requestContext, "ElasticSearchRestHighImpl:save: method started at ==" + startTime + " for Index " + index); | ||
|
|
||
| if (StringUtils.isBlank(identifier) || StringUtils.isBlank(index)) { | ||
| logger.info(requestContext, "ElasticSearchRestHighImpl:save: Identifier or Index value is null or empty, identifier : " | ||
| + identifier + ", index: " + index + ", not able to save data."); | ||
| promise.failure(ProjectUtil.createClientException(ResponseCode.invalidData)); | ||
| return promise.future(); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
The save method now throws an exception for invalid input instead of returning "ERROR" as a string. This is an API behavior change that could break existing error handling code that checks for the "ERROR" string return value. The old implementation (based on the removed test) returned "ERROR" on validation failure, but the new implementation throws an exception via promise.failure().
Consider adding migration documentation or checking if all callers handle exceptions properly instead of checking for "ERROR" string.
| long startTime = System.currentTimeMillis(); | |
| Promise<String> promise = Futures.promise(); | |
| logger.debug(requestContext, "ElasticSearchRestHighImpl:save: method started at ==" + startTime + " for Index " + index); | |
| if (StringUtils.isBlank(identifier) || StringUtils.isBlank(index)) { | |
| logger.info(requestContext, "ElasticSearchRestHighImpl:save: Identifier or Index value is null or empty, identifier : " | |
| + identifier + ", index: " + index + ", not able to save data."); | |
| promise.failure(ProjectUtil.createClientException(ResponseCode.invalidData)); | |
| return promise.future(); | |
| } | |
| try { | |
| long startTime = System.currentTimeMillis(); | |
| Promise<String> promise = Futures.promise(); | |
| logger.debug(requestContext, "ElasticSearchRestHighImpl:save: method started at ==" + startTime + " for Index " + index); | |
| if (StringUtils.isBlank(identifier) || StringUtils.isBlank(index)) { | |
| logger.info( | |
| requestContext, | |
| "ElasticSearchRestHighImpl:save: Identifier or Index value is null or empty, identifier : " | |
| + identifier | |
| + ", index: " | |
| + index | |
| + ", not able to save data."); | |
| // Maintain backward-compatible behavior: return "ERROR" instead of throwing an exception. | |
| promise.success(ERROR); | |
| return promise.future(); | |
| } | |
| try { |
| String[] splitedHost = hostName.split(","); | ||
| for (String host : splitedHost) { | ||
| String trimmedHost = host.trim(); | ||
| if (StringUtils.isNotBlank(trimmedHost)) { | ||
| hosts.add(trimmedHost); | ||
| } | ||
| } | ||
|
|
||
| // Parse comma-separated ports (currently stored but not used) | ||
| String[] splitedPort = port.split(","); |
There was a problem hiding this comment.
Inconsistent use of "splited" instead of "split" in variable names. While this is existing code being moved, consider using proper English grammar: splitedHost should be splitHost or hostArray, and splitedPort should be splitPort or portArray.
| // Parse comma-separated ports (currently stored but not used) | ||
| String[] splitedPort = port.split(","); | ||
| for (String portStr : splitedPort) { | ||
| String trimmedPort = portStr.trim(); | ||
| if (StringUtils.isNotBlank(trimmedPort)) { | ||
| try { | ||
| ports.add(Integer.parseInt(trimmedPort)); | ||
| } catch (NumberFormatException e) { | ||
| logger.warn(null, "Invalid port number in SUNBIRD_ES_PORT: " + trimmedPort, null); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ports list is populated from environment variables but never actually used. The hardcoded port 9200 is always used on line 155. Either remove the unused ports list and related parsing logic (lines 76-87), or use the parsed port values instead of hardcoding 9200. The comment on line 27 acknowledges this but it should be addressed.
| /** 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.
| /** List of Elasticsearch ports (populated from env but currently unused). */ | |
| private static final List<Integer> ports = new ArrayList<>(); | |
|
|







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.course-mw/sunbird-util/sunbird-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.