You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2022/05/25 14:16:20 UTC
[GitHub] [sling-org-apache-sling-event] stefan-egli commented on a diff in pull request #6: [SLING-8582] provide some test coverage for Eventing's JobHandler
stefan-egli commented on code in PR #6:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/6#discussion_r881710403
##########
src/main/java/org/apache/sling/event/impl/jobs/JobHandler.java:
##########
@@ -78,91 +78,77 @@ public boolean startProcessing(final Queue queue) {
* Reschedule the job
* Update the retry count and remove the started time.
* @return <code>true</code> if rescheduling was successful, <code>false</code> otherwise.
- */
+ */
public boolean reschedule() {
- final ResourceResolver resolver = this.configuration.createResourceResolver();
- try {
- final Resource jobResource = resolver.getResource(job.getResourcePath());
- if ( jobResource != null ) {
- final ModifiableValueMap mvm = jobResource.adaptTo(ModifiableValueMap.class);
- mvm.put(Job.PROPERTY_JOB_RETRY_COUNT, job.getProperty(Job.PROPERTY_JOB_RETRY_COUNT, Integer.class));
- if ( job.getProperty(Job.PROPERTY_RESULT_MESSAGE) != null ) {
- mvm.put(Job.PROPERTY_RESULT_MESSAGE, job.getProperty(Job.PROPERTY_RESULT_MESSAGE));
- }
- mvm.remove(Job.PROPERTY_JOB_STARTED_TIME);
- mvm.put(JobImpl.PROPERTY_JOB_QUEUED, Calendar.getInstance());
- try {
- resolver.commit();
- return true;
- } catch ( final PersistenceException pe ) {
- this.configuration.getMainLogger().debug("Unable to update reschedule properties for job " + job.getId(), pe);
- }
+ return withJobResource((jobResource,mvm) -> {
+ mvm.put(Job.PROPERTY_JOB_RETRY_COUNT, job.getProperty(Job.PROPERTY_JOB_RETRY_COUNT, Integer.class));
+ if ( job.getProperty(Job.PROPERTY_RESULT_MESSAGE) != null ) {
+ mvm.put(Job.PROPERTY_RESULT_MESSAGE, job.getProperty(Job.PROPERTY_RESULT_MESSAGE));
}
- } finally {
- resolver.close();
- }
-
- return false;
+ mvm.remove(Job.PROPERTY_JOB_STARTED_TIME);
+ mvm.put(JobImpl.PROPERTY_JOB_QUEUED, Calendar.getInstance());
+ try {
+ jobResource.getResourceResolver().commit();
+ return true;
+ } catch ( final PersistenceException pe ) {
+ this.configuration.getMainLogger().debug("Unable to update reschedule properties for job " + job.getId(), pe);
+ }
+ return false;
+ });
}
/**
* Finish a job.
* @param state The state of the processing
* @param keepJobInHistory whether to keep the job in the job history.
* @param duration the duration of the processing.
- */
- public void finished(final Job.JobState state,
- final boolean keepJobInHistory,
- final Long duration) {
+ */
+ public void finished(final Job.JobState state, final boolean keepJobInHistory, final Long duration) {
final boolean isSuccess = (state == Job.JobState.SUCCEEDED);
- final ResourceResolver resolver = this.configuration.createResourceResolver();
- try {
- final Resource jobResource = resolver.getResource(job.getResourcePath());
- if ( jobResource != null ) {
- try {
- String newPath = null;
- if ( keepJobInHistory ) {
- final ValueMap vm = ResourceHelper.getValueMap(jobResource);
- newPath = this.configuration.getStoragePath(job.getTopic(), job.getId(), isSuccess);
- final Map<String, Object> props = new HashMap<>(vm);
- props.put(JobImpl.PROPERTY_FINISHED_STATE, state.name());
- if ( isSuccess ) {
- // we set the finish date to start date + duration
- final Date finishDate = new Date();
- finishDate.setTime(job.getProcessingStarted().getTime().getTime() + duration);
- final Calendar finishCal = Calendar.getInstance();
- finishCal.setTime(finishDate);
- props.put(JobImpl.PROPERTY_FINISHED_DATE, finishCal);
- } else {
- // current time is good enough
- props.put(JobImpl.PROPERTY_FINISHED_DATE, Calendar.getInstance());
- }
- if ( job.getProperty(Job.PROPERTY_RESULT_MESSAGE) != null ) {
- props.put(Job.PROPERTY_RESULT_MESSAGE, job.getProperty(Job.PROPERTY_RESULT_MESSAGE));
- }
- ResourceHelper.getOrCreateResource(resolver, newPath, props);
+ withJobResource((jobResource,mvm) -> {
+ try {
+ ResourceResolver rr = jobResource.getResourceResolver();
+ String newPath = null;
+ if (keepJobInHistory) {
+ newPath = this.configuration.getStoragePath(job.getTopic(), job.getId(), isSuccess);
+ final Map<String, Object> props = new HashMap<>(mvm);
+ props.put(JobImpl.PROPERTY_FINISHED_STATE, state.name());
+ if (isSuccess) {
+ // we set the finish date to start date + duration
+ final Date finishDate = new Date();
+ finishDate.setTime(job.getProcessingStarted().getTime().getTime() + duration);
+ final Calendar finishCal = Calendar.getInstance();
+ finishCal.setTime(finishDate);
+ props.put(JobImpl.PROPERTY_FINISHED_DATE, finishCal);
+ } else {
+ // current time is good enough
+ props.put(JobImpl.PROPERTY_FINISHED_DATE, Calendar.getInstance());
+ }
+ if (job.getProperty(Job.PROPERTY_RESULT_MESSAGE) != null) {
+ props.put(Job.PROPERTY_RESULT_MESSAGE, job.getProperty(Job.PROPERTY_RESULT_MESSAGE));
}
- resolver.delete(jobResource);
- resolver.commit();
+ ResourceHelper.getOrCreateResource(rr, newPath, props);
+ }
+ rr.delete(jobResource);
+ rr.commit();
- if ( keepJobInHistory && configuration.getMainLogger().isDebugEnabled() ) {
- if ( isSuccess ) {
- configuration.getMainLogger().debug("Kept successful job {} at {}", Utility.toString(job), newPath);
- } else {
- configuration.getMainLogger().debug("Moved cancelled job {} to {}", Utility.toString(job), newPath);
- }
+ if (keepJobInHistory && configuration.getMainLogger().isDebugEnabled()) {
+ if (isSuccess) {
+ configuration.getMainLogger().debug("Kept successful job {} at {}", Utility.toString(job),
+ newPath);
+ } else {
+ configuration.getMainLogger().debug("Moved cancelled job {} to {}", Utility.toString(job),
+ newPath);
}
- } catch ( final PersistenceException pe ) {
- this.configuration.getMainLogger().warn("Unable to finish job " + job.getId(), pe);
- } catch (final InstantiationException ie) {
- // something happened with the resource in the meantime
- this.configuration.getMainLogger().debug("Unable to instantiate job", ie);
}
+ } catch (final PersistenceException pe) {
+ this.configuration.getMainLogger().warn("Unable to finish job " + job.getId(), pe);
}
- } finally {
- resolver.close();
- }
+ return false;
Review Comment:
IIUC this `return false` is a no-op here, as the return value is not really evaluated by the caller.
Yet I was wondering if there's a particular reason to return `false` rather than `true`? (or should we add a comment that states that "this return value is not used anywhere"?)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org