[GitHub] [incubator-druid] gianm commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks

[GitBox <gi...@apache.org>]

gianm commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
URL: https://github.com/apache/incubator-druid/pull/8236#discussion_r311285212
 
 

 ##########
 File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java
 ##########
 @@ -146,10 +146,13 @@
   private final RetryPolicyFactory retryPolicyFactory;
 
   @JsonIgnore
-  private List<IndexTask> indexTaskSpecs;
+  private final AppenderatorsManager appenderatorsManager;
 
   @JsonIgnore
-  private AppenderatorsManager appenderatorsManager;
+  private List<IndexTask> indexTaskSpecs;
+
+  @Nullable
+  private volatile IndexTask currentRunningTaskSpec = null;
 
 Review comment:
   In this particular API, `stopGracefully` can be called at any point during `run`. In response, the Task should "arrange for its 'run' method to exit promptly". So I think the `volatile` is needed for safe publication. To me a comment like this would explain it sufficiently.
   
   > The sub-task that is currently running. Volatile since it will potentially be accessed by `stopGracefully` concurrently with `runTask`, which is responsible for assigning the value.
   
   Side note, I don't think this applies here, but in general do you think it's a valid explanation to say: "this field is accessed by multiple threads, and I haven't conclusively proven that volatile is _not_ necessary, so I've included it"?
   
   Probably a lot of the volatiles in the codebase today are like this. I don't necessarily see a big problem with it. Even though some of them could probably be safely removed, the 'just-in-case' volatiles still help lower the cognitive overhead of documenting and dealing with the codebase. (Otherwise, you'd need to think through these arguments again every time you make a change.) And in most cases, including this one, there is negligible performance cost.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org