Duplicate records in obd file fix #873
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant CallSummaryRecord
participant CallSummaryRecordDto
CallSummaryRecord->>CallSummaryRecordDto: toDto() (now includes serviceId)
sequenceDiagram
participant Service
participant SubscriptionServiceImpl
participant CallRetryDataService
Service->>SubscriptionServiceImpl: updateCallRetry(subscriptionId, msisdn)
SubscriptionServiceImpl->>CallRetryDataService: findBySubscriptionId(subscriptionId)
CallRetryDataService-->>SubscriptionServiceImpl: CallRetry
SubscriptionServiceImpl->>CallRetryDataService: update(msisdn)
sequenceDiagram
participant DayOfTheWeek
participant Client
Client->>DayOfTheWeek: getDayOfTheWeekFromTimestamp(timestamp)
DayOfTheWeek-->>Client: DayOfTheWeek enum value
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/CsrServiceImpl.java (1)
Line range hint
150-251: Refactor method for better readability and maintainability.The
doReschedulemethod has become complex with multiple nested conditions and mixed responsibilities. Consider:
- Extracting the retry logic into separate methods
- Using early returns to reduce nesting
- Moving logging to a debug level and using structured logging
Example refactor for better structure:
private void doReschedule(Subscription subscription, CallRetry existingCallRetry, CallSummaryRecordDto csrDto) { boolean invalidNr = StatusCode.fromInt(csrDto.getStatusCode()).equals(StatusCode.OBD_FAILED_INVALIDNUMBER); - LOGGER.info("inside doreshedule method"); - LOGGER.info("this is the data inside: {},{},{]",subscription,existingCallRetry, csrDto); + LOGGER.debug("Processing reschedule - subscription: {}, existingCallRetry: {}, csrDto: {}", + subscription, existingCallRetry, csrDto); if (shouldCreateNewRetry(subscription, existingCallRetry)) { createNewRetry(subscription, csrDto, invalidNr); return; } if (shouldStopRetrying(subscription, existingCallRetry)) { handleLastRetry(subscription, existingCallRetry, csrDto); return; } updateExistingRetry(existingCallRetry, invalidNr); } private boolean shouldCreateNewRetry(Subscription subscription, CallRetry existingCallRetry) { return existingCallRetry == null && SubscriptionStatus.ACTIVE.equals(subscription.getStatus()); }
🧹 Nitpick comments (5)
kilkari/src/main/java/org/motechproject/nms/kilkari/dto/CallSummaryRecordDto.java (1)
182-183: LGTM! Parameter mapping methods updated consistently.The fromParams and toParams methods are correctly updated to handle the new serviceId field.
However, consider adding null checks for serviceId in fromParams to prevent potential NullPointerException.
@Ignore public static CallSummaryRecordDto fromParams(Map<String, Object> params) { CallSummaryRecordDto csr; csr = new CallSummaryRecordDto( (String) params.get("subscriptionId"), (int) params.get("statusCode"), (int) params.get("finalStatus"), (String) params.get("contentFileName"), (String) params.get("weekId"), (String) params.get("languageCode"), (String) params.get("circleName"), (String) params.get("targetFileTimeStamp"), (Boolean) params.get("opt_in_call_eligibility"), (String) params.get("opt_in_input"), - (String) params.get("serviceId") + params.containsKey("serviceId") ? (String) params.get("serviceId") : null ); return csr; }Also applies to: 201-201
kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriptionServiceImpl.java (1)
Line range hint
1020-1036: Consider using bulk update for modification dates.While the code correctly updates modification dates, consider using a bulk update operation for better performance when dealing with multiple active subscriptions.
-Set<Subscription> activeSubscriptions = subscriber.getActiveAndPendingSubscriptions(); -if (activeSubscriptions != null && !activeSubscriptions.isEmpty()) { - for(Subscription sub : activeSubscriptions ){ - sub.setModificationDate(DateTime.now()); - } -} -subscription.setModificationDate(DateTime.now()); +DateTime now = DateTime.now(); +Set<Subscription> activeSubscriptions = subscriber.getActiveAndPendingSubscriptions(); +if (activeSubscriptions != null && !activeSubscriptions.isEmpty()) { + subscriptionDataService.bulkUpdate("modificationDate", now, activeSubscriptions); +} +subscription.setModificationDate(now);kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriberServiceImpl.java (1)
409-427: Improve logging statements using parameterized logging.The logging statements should use parameterized logging for better performance and readability.
-LOGGER.info("1st value: {}",subscriberByRchId.getLastMenstrualPeriod().getDayOfYear()); -LOGGER.info("second value: {}",lmp.getDayOfYear()); -LOGGER.info("third value: {}",subscriberByRchId.getLastMenstrualPeriod().getYear()); -LOGGER.info("second value: {}",lmp.getYear()); -LOGGER.info("this is the first case: {}",(subscriberByRchId.getLastMenstrualPeriod().getDayOfYear() == lmp.getDayOfYear())); -LOGGER.info("this is the second case2: {}",(subscriberByRchId.getLastMenstrualPeriod().getYear() == lmp.getYear())); +LOGGER.info("LMP comparison - Current: day={}, year={}, New: day={}, year={}, DayMatch={}, YearMatch={}", + subscriberByRchId.getLastMenstrualPeriod().getDayOfYear(), + subscriberByRchId.getLastMenstrualPeriod().getYear(), + lmp.getDayOfYear(), + lmp.getYear(), + subscriberByRchId.getLastMenstrualPeriod().getDayOfYear() == lmp.getDayOfYear(), + subscriberByRchId.getLastMenstrualPeriod().getYear() == lmp.getYear());kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/CsrServiceImpl.java (2)
31-31: Remove unused import.The
SettingsFacadeimport is not used anywhere in the code.-import org.motechproject.server.config.SettingsFacade;
411-421: Enhance robustness of route number extraction.The current implementation could be improved for better maintainability and error handling.
Consider this enhanced implementation:
- private String extractRouteNumber(String serviceId) { - if (serviceId != null && serviceId.contains("Retryonroute")) { - LOGGER.info("inside this1"); - int startIndex = serviceId.indexOf("Retryonroute") + "Retryonroute".length(); - if (startIndex < serviceId.length()) { - LOGGER.info("inside this2"); - return String.valueOf(serviceId.charAt(startIndex)); - } - } - return ""; - } + private static final String ROUTE_PREFIX = "Retryonroute"; + + private String extractRouteNumber(String serviceId) { + if (serviceId == null || !serviceId.contains(ROUTE_PREFIX)) { + return ""; + } + + try { + int startIndex = serviceId.indexOf(ROUTE_PREFIX) + ROUTE_PREFIX.length(); + return startIndex < serviceId.length() ? + String.valueOf(serviceId.charAt(startIndex)) : + ""; + } catch (IndexOutOfBoundsException e) { + LOGGER.warn("Failed to extract route number from serviceId: {}", serviceId, e); + return ""; + } + }Improvements:
- Added constant for the route prefix
- Added try-catch for better error handling
- Removed unnecessary debug logging
- Made the code more concise and readable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
imi/src/main/java/org/motechproject/nms/imi/domain/CallSummaryRecord.java(1 hunks)kilkari/src/main/java/org/motechproject/nms/kilkari/dto/CallSummaryRecordDto.java(6 hunks)kilkari/src/main/java/org/motechproject/nms/kilkari/service/SubscriptionService.java(1 hunks)kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/CsrServiceImpl.java(10 hunks)kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriberServiceImpl.java(3 hunks)kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriptionServiceImpl.java(1 hunks)props/src/main/java/org/motechproject/nms/props/domain/DayOfTheWeek.java(2 hunks)
🔇 Additional comments (9)
imi/src/main/java/org/motechproject/nms/imi/domain/CallSummaryRecord.java (1)
206-207: LGTM! Addition of serviceId parameter helps track service information.The change to include serviceId in the DTO conversion is well-implemented and aligns with the goal of preventing duplicate records.
kilkari/src/main/java/org/motechproject/nms/kilkari/dto/CallSummaryRecordDto.java (3)
40-41: LGTM! Field declaration is well-placed.The serviceId field is properly declared and positioned with other fields.
58-58: LGTM! Constructor updated correctly.The constructor is properly updated to include the new serviceId parameter while maintaining backward compatibility.
Also applies to: 69-69
161-168: LGTM! Getter and setter follow conventions.The getter and setter methods for serviceId are well-implemented following Java bean conventions.
props/src/main/java/org/motechproject/nms/props/domain/DayOfTheWeek.java (1)
4-5: LGTM!The added imports from Joda Time are appropriate for the new timestamp parsing functionality.
kilkari/src/main/java/org/motechproject/nms/kilkari/service/SubscriptionService.java (1)
109-110: LGTM! Method signature is well-defined.The new method signature clearly specifies its purpose to update call retry information with appropriate parameters.
kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriptionServiceImpl.java (1)
947-955: LGTM! Implementation is clean and handles null checks.The implementation correctly updates the MSISDN for an existing call retry record and includes appropriate null checks.
kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriberServiceImpl.java (1)
749-757: LGTM! Call retry management is correctly implemented.The code correctly manages call retry records based on date changes, either deleting or updating them as needed.
kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/CsrServiceImpl.java (1)
360-364: 🛠️ Refactor suggestionDocument the complex condition and consider edge cases.
The condition for handling RCH updates is complex and lacks documentation explaining the business logic. Additionally, there might be a race condition if multiple updates arrive simultaneously.
- Add documentation explaining the logic:
+ // Handle cases where RCH updates have changed the LMP date + // Route 1: Fresh calls after RCH update + // Route 2: Retry calls after RCH update + // Skip rescheduling if the day of week has changed due to LMP update if(callRetry == null && !csrDto.getWeekId().equals("w1_1") && !subscription.getFirstMessageDayOfWeek().equals(DayOfTheWeek.getDayOfTheWeekFromTimestamp(csrDto.getTargetFileTimeStamp())) && "1".equals(extractRouteNumber(csrDto.getServiceId()))) { LOGGER.info("inside fresh call condition after rch update"); } else if(callRetry == null && !csrDto.getWeekId().equals("w1_1") && "2".equals(extractRouteNumber(csrDto.getServiceId()))){ LOGGER.info("inside retry call condition after rch update"); }
- Consider adding synchronization or version control to handle concurrent updates:
@Version private Long version;✅ Verification successful
Complex condition is necessary, but needs documentation
The condition handles a specific business case for RCH updates affecting LMP dates, and the extensive debug logging already helps with monitoring. The suggested synchronization is unnecessary as this appears to be a business logic concern rather than a concurrency issue.
- Add documentation as suggested to explain the business logic:
+ // Handle cases where RCH updates have changed the LMP date: + // - Skip processing for first week messages (w1_1) + // - Verify day of week changes from LMP updates + // - Handle fresh calls (Route 1) and retry calls (Route 2) separately🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for similar RCH update handling patterns and version annotations echo "=== Searching for RCH update handling patterns ===" rg -l "rch update" --type java echo -e "\n=== Checking for Version annotations and synchronization ===" rg "@Version|@Lock" --type java echo -e "\n=== Looking for related documentation ===" fd README.md | xargs rg -i "rch|lmp|synchronization" || true echo -e "\n=== Searching for similar complex conditions ===" rg "getFirstMessageDayOfWeek|getDayOfTheWeekFromTimestamp" --type java -A 2 -B 2Length of output: 4614
| public static DayOfTheWeek getDayOfTheWeekFromTimestamp(String timestamp) { | ||
| DateTimeFormatter formatter = DateTimeFormat.forPattern("yyyyMMddHHmmss"); | ||
|
|
||
| DateTime dateTime = formatter.parseDateTime(timestamp); | ||
|
|
||
| int dayOfWeek = dateTime.getDayOfWeek(); // 1 = Monday, 7 = Sunday | ||
|
|
||
| return DayOfTheWeek.fromInt(dayOfWeek); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and documentation.
The method needs the following improvements:
- Add null check and proper exception handling for parsing errors
- Add javadoc to document the expected timestamp format
- Make the formatter a static final field for better performance
Here's the suggested implementation:
+ private static final DateTimeFormatter TIMESTAMP_FORMATTER =
+ DateTimeFormat.forPattern("yyyyMMddHHmmss");
+
+ /**
+ * Converts a timestamp string to DayOfTheWeek.
+ *
+ * @param timestamp String in format "yyyyMMddHHmmss"
+ * @return DayOfTheWeek corresponding to the timestamp
+ * @throws IllegalArgumentException if timestamp is null or in invalid format
+ */
public static DayOfTheWeek getDayOfTheWeekFromTimestamp(String timestamp) {
- DateTimeFormatter formatter = DateTimeFormat.forPattern("yyyyMMddHHmmss");
+ if (timestamp == null) {
+ throw new IllegalArgumentException("Timestamp cannot be null");
+ }
+ try {
+ DateTime dateTime = TIMESTAMP_FORMATTER.parseDateTime(timestamp);
+ return DayOfTheWeek.fromInt(dateTime.getDayOfWeek());
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(
+ "Invalid timestamp format. Expected: yyyyMMddHHmmss", e);
+ }
- DateTime dateTime = formatter.parseDateTime(timestamp);
-
- int dayOfWeek = dateTime.getDayOfWeek(); // 1 = Monday, 7 = Sunday
-
- return DayOfTheWeek.fromInt(dayOfWeek);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static DayOfTheWeek getDayOfTheWeekFromTimestamp(String timestamp) { | |
| DateTimeFormatter formatter = DateTimeFormat.forPattern("yyyyMMddHHmmss"); | |
| DateTime dateTime = formatter.parseDateTime(timestamp); | |
| int dayOfWeek = dateTime.getDayOfWeek(); // 1 = Monday, 7 = Sunday | |
| return DayOfTheWeek.fromInt(dayOfWeek); | |
| } | |
| private static final DateTimeFormatter TIMESTAMP_FORMATTER = | |
| DateTimeFormat.forPattern("yyyyMMddHHmmss"); | |
| /** | |
| * Converts a timestamp string to DayOfTheWeek. | |
| * | |
| * @param timestamp String in format "yyyyMMddHHmmss" | |
| * @return DayOfTheWeek corresponding to the timestamp | |
| * @throws IllegalArgumentException if timestamp is null or in invalid format | |
| */ | |
| public static DayOfTheWeek getDayOfTheWeekFromTimestamp(String timestamp) { | |
| if (timestamp == null) { | |
| throw new IllegalArgumentException("Timestamp cannot be null"); | |
| } | |
| try { | |
| DateTime dateTime = TIMESTAMP_FORMATTER.parseDateTime(timestamp); | |
| return DayOfTheWeek.fromInt(dateTime.getDayOfWeek()); | |
| } catch (IllegalArgumentException e) { | |
| throw new IllegalArgumentException( | |
| "Invalid timestamp format. Expected: yyyyMMddHHmmss", e); | |
| } | |
| } |
…n update from frch for lmp
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriberServiceImpl.java (1)
436-436: Remove duplicate deleteCallRetry call.This line duplicates the deleteCallRetry logic already handled in lines 420-428. It will delete retry records even when only MSISDN changed (not LMP), which contradicts the intended logic.
- if(subscription != null){subscriptionService.deleteCallRetry(subscription.getSubscriptionId());}
🧹 Nitpick comments (3)
api/src/main/java/org/motechproject/nms/api/web/KilkariController.java (1)
402-411: Consider improving CSV parsing robustness.The current implementation uses simple comma splitting which won't handle quoted values containing commas or other CSV edge cases.
Consider using a proper CSV parsing library like Apache Commons CSV or OpenCSV for more robust parsing:
- private List<String[]> readFileContent(BufferedReader reader) throws IOException { - List<String[]> records = new ArrayList<>(); - String line; - while ((line = reader.readLine()) != null) { - if (!line.trim().isEmpty()) { - records.add(line.split(",")); - } - } - return records; - } + private List<String[]> readFileContent(BufferedReader reader) throws IOException { + // Consider using Apache Commons CSV for robust parsing + List<String[]> records = new ArrayList<>(); + String line; + while ((line = reader.readLine()) != null) { + if (!line.trim().isEmpty()) { + // TODO: Handle quoted values, escaped commas, etc. + records.add(line.split(",", -1)); // -1 to preserve empty trailing fields + } + } + return records; + }kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriberServiceImpl.java (2)
413-422: Remove or adjust debug logging statements.These info-level logs appear to be temporary debugging statements. They should either be removed or changed to debug level to avoid cluttering production logs.
- LOGGER.info("we are inside not null condition"); - LOGGER.info("1st value: {}",subscriberByRchId.getLastMenstrualPeriod().getDayOfYear()); - LOGGER.info("second value: {}",lmp.getDayOfYear()); - LOGGER.info("third value: {}",subscriberByRchId.getLastMenstrualPeriod().getYear()); - LOGGER.info("second value: {}",lmp.getYear()); - LOGGER.info("this is the first case: {}",(subscriberByRchId.getLastMenstrualPeriod().getDayOfYear() == lmp.getDayOfYear())); - LOGGER.info("this is the second case2: {}",(subscriberByRchId.getLastMenstrualPeriod().getYear() == lmp.getYear())); + LOGGER.debug("Checking LMP date change - old: {}, new: {}", + subscriberByRchId.getLastMenstrualPeriod(), lmp);
1235-1243: Consider more specific exception handling.The catch block catches all exceptions, which might mask specific issues. Consider catching and handling specific exceptions differently.
- } catch (Exception e) { - failureMessages.add("Failed to process msisdn " + record[0] + ": " + e.getMessage()); + } catch (IllegalArgumentException e) { + failureMessages.add("Invalid data for msisdn " + record[0] + ": " + e.getMessage()); + } catch (IllegalStateException e) { + failureMessages.add("Failed to deactivate msisdn " + record[0] + ": " + e.getMessage()); + } catch (Exception e) { + LOGGER.error("Unexpected error processing msisdn " + record[0], e); + failureMessages.add("Unexpected error for msisdn " + record[0] + ": " + e.getMessage());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/src/main/java/org/motechproject/nms/api/web/BaseController.java(1 hunks)api/src/main/java/org/motechproject/nms/api/web/KilkariController.java(4 hunks)imi/src/main/resources/imi.properties(1 hunks)kilkari/src/main/java/org/motechproject/nms/kilkari/service/SubscriberService.java(2 hunks)kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriberServiceImpl.java(7 hunks)rch/src/main/java/org/motechproject/nms/rch/service/impl/RchWebServiceFacadeImpl.java(1 hunks)rch/src/main/resources/rch.properties(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- imi/src/main/resources/imi.properties
- rch/src/main/resources/rch.properties
- rch/src/main/java/org/motechproject/nms/rch/service/impl/RchWebServiceFacadeImpl.java
🔇 Additional comments (6)
api/src/main/java/org/motechproject/nms/api/web/BaseController.java (1)
390-390: Good security improvement!Replacing the actual exception message with a generic error message prevents potential information leakage about internal implementation details.
kilkari/src/main/java/org/motechproject/nms/kilkari/service/SubscriberService.java (1)
17-17: LGTM! Well-designed method signature for bulk processing.The method signature is well-designed with thread-safe
AtomicIntegerfor success counting and a list for collecting failure messages, suitable for concurrent processing.Also applies to: 121-121
kilkari/src/main/java/org/motechproject/nms/kilkari/service/impl/SubscriberServiceImpl.java (4)
412-428: Critical fix for preventing duplicate OBD records!This logic correctly handles call retry records when LMP dates change, which addresses the core issue mentioned in the PR objective. When the LMP date changes, existing call retry records are deleted to prevent duplicate OBD entries. When only the MSISDN changes, the retry records are updated.
752-759: Good implementation of call retry handling for child records.The logic correctly handles call retry records based on DOB changes, consistent with the mother subscriber handling.
1011-1018: Excellent logic to prevent conflicting subscriptions!The checks for active child subscriptions before reactivating mother subscriptions prevent data inconsistencies and potential duplicate records. This is a crucial fix for maintaining data integrity.
Also applies to: 1036-1046
1133-1133: Good addition of transactional boundary.Adding
@Transactionalensures atomicity for the entire deactivation process, preventing partial updates in case of failures.
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | ||
| List<Future<Void>> futures = new ArrayList<>(); | ||
|
|
||
| for (int i = 0; i < threadCount; i++) { | ||
| int start = i * chunkSize; | ||
| int end = Math.min(start + chunkSize, records.size()); | ||
| if (start >= records.size()) {break;} | ||
| List<String[]> chunk = records.subList(start, end); | ||
|
|
||
| Future<Void> future = executor.submit(new Callable<Void>() { | ||
| @Override | ||
| public Void call() throws Exception { | ||
| subscriberService.processChunkOfDeactivation(chunk, successCount, failureMessages); | ||
| return null; | ||
| } | ||
| }); | ||
| futures.add(future); | ||
| } | ||
|
|
||
| for (Future<Void> future : futures) { | ||
| future.get(); | ||
| } | ||
| executor.shutdown(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper executor shutdown and error handling.
The executor service needs better resource management and error handling:
- The executor should be shut down in a finally block or use try-with-resources
- Add timeout for executor termination
Apply this diff to improve executor management:
- ExecutorService executor = Executors.newFixedThreadPool(threadCount);
- List<Future<Void>> futures = new ArrayList<>();
-
- for (int i = 0; i < threadCount; i++) {
- int start = i * chunkSize;
- int end = Math.min(start + chunkSize, records.size());
- if (start >= records.size()) {break;}
- List<String[]> chunk = records.subList(start, end);
-
- Future<Void> future = executor.submit(new Callable<Void>() {
- @Override
- public Void call() throws Exception {
- subscriberService.processChunkOfDeactivation(chunk, successCount, failureMessages);
- return null;
- }
- });
- futures.add(future);
- }
-
- for (Future<Void> future : futures) {
- future.get();
- }
- executor.shutdown();
+ ExecutorService executor = Executors.newFixedThreadPool(threadCount);
+ try {
+ List<Future<Void>> futures = new ArrayList<>();
+
+ for (int i = 0; i < threadCount; i++) {
+ int start = i * chunkSize;
+ int end = Math.min(start + chunkSize, records.size());
+ if (start >= records.size()) {break;}
+ List<String[]> chunk = records.subList(start, end);
+
+ Future<Void> future = executor.submit(new Callable<Void>() {
+ @Override
+ public Void call() throws Exception {
+ subscriberService.processChunkOfDeactivation(chunk, successCount, failureMessages);
+ return null;
+ }
+ });
+ futures.add(future);
+ }
+
+ for (Future<Void> future : futures) {
+ future.get();
+ }
+ } finally {
+ executor.shutdown();
+ try {
+ if (!executor.awaitTermination(60, TimeUnit.SECONDS)) {
+ executor.shutdownNow();
+ }
+ } catch (InterruptedException e) {
+ executor.shutdownNow();
+ Thread.currentThread().interrupt();
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | |
| List<Future<Void>> futures = new ArrayList<>(); | |
| for (int i = 0; i < threadCount; i++) { | |
| int start = i * chunkSize; | |
| int end = Math.min(start + chunkSize, records.size()); | |
| if (start >= records.size()) {break;} | |
| List<String[]> chunk = records.subList(start, end); | |
| Future<Void> future = executor.submit(new Callable<Void>() { | |
| @Override | |
| public Void call() throws Exception { | |
| subscriberService.processChunkOfDeactivation(chunk, successCount, failureMessages); | |
| return null; | |
| } | |
| }); | |
| futures.add(future); | |
| } | |
| for (Future<Void> future : futures) { | |
| future.get(); | |
| } | |
| executor.shutdown(); | |
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | |
| try { | |
| List<Future<Void>> futures = new ArrayList<>(); | |
| for (int i = 0; i < threadCount; i++) { | |
| int start = i * chunkSize; | |
| int end = Math.min(start + chunkSize, records.size()); | |
| if (start >= records.size()) { break; } | |
| List<String[]> chunk = records.subList(start, end); | |
| Future<Void> future = executor.submit(new Callable<Void>() { | |
| @Override | |
| public Void call() throws Exception { | |
| subscriberService.processChunkOfDeactivation(chunk, successCount, failureMessages); | |
| return null; | |
| } | |
| }); | |
| futures.add(future); | |
| } | |
| for (Future<Void> future : futures) { | |
| future.get(); | |
| } | |
| } finally { | |
| executor.shutdown(); | |
| try { | |
| if (!executor.awaitTermination(60, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); | |
| } | |
| } catch (InterruptedException e) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In api/src/main/java/org/motechproject/nms/api/web/KilkariController.java around
lines 368 to 390, the ExecutorService is not properly shut down and lacks error
handling. Refactor the code to shut down the executor in a finally block or use
try-with-resources to ensure it always shuts down. Add a timeout to await
termination after shutdown to avoid indefinite blocking. Also, handle
InterruptedException and ExecutionException when calling future.get() to
properly manage errors during task execution.
Kantas2601
force-pushed
the
Duplicates-in-OBD-file-fix
branch
from
0b09fc3 to
880faef
Compare
Made changes to prevent duplicate records in the OBD file when we receive an update from RCH for the LMP date.
Summary by CodeRabbit
New Features
Improvements