Code cleanup around Pollers #1070
Conversation
Spikhalskiy
requested review from
mfateev,
Sushisource,
cretz,
bergundy and
mmcshane
as code owners
| @@ -110,6 +110,7 @@ public ActivityTask poll() { | |||
| ProtobufTimeUtils.toM3Duration( | |||
| response.getStartedTime(), response.getCurrentAttemptScheduledTime())); | |||
| isSuccessful = true; | |||
| return new ActivityTask(response, pollSemaphore::release); | |||
There was a problem hiding this comment.
Paired with
if (!isSuccessful) pollSemaphore.release();
in finally, this location of return is cleaner for the reader.
| @@ -168,15 +168,18 @@ public void awaitTermination(long timeout, TimeUnit unit) { | |||
|
|
|||
| @Override | |||
| public void suspendPolling() { | |||
| log.info("suspendPolling: {}", this); | |||
| suspendLatch.set(new CountDownLatch(1)); | |||
There was a problem hiding this comment.
Absense of CAS here may replace and lose the latch which is already used by Pollers to block on. This way they will never be unblocked if suspentPolling was called twice.
| if (pollerOptions == null) { | ||
| pollerOptions = | ||
| PollerOptions.newBuilder() | ||
| .setPollBackoffInitialInterval(Duration.ofMillis(200)) |
There was a problem hiding this comment.
This code path is dead. We come here only in some tests where we do
SingleWorkerOptions.newBuilder().build()
and these options and not important for these tests. As a result, these magic values 200 and 20 here are never used and better be removed from the production code paths to don't mislead the reader.
After this cleanup, it's actually clear that we never call PollerOptions#setPollBackoffMaximumInterval anywhere.
| @@ -181,7 +181,7 @@ private static SingleWorkerOptions toLocalActivityOptions( | |||
| List<ContextPropagator> contextPropagators, | |||
| Scope metricsScope) { | |||
| return toSingleWorkerOptions(factoryOptions, options, clientOptions, contextPropagators) | |||
| .setPollerOptions(PollerOptions.newBuilder().build()) | |||
There was a problem hiding this comment.
ActivityWorker and WorkflowWorker PollerOptions specify this parameter explicitly.
To simplify the untangling for the reader, let's specify it for LocalActivityWorker explicitly too. 1 is the default value for pollThreadCount used inside PollerOptions.
Fix for potential pollers deadlock if suspendPolling called twice.
Refactorings that make the code around Polling easier to follow.