leventov 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_r311659574
########## 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: > 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.) To me, the main problem is that `volatile` is often used to delude oneself or reviewers that some code is concurrently safe without forcing through the threading/concurrent control flow models of the code, the desired properties of the code, and the proof that the code has the desired properties. If "`volatile` = probably worse perf, but no concurrency defects" was the case, that wouldn't be too bad, indeed. Unfortunately, in reality, what we have is "`volatile` = high probability that there are some concurrency defects around the code". This is why https://github.com/code-review-checklists/java-concurrency#justify-volatile demands to justify `volatile` in JMM terms rather than in vague phrases. ---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
