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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
