Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SAM-3170 Convert Timer() thread to ScheduledThreadPoolExecutor() service #4227

Merged
merged 5 commits into from
Jun 2, 2017
Merged

Conversation

dnhutchins
Copy link
Contributor

This replaces the Timer() that runs a single SubmitTimedAssessmentThread at a fixed rate, with a ScheduledThreadPoolExecutor that uses a pool of threads to execute individual TimedAssessmentRunnables at a fixed rate for each timed assessment in progress.

This change adds the property samigo.timerThreadCount with a default value of 4, in case anyone has a need to tune that value.

@dnhutchins dnhutchins changed the title Sam 3170 Convert Timer() thread to ScheduledThreadPoolExecutor() service Apr 10, 2017
@ottenhoff ottenhoff requested a review from ern April 14, 2017 15:40
@dnhutchins dnhutchins changed the title Convert Timer() thread to ScheduledThreadPoolExecutor() service SAM-3170 Convert Timer() thread to ScheduledThreadPoolExecutor() service Apr 17, 2017
@ncaidin ncaidin added the cla? label Apr 18, 2017
log.debug("***2. TimedAssessmentQueue.add, after schedule timer="+timer);
}
// Add grading data to the queue
synchronized (queue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

synchronized on a ConcurentHashMap.put() is not needed as this method is atomic.

tasks.get(timedAG).cancel(true);
tasks.remove(timedAG);
// Remove the grading data from the queue
synchronized (queue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above ConcurrentHashMap.remove() is atomic

@dnhutchins
Copy link
Contributor Author

Removed synchronization from the ConcurrentHashMap put/remove calls.

@ncaidin ncaidin removed the cla? label May 16, 2017
@ncaidin
Copy link
Contributor

ncaidin commented May 16, 2017

CLA is on file. Removed cla? label.

@jonespm
Copy link
Contributor

jonespm commented May 16, 2017

Has one file currently in conflict SubmitTimedAssessmentThread.java

@dnhutchins
Copy link
Contributor Author

dnhutchins commented May 16, 2017

This PR removes that file completely. So I'm a bit confused as to what the conflict is :/

@jonespm
Copy link
Contributor

jonespm commented May 16, 2017

The change was on 59e65c3#diff-50bd1651546e802bd2c1022818829dae in SAM-3012

Looks like the diff was changing a event name. So this would have to still be rebased, and the event name EVENT_ASSESSMENT_SUBMITTED_TIMED should be renamed to EVENT_ASSESSMENT_SUBMITTED_TIMER_THREAD

-            EventTrackingService.post(EventTrackingService.newEvent(SamigoConstants.EVENT_ASSESSMENT_SUBMITTED_TIMED, notiValues.toString(), siteId, true, SamigoConstants.NOTI_EVENT_ASSESSMENT_TIMED_SUBMITTED));
+            EventTrackingService.post(EventTrackingService.newEvent(SamigoConstants.EVENT_ASSESSMENT_SUBMITTED_TIMER_THREAD, notiValues.toString(), siteId, true, SamigoConstants.NOTI_EVENT_ASSESSMENT_TIMED_SUBMITTED))

@dnhutchins
Copy link
Contributor Author

I synced this PR with master, and made the appropriate changes to the new constant.

@ottenhoff ottenhoff merged commit d11b7bf into sakaiproject:master Jun 2, 2017
@dnhutchins dnhutchins deleted the SAM-3170 branch June 2, 2017 03:23
jonespm pushed a commit to jonespm/sakai that referenced this pull request Jun 24, 2017
bjones86 pushed a commit to Western-OWL/sakai that referenced this pull request Aug 22, 2017
jonespm pushed a commit to LongsightGroup/sakai that referenced this pull request Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants