[GitHub] [fineract] ptuomola commented on a change in pull request #1886: Feat: Node aware job scheduler

[GitBox <gi...@apache.org>]

ptuomola commented on a change in pull request #1886:
URL: https://github.com/apache/fineract/pull/1886#discussion_r725401869



##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/JobRegisterServiceImpl.java
##########
@@ -125,6 +132,7 @@ public void loadAllJobs() {
             ThreadLocalContextUtil.setTenant(tenant);
             final List<ScheduledJobDetail> scheduledJobDetails = this.schedularWritePlatformService.retrieveAllJobs();
             for (final ScheduledJobDetail jobDetails : scheduledJobDetails) {
+                readAndSetSchedulerNode();
                 scheduleJob(jobDetails);

Review comment:
       I don't think we need to read this variable again for every job... it should not change, right?

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/JobRegisterServiceImpl.java
##########
@@ -125,6 +132,7 @@ public void loadAllJobs() {
             ThreadLocalContextUtil.setTenant(tenant);
             final List<ScheduledJobDetail> scheduledJobDetails = this.schedularWritePlatformService.retrieveAllJobs();

Review comment:
       Would it not make more sense just to retrieve all jobs meant for this node (or for any node)? Then you would not need any more logic later on for dirty jobs etc - as the list would already be correct. 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/domain/ScheduledJobDetail.java
##########
@@ -135,6 +141,14 @@ public void updateJobKey(final String jobKey) {
         this.jobKey = jobKey;
     }
 
+    public boolean isDirtyJob() {
+        return this.isDirtyJob;

Review comment:
       Unless I'm missing something, the "dirty" flag is used to mark a job that is intended to be run on another node - correct? If yes, can we name the variable something more descriptive so that that is obvious? 
   
   IMHO dirty is not a good name for this, as dirty normally means e.g. entries that have been modified in memory but not yet written to persistent storage - and that's not the case here.
   
   Or do we even need a new flag for this? Why not just compare nodeId of job with nodeId of node, and determine this as required? 

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/JobRegisterServiceImpl.java
##########
@@ -286,6 +314,29 @@ private void scheduleJob(final ScheduledJobDetail scheduledJobDetails) {
         scheduledJobDetails.updateCurrentlyRunningStatus(false);
     }
 
+    private void readAndSetSchedulerNode() {
+        Properties prop = new Properties();

Review comment:
       Instead of doing this "manually", why not using the Spring Boot capabilities for reading configuration values. E.g. @Value annotation or @Autowiring environment?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org