Use a separate ExecutorService to fix #71. #72
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,48 @@ | ||
| package com.monitorjbl.plugins; | ||
|
|
||
| import com.atlassian.bitbucket.util.Version; | ||
| import com.atlassian.bitbucket.util.concurrent.ExecutorUtils; | ||
| import com.atlassian.sal.api.ApplicationProperties; | ||
| import com.atlassian.sal.api.lifecycle.LifecycleAware; | ||
| import com.atlassian.util.concurrent.ThreadFactories; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
|
|
||
| public class AsyncProcessor implements LifecycleAware { | ||
|
There was a problem hiding this comment.
|
||
|
|
||
| private static final Version VERSION_5_9 = new Version(5, 9); | ||
|
|
||
| private final ExecutorService executorService; | ||
| private final boolean ownsExecutor; | ||
|
|
||
| public AsyncProcessor(ApplicationProperties applicationProperties, ExecutorService executorService) { | ||
| ownsExecutor = new Version(applicationProperties.getVersion()).compareTo(VERSION_5_9) < 0; | ||
| if (ownsExecutor) { | ||
| //For Bitbucket Server versions before 5.9, use our own ExecutorService to avoid starving out hook callbacks | ||
| this.executorService = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors(), | ||
| ThreadFactories.namedThreadFactory("pr-harmony", ThreadFactories.Type.DAEMON)); | ||
| } else { | ||
| //Bitbucket Server 5.9 includes a fix for BSERV-10652, which moves hook callback to their own threadpool, so | ||
| //for 5.9+ use the shared executor | ||
| this.executorService = executorService; | ||
|
There was a problem hiding this comment. For 5.9, this allows PR Harmony to continue using our shared There was a problem hiding this comment. Will this not cause thread exhaustion when the server gets a lot of build updates? If the executor wasn't shared, it wouldn't directly interfere with that (although it certainly could interfere with CPU/memory consumption). There was a problem hiding this comment. This is a (difficult) balancing act. Certainly, having a significant number of build status changes could result in PR Harmony creating a lot of tasks for the shared executor. In a system where there are many add-ons using the shared executor, those build status tasks may delay tasks created by other add-ons--just like a large backlog of tasks from other add-ons would be able to delay PR Harmony's build status processing. For Bitbucket Server, though, that's actually desirable, as long as the tasks being delayed are not critical. That's the real "bug" in BSERV-10652; we were using the shared executor for critical processing: Hook callbacks. Having a shared limit on background processing that can be delayed helps to ensure the accumulation of add-ons doesn't overwhelm the system. (That's not to say that PR Harmony's build status processing is not important; clearly it is. But a delay of minutes is less critical than with, say, hosting operations.) Now, it could certainly be contended that if PR Harmony just always used its own executor, it wouldn't starve critical processing, but it also wouldn't starve, or be starved by, other add-ons. From a purely PR Harmony standpoint, that's all good. But from my standpoint, which I'll readily admit is biased, it's less good. The build status processing PR Harmony does is quite heavy, and produces non-trivial system load. When load becomes too high, it's usually not add-on vendors that see the support load, at least initially; it's Bitbucket Server's developers and supporters. If all add-ons create and use their own executors, it's likely some (most? all?) of them will use non-configurable sizes (as I've done here, I'll note, for 5.8 and earlier). But even if they are configurable, that still means a system administrator has to find out what all those configuration properties are and apply them--and they'd need to have some way of readily determining which add-on(s) executor(s) they need to reconfigure based on who's starving (or who's creating too much load). That's not always straightforward to figure out, especially for non-programmers. By comparison, if add-ons use the shared executor, it's a single thing to size. That implies, if tasks are getting backed up, administrators can consider their current system load and opt to add more threads to the shared executor, which would then be "spread" across all the add-ons that use it and allow the backlog to be cleared more quickly. Additionally, we're planning to add monitoring for the queue size on the shared executor in a future release of Bitbucket Server, which will make it easier for administrators to track the size of the backlog on their server. That's another feature add-ons which use their own executors won't benefit from. We may add an SPI that they could use (not sure what the plan is there, at the moment), but they'd still have to manually build it out. |
||
| } | ||
| } | ||
|
|
||
| public void dispatch(Runnable taskRequest) { | ||
| executorService.execute(taskRequest); | ||
| } | ||
|
|
||
| @Override | ||
| public void onStart() { | ||
| //No-op | ||
| } | ||
|
|
||
| @Override | ||
| public void onStop() { | ||
| if (ownsExecutor) { | ||
| ExecutorUtils.shutdown(executorService, LoggerFactory.getLogger(getClass())); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, if you have a
<scope>import</scope>forbitbucket-parent, you shouldn't define explicit versions on your dependencies. Our parent POM defines the versions of the libraries that we actually ship with, so using those versions ensures you're (for example) using the "right" APIs for Bitbucket Server versions you target.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip. The POM was largely copied from Atlassian's example plugin project, so if you haven't already you might want to update that to prevent others from doing this.