Added failed connection logic in generate demand Bulk topic #884
Conversation
WalkthroughRecent changes modernize the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (6 hunks)
- municipal-services/ws-calculator/src/main/resources/application.properties (1 hunks)
Additional comments not posted (12)
municipal-services/ws-calculator/src/main/resources/application.properties (3)
163-163: LGTM!The
spring.kafka.consumer.session.timeout.msproperty is correctly set to 30000.
164-164: LGTM!The
spring.kafka.consumer.heartbeat.interval.msproperty is correctly set to 10000.
165-165: LGTM!The
spring.kafka.consumer.max.poll.interval.msproperty is correctly set to 600000.municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (9)
277-277: LGTM!The commented-out code using
Calendarhas been replaced with a more modern and clear approach usingLocalDateandLocalDateTime.
Line range hint
305-315: LGTM!The commented-out code for checking demand and connection existence in the previous billing cycle has been removed, simplifying the logic.
610-611: LGTM!The
DateTimeFormatteris correctly defined for parsing dates.
613-614: LGTM!The
LocalDateinstances forfromDateandtoDateare correctly parsed from thebillingCycle.
616-621: LGTM!The
LocalDateTimeinstances fordayStartTimeanddayEndTimeare correctly calculated.
633-647: LGTM!The logic for checking demand and connection existence and generating demand has been improved and is clear.
580-587: LGTM!The detailed logging statements enhance traceability and debugging.
588-590: LGTM!The
updateAddPenaltymethod correctly processes theDemandRequestand updates the penalty.
Line range hint
591-600: LGTM!The added check for excluded tenants adds robustness to the demand generation process.
| Calendar previousFromDate = Calendar.getInstance(); | ||
| Calendar previousToDate = Calendar.getInstance(); | ||
|
|
||
| previousFromDate.setTimeInMillis(dayStartTime); | ||
| previousToDate.setTimeInMillis(dayEndTime); | ||
|
|
||
| previousFromDate.add(Calendar.MONTH, -1); //assuming billing cycle will be first day of month | ||
| previousToDate.add(Calendar.MONTH, -1); | ||
| int max = previousToDate.getActualMaximum(Calendar.DAY_OF_MONTH); | ||
| previousToDate.set(Calendar.DAY_OF_MONTH, max); |
There was a problem hiding this comment.
Consider removing redundant code.
The Calendar instances for previousFromDate and previousToDate are still being used, which seems redundant given the new LocalDate approach.
- Calendar previousFromDate = Calendar.getInstance();
- Calendar previousToDate = Calendar.getInstance();
- previousFromDate.setTimeInMillis(dayStartTime);
- previousToDate.setTimeInMillis(dayEndTime);
- previousFromDate.add(Calendar.MONTH, -1); //assuming billing cycle will be first day of month
- previousToDate.add(Calendar.MONTH, -1);
- int max = previousToDate.getActualMaximum(Calendar.DAY_OF_MONTH);
- previousToDate.set(Calendar.DAY_OF_MONTH, max);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.
| Calendar previousFromDate = Calendar.getInstance(); | |
| Calendar previousToDate = Calendar.getInstance(); | |
| previousFromDate.setTimeInMillis(dayStartTime); | |
| previousToDate.setTimeInMillis(dayEndTime); | |
| previousFromDate.add(Calendar.MONTH, -1); //assuming billing cycle will be first day of month | |
| previousToDate.add(Calendar.MONTH, -1); | |
| int max = previousToDate.getActualMaximum(Calendar.DAY_OF_MONTH); | |
| previousToDate.set(Calendar.DAY_OF_MONTH, max); |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- municipal-services/ws-calculator/src/main/resources/application.properties (1 hunks)
Files skipped from review due to trivial changes (1)
- municipal-services/ws-calculator/src/main/resources/application.properties
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (5)
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (5)
272-274: Consider refining logging messages.The logging message for the time taken to get connections could be more descriptive. Consider including the tenant ID or other context to make logs more informative.
- log.info("Time taken for the for Get Connetions : "+(System.currentTimeMillis()-StartTimeForGetConnetion)/1000+ " Secondss"); + log.info("Time taken to retrieve connections for tenantId {}: {} seconds", tenantId, (System.currentTimeMillis()-StartTimeForGetConnetion)/1000);
290-294: Clarify logging message for MDMS data retrieval.The log message for MDMS data retrieval time could include additional context, such as the tenant ID.
- log.info("Time taken for the for Get Mdms Data : "+(System.currentTimeMillis()-startTimeForMdms)/1000+ " Secondss"); + log.info("Time taken to retrieve MDMS data for tenantId {}: {} seconds", tenantId, (System.currentTimeMillis()-startTimeForMdms)/1000);
Line range hint
297-335:
Optimize loop logging.The log message for the loop execution time could be more descriptive by including the number of connections processed.
- log.info("Time taken for the for loop : "+(System.currentTimeMillis()-startTimeForLoop)/1000+ " Secondss"); + log.info("Time taken to process {} connections: {} seconds", connectionNos.size(), (System.currentTimeMillis()-startTimeForLoop)/1000);
371-371: Clarify notification logging message.The log message for notification time could include additional context, such as the tenant ID or the number of notifications sent.
- log.info("Time taken for notification : "+(System.currentTimeMillis()-starttimeforNotification)/1000+ " Secondss"); + log.info("Time taken to send notifications for tenantId {}: {} seconds", tenantId, (System.currentTimeMillis()-starttimeforNotification)/1000);
585-592: Enhance logging for demand generation.The log messages for demand generation could be more informative by including additional context, such as the number of demands generated.
- log.info("CALL FROM TOPIC egov.generate.bulk.demand.manually.topic for tenantid:" + bulkDemand.getTenantId() + " BillPeriod:" + billingPeriod + " Start Time:" + System.currentTimeMillis()); - log.info("time takenn to generate demand for Tenantid:" + bulkDemand.getTenantId() + " BillPeriod:" + billingPeriod + " is : " + diff/1000 +" seconds"); + log.info("Bulk demand generation started for tenantId: {}, billingPeriod: {}, startTime: {}", bulkDemand.getTenantId(), billingPeriod, System.currentTimeMillis()); + log.info("Time taken to generate demand for tenantId: {}, billingPeriod: {}: {} seconds", bulkDemand.getTenantId(), billingPeriod, diff/1000);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (8 hunks)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/repository/WSCalculationDaoImpl.java (1 hunks)
Additional comments not posted (3)
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/repository/WSCalculationDaoImpl.java (1)
130-131: Logging Enhancement Approved.The added logging statements for the SQL query and prepared statement parameters will improve traceability and debugging capabilities.
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (2)
Line range hint
278-287:
Remove commented-out code.The code block using the
Calendarclass is commented out. If it's no longer needed, consider removing it to keep the codebase clean.
615-652: Consider removing redundantCalendarusage.The
Calendarinstances forpreviousFromDateandpreviousToDateare redundant given the newLocalDateapproach.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (1)
594-596: Improve Exception Handling inDemandGenerationConsumerThe
DemandGenerationConsumer.javafile contains exception handling that usese.printStackTrace()andSystem.out.println(). These are not suitable for production environments. Consider implementing proper logging and handling mechanisms to ensure robustness and maintainability.
- File:
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java
- Lines with inadequate handling:
e.printStackTrace();System.out.println("Got the exception while generating the demands:" + e);Analysis chain
Verify exception handling comprehensiveness.
Ensure that all potential exceptions are adequately handled to prevent unexpected failures during demand generation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify exception handling in `generateBulkDemandForULB`. # Test: Search for exception handling patterns. Expect: Comprehensive handling of exceptions. rg --type java 'catch \(Exception e\)' -A 3Length of output: 66093
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (8 hunks)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/service/WSCalculationServiceImpl.java (1 hunks)
Files skipped from review due to trivial changes (1)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/service/WSCalculationServiceImpl.java
Additional comments not posted (4)
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (4)
272-274: LGTM: Use of modern date-time API.The transition to
LocalDateandLocalDateTimeimproves code clarity and reduces potential errors associated withCalendar.
Line range hint
278-287:
Remove redundantCalendarcode.The
Calendarinstances are commented out and no longer needed. Consider removing this code to clean up the file.
644-655: LGTM: Logic for checking demands and connections.The logic for verifying the existence of demands and connections is well-implemented and enhances the robustness of the demand generation process.
589-596: LGTM: Logging enhancements.The logging of start and end times for demand generation is beneficial for monitoring and debugging.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (10 hunks)
Additional comments not posted (6)
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (6)
75-76: KafkaTemplate Injection Looks Good.The injection of
KafkaTemplateis appropriate for handling Kafka messages. Ensure that the KafkaTemplate is properly configured in the application context.
276-278: Logging Enhancements Approved.The detailed timing logs enhance the ability to monitor performance and identify bottlenecks.
Line range hint
282-291: Commented-Out Calendar Code Approved.Commenting out the
Calendarcode aligns with the transition toLocalDate. Ensure all relevant code uses the modern date-time API.
338-340: Kafka Message Production Approved.The use of
producer.pushfor Kafka message handling is appropriate. Ensure that messages are validated and errors are handled properly.
374-374: KafkaTemplate Usage for SMS Notifications Approved.The integration of
kafkaTemplatefor sending SMS notifications is appropriate. Ensure the SMS notification topic is correctly configured.
593-600: Logging Enhancements Approved.The detailed logs provide valuable insights into the demand generation process, aiding in debugging and performance monitoring.
| Calendar previousFromDate = Calendar.getInstance(); | ||
| Calendar previousToDate = Calendar.getInstance(); | ||
|
|
||
| previousFromDate.setTimeInMillis(dayStartTime); | ||
| previousToDate.setTimeInMillis(dayEndTime); | ||
|
|
||
| previousFromDate.add(Calendar.MONTH, -1); //assuming billing cycle will be first day of month | ||
| previousToDate.add(Calendar.MONTH, -1); | ||
| int max = previousToDate.getActualMaximum(Calendar.DAY_OF_MONTH); | ||
| previousToDate.set(Calendar.DAY_OF_MONTH, max); |
There was a problem hiding this comment.
Refactor Calendar Usage to LocalDate.
Replace Calendar with LocalDate for consistency and to leverage the modern date-time API.
LocalDate previousFromDate = fromDate.minusMonths(1);
LocalDate previousToDate = toDate.minusMonths(1).with(TemporalAdjusters.lastDayOfMonth());
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- municipal-services/ws-calculator/src/main/resources/application.properties (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- municipal-services/ws-calculator/src/main/resources/application.properties
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (1)
Line range hint
282-291: Remove Commented-Out Calendar Code.The transition to
LocalDateis beneficial, but the commented-outCalendarcode should be removed for code cleanliness.- /* Calendar previousFromDate = Calendar.getInstance(); - Calendar previousToDate = Calendar.getInstance(); - - previousFromDate.setTimeInMillis(dayStartTime); - previousToDate.setTimeInMillis(dayEndTime); - - previousFromDate.add(Calendar.MONTH, -1); //assuming billing cycle will be first day of month - previousToDate.add(Calendar.MONTH, -1); - int max = previousToDate.getActualMaximum(Calendar.DAY_OF_MONTH); - previousToDate.set(Calendar.DAY_OF_MONTH, max);*/
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (10 hunks)
- municipal-services/ws-calculator/src/main/resources/application.properties (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- municipal-services/ws-calculator/src/main/resources/application.properties
Additional comments not posted (6)
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (6)
40-40: KafkaTemplate Import Addition Approved.The addition of
KafkaTemplateis a positive change, leveraging Spring's Kafka support for better message handling.
75-76: KafkaTemplate Field Addition Approved.Introducing
KafkaTemplateas a field is a good practice for managing Kafka message production within the Spring framework.
276-278: Logging Additions Approved.The added logging statements enhance the method's traceability and performance monitoring.
635-644: Refactor Calendar Usage to LocalDate.Replace
CalendarwithLocalDatefor consistency and to leverage the modern date-time API.LocalDate previousFromDate = fromDate.minusMonths(1); LocalDate previousToDate = toDate.minusMonths(1).with(TemporalAdjusters.lastDayOfMonth());
654-659: Logging Additions Approved.The detailed logging for request processing time is beneficial for performance monitoring and debugging.
593-600: Logging Additions Approved.The logging for manual demand generation provides valuable information for tracking and debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
municipal-services/ws-calculator/src/main/resources/application.properties (1)
165-165: Potential Issue:ws.generate.demand.bulktopic not utilized in codebase.The Kafka topic
ws-generate-demand-bulkis defined in theapplication.propertiesfile, but there are no references to it in the Java or Kotlin codebase. Please verify if this topic is used elsewhere or if it is intended for future use.
- Ensure the topic is utilized in the application logic or remove it if unnecessary.
Analysis chain
Configuration approved:
ws.generate.demand.bulk=ws-generate-demand-bulk.Ensure that this Kafka topic is correctly configured and utilized in the application.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `ws.generate.demand.bulk` topic in the codebase. # Test: Search for the topic usage. Expect: Occurrences of the topic in the codebase. rg --type java --type kotlin 'ws-generate-demand-bulk'Length of output: 177
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- municipal-services/ws-calculator/src/main/resources/application.properties (2 hunks)
Additional comments not posted (5)
municipal-services/ws-calculator/src/main/resources/application.properties (5)
25-25: Configuration approved:spring.kafka.producer.linger.ms=10.This setting is appropriate for balancing latency and throughput in Kafka message production.
26-26: Configuration approved:spring.kafka.producer.batch.size=32768.This batch size is reasonable for balancing throughput and resource usage.
27-27: Configuration approved:spring.kafka.producer.buffer.memory=33554432.This buffer size is suitable for managing memory usage and ensuring smooth message production.
28-28: Configuration approved:spring.kafka.consumer.session.timeout.ms=30000.This timeout value is appropriate for ensuring consumer session stability.
29-29: Configuration approved:spring.kafka.consumer.heartbeat.interval.ms=10000.This interval is suitable for maintaining consumer session health and should be less than the session timeout.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (9 hunks)
- municipal-services/ws-calculator/src/main/resources/application.properties (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- municipal-services/ws-calculator/src/main/resources/application.properties
Additional comments not posted (4)
municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/consumer/DemandGenerationConsumer.java (4)
75-77: LGTM!The addition of
KafkaTemplatelooks good and aligns with the PR objectives.
Line range hint
276-294:
LGTM!The use of
LocalDateandLocalDateTimefor date handling is a good improvement over theCalendarAPI.
588-595: LGTM!The logging improvements provide better visibility into the demand generation process.
618-654: LGTM!The method correctly processes messages and updates demands with penalties.
| Calendar previousFromDate = Calendar.getInstance(); | ||
| Calendar previousToDate = Calendar.getInstance(); | ||
|
|
||
| log.info("got generate demand call for :"+calculationReq.getCalculationCriteria().get(0).getConnectionNo()); | ||
| generateDemandInBulk(calculationReq,billingCycle,masterMap,isSendMessage,tenantId); | ||
| previousFromDate.setTimeInMillis(dayStartTime); | ||
| previousToDate.setTimeInMillis(dayEndTime); | ||
|
|
||
| previousFromDate.add(Calendar.MONTH, -1); //assuming billing cycle will be first day of month | ||
| previousToDate.add(Calendar.MONTH, -1); | ||
| int max = previousToDate.getActualMaximum(Calendar.DAY_OF_MONTH); | ||
| previousToDate.set(Calendar.DAY_OF_MONTH, max); |
There was a problem hiding this comment.
Refactor Calendar Usage to LocalDate.
Replace Calendar with LocalDate for consistency and to leverage the modern date-time API.
LocalDate previousFromDate = fromDate.minusMonths(1);
LocalDate previousToDate = toDate.minusMonths(1).with(TemporalAdjusters.lastDayOfMonth());
Summary by CodeRabbit
New Features
Configuration Changes
Bug Fixes