MOSIP-40963 - fix/enhancement - batch + parallel enrichment for Zones/Centers Users Mapping #1262
Conversation
| Math.max(2, Runtime.getRuntime().availableProcessors()), | ||
| 8 |
There was a problem hiding this comment.
Min(2) and Max(8) value can be taken from properties and set the default values to 2 and 8
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a configurable fixed-thread enrichment executor and refactors services to perform asynchronous batch enrichment of user names and zone names using CompletableFuture and the new executor; repository and utility methods added to fetch zones by codes and language. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant Executor
participant UserAPI
participant ZoneService
rect rgb(230,245,255)
Note over Executor: New enrichmentExecutor (fixed pool) used for async tasks
end
Client->>Service: search...Mapping(filters)
activate Service
Service->>Service: map DTOs, collect userIds & zoneCodes
par Batch fetches (async)
Service->>Executor: submit getUsersNames(userIds)
activate Executor
Executor->>UserAPI: batch fetch user display names
Executor-->>Service: return Map<userId,name>
deactivate Executor
and
Service->>ZoneService: getZonesByCodesAndLanguage(zoneCodes, lang)
ZoneService-->>Service: return List<ZoneNameDto>
end
Service->>Service: join futures, populate DTOs with userName & zoneName
Service-->>Client: return enriched response
deactivate Service
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/config/CommonConfig.java (1)
89-90: Make thread pool size configurable.The min (2) and max (8) thread pool sizes are hard-coded. Consider externalizing these to configuration properties for better flexibility across environments.
Apply this diff to use configurable properties:
+@Value("${mosip.kernel.masterdata.enrichment-executor.min-pool-size:2}") +private int enrichmentMinPoolSize; + +@Value("${mosip.kernel.masterdata.enrichment-executor.max-pool-size:8}") +private int enrichmentMaxPoolSize; + @Bean(destroyMethod = "shutdown") public ExecutorService enrichmentExecutor() { final ExecutorService executorService = Executors.newFixedThreadPool( Math.min( - Math.max(2, Runtime.getRuntime().availableProcessors()), - 8 + Math.max(enrichmentMinPoolSize, Runtime.getRuntime().availableProcessors()), + enrichmentMaxPoolSize ) ); return new DelegatingSecurityContextExecutorService(executorService); }
🧹 Nitpick comments (3)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneServiceImpl.java (1)
297-319: Consider returning empty list instead of throwing exception for batch operations.The current implementation throws
DataNotFoundExceptionwhen no zones are found (Line 309). For batch enrichment scenarios, it might be more resilient to return an empty list, allowing partial results rather than failing the entire operation.Apply this diff if you prefer a more lenient approach:
if (zones == null || zones.isEmpty()) { - throw new DataNotFoundException(ZoneErrorCode.ZONE_ENTITY_NOT_FOUND.getErrorCode(), - ZoneErrorCode.ZONE_ENTITY_NOT_FOUND.getErrorMessage()); + return Collections.emptyList(); }admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java (1)
470-509: Improve error handling in batch user name retrieval.The method silently returns an empty map on any exception (Line 508), which could mask critical issues like authentication failures or service unavailability. Consider differentiating between expected empty results and actual errors.
Consider this approach:
} catch (Exception e) { logger.error("failed to get users names from authmanager", e); + // Re-throw if it's an authentication/authorization issue + if (e instanceof HttpClientErrorException && + ((HttpClientErrorException) e).getRawStatusCode() == 401) { + throw new MasterDataServiceException("AUTH-001", "Authentication failed"); + } } return Collections.emptyMap();admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java (1)
780-820: Extract duplicate getUsersNames method to shared utility.The
getUsersNamesmethod is duplicated betweenUserDetailsServiceImpl(Lines 780-820) andZoneUserServiceImpl(Lines 470-509 in that file). Consider extracting this to a shared utility class to reduce duplication and improve maintainability.Create a new
UserNameEnrichmentUtilclass:@Component public class UserNameEnrichmentUtil { @Autowired private RestTemplate restTemplate; @Autowired private ObjectMapper objectMapper; @Value("${zone.user.details.url}") private String userDetailsUrl; public Map<String, String> getUsersNames(List<String> usersIds) { // Move implementation here } }Then inject and use in both service implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/config/CommonConfig.java(3 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/ZoneRepository.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/ZoneService.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java(5 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneServiceImpl.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java(6 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/utils/ZoneUtils.java(1 hunks)
🔇 Additional comments (4)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/utils/ZoneUtils.java (1)
390-392: LGTM! Simple delegation to repository.The method appropriately delegates to the repository layer for batch zone retrieval by codes and language.
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/ZoneService.java (1)
46-47: LGTM! Service method declaration.The new method signature appropriately supports batch zone retrieval by codes and language, aligning with the enrichment optimization goals.
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java (1)
101-102: LGTM! Executor properly autowired for async enrichment.The enrichment executor is correctly autowired and will propagate security context to async tasks.
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java (1)
154-156: LGTM! Executor properly autowired for async enrichment.The enrichment executor is correctly autowired for parallel data fetching.
...l-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/ZoneRepository.java
Outdated
Show resolved
Hide resolved
...ta-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java
Outdated
Show resolved
Hide resolved
...rdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java
Outdated
Show resolved
Hide resolved
ymahtat-dev
force-pushed
the
draft-develop-2
branch
2 times, most recently
from
8aa7394 to
ce468ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/utils/ZoneUtils.java (1)
390-392: Add input validation for robustness.The method delegates directly to the repository without validating inputs. Consider adding null/empty checks for
zonesCodesandlangCodeto fail fast with a clear error or return an empty list for empty input.Apply this diff to add validation:
public List<Zone> getZonesByCodesAndLanguage(List<String> zonesCodes, String langCode) { + if (zonesCodes == null || zonesCodes.isEmpty()) { + return Collections.emptyList(); + } + if (langCode == null || langCode.isEmpty()) { + logger.warn("langCode is null or empty in getZonesByCodesAndLanguage"); + return Collections.emptyList(); + } return zoneRepository.findZonesByCodesAndLangCodeNonDeletedAndIsActive(zonesCodes, langCode); }admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java (1)
796-836: Consider extracting duplicate code to a shared utility.The
getUsersNamesmethod in this file is nearly identical to the same method inZoneUserServiceImpl.java(lines 486-525). This duplication violates the DRY principle and makes maintenance harder. Consider extracting this logic to a shared utility class or a common base service.You could create a
UserNameEnrichmentUtilor add this method to an existing utility class that both services can use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/config/CommonConfig.java(4 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/ZoneRepository.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/ZoneService.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java(5 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneServiceImpl.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java(6 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/utils/ZoneUtils.java(1 hunks)admin/kernel-masterdata-service/src/main/resources/application-local1.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneServiceImpl.java
- admin/kernel-masterdata-service/src/main/resources/application-local1.properties
- admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/config/CommonConfig.java
- admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/ZoneRepository.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: admin-service / build-dockers
- GitHub Check: build-maven-apitest-masterdata / maven-build
🔇 Additional comments (5)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java (2)
155-159: LGTM!The executor and timeout configuration are properly wired and follow good practices with sensible defaults.
699-728: LGTM!The parallel enrichment logic with timeout protection and fallback handling is well implemented. The use of
orTimeoutwithexceptionallyensures the thread won't block indefinitely and provides safe defaults on failure.admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/ZoneService.java (1)
46-47: LGTM!The new interface method declaration is clear and follows the existing pattern in this service interface.
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java (2)
102-106: LGTM!The executor and timeout configuration are properly wired and consistent with the implementation in
UserDetailsServiceImpl.java.
388-417: LGTM!The parallel enrichment logic with timeout protection and fallback handling is well implemented. The approach is consistent with
UserDetailsServiceImpl.javaand handles edge cases appropriately.
...ta-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java
Show resolved
Hide resolved
...rdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java
Show resolved
Hide resolved
…/Centers Users Mapping Signed-off-by: Youssef MAHTAT <youssef.mahtat.as.developer@gmail.com>
ymahtat-dev
force-pushed
the
draft-develop-2
branch
from
ce468ef to
3364a9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java (1)
796-836: Consider extracting to shared utility to reduce duplication.This
getUsersNamesmethod is identical to the one inZoneUserServiceImpl(lines 486-525). As noted in the review of that file, consider extracting this to a shared service or utility class to eliminate duplication.
🧹 Nitpick comments (1)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java (1)
486-525: Consider extracting to shared utility to reduce duplication.This
getUsersNamesmethod is duplicated verbatim inUserDetailsServiceImpl(lines 796-836). Consider extracting it to a shared service or utility class to maintain a single source of truth and reduce maintenance burden.For example, create a new
UserNameServiceclass:@Component public class UserNameService { @Autowired private RestTemplate restTemplate; @Value("${zone.user.details.url}") private String userDetails; @Autowired private ObjectMapper mapper; public Map<String, String> getUsersNames(List<String> usersIds) { // move implementation here } }Then inject and use it in both
ZoneUserServiceImplandUserDetailsServiceImpl.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/config/CommonConfig.java(4 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/ZoneRepository.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/ZoneService.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java(5 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneServiceImpl.java(1 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java(6 hunks)admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/utils/ZoneUtils.java(1 hunks)admin/kernel-masterdata-service/src/main/resources/application-local1.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- admin/kernel-masterdata-service/src/main/resources/application-local1.properties
- admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/ZoneService.java
- admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneServiceImpl.java
- admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/utils/ZoneUtils.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-maven-admin-services / maven-build
- GitHub Check: build-maven-apitest-masterdata / maven-build
🔇 Additional comments (7)
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/config/CommonConfig.java (2)
37-41: LGTM!The configurable thread pool bounds with sensible defaults (min=2, max=8) address the previous review feedback and allow runtime tuning without code changes.
92-101: LGTM!The executor configuration correctly:
- Bounds the thread pool size between configured min/max using available processors as a baseline
- Wraps with
DelegatingSecurityContextExecutorServiceto propagate security context to async enrichment tasks- Ensures proper cleanup with
destroyMethod="shutdown"admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/repository/ZoneRepository.java (1)
28-29: LGTM!The batch zone query correctly filters by codes, language, non-deleted status, and active status. The previous review concern about the missing
isActivecheck has been properly addressed.admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/ZoneUserServiceImpl.java (2)
102-106: LGTM!The enrichment executor is properly autowired and the timeout configuration with a sensible default of 30 seconds aligns with the overall enrichment strategy.
388-417: LGTM!The parallel batch enrichment correctly:
- Collects user IDs and zone codes for batch lookups
- Executes both batch operations concurrently using the enrichment executor
- Applies configurable timeout protection via
orTimeout- Provides safe fallbacks (empty collections) with error logging
This replaces N+1 per-row lookups with two parallel batch operations, significantly improving performance.
admin/kernel-masterdata-service/src/main/java/io/mosip/kernel/masterdata/service/impl/UserDetailsServiceImpl.java (2)
155-159: LGTM!The enrichment executor wiring is consistent with
ZoneUserServiceImpland follows the same pattern with appropriate timeout configuration.
699-728: LGTM!The parallel batch enrichment follows the same robust pattern as
ZoneUserServiceImpl:
- Concurrent execution of batch user name and zone lookups
- Timeout protection with safe fallbacks
- Proper error logging
The use of
Zoneentities (viazoneUtils) instead of DTOs is appropriate for this context.
dhanendra06
left a comment
dhanendra06
left a comment
There was a problem hiding this comment.
Please check this issue in qa-java21 env if issue still persist because as part of sync data performance turning activity this was addressed.
3 participants
to propagate SecurityContext for async tasks (CommonConfig.java)
(UserDetailsServiceImpl.java / ZoneUserServiceImpl.java)
ZoneRepository.findZonesByCodesAndLangCodeNonDeletedAndIsActive(...)
(UserDetailsServiceImpl.java, ZoneUserServiceImpl.java)
(usersNamesByIds.get(...), zoneNameByCode.get(...))
Summary by CodeRabbit
New Features
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.