[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r310723222 ## 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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List indexTaskSpecs; + + @Nullable + private volatile IndexTask currentRunningTaskSpec = null; Review comment: https://github.com/code-review-checklists/java-concurrency#justify-volatile 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r311040270 ## 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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List indexTaskSpecs; + + @Nullable + private volatile IndexTask currentRunningTaskSpec = null; Review comment: The added Javadoc doesn't answer the question "Why the semantics of volatile field reads and writes (as defined in the Java Memory Model) are required for the field?" from the referenced checklist item. It's actually likely that just adding `volatile` to the field is either not enough to achieve certain concurrency goal (which is also not clarified in the Javadoc), or not needed. "by an HTTP thread when {@link #stopGracefully} is called." - please point to a specific class and method where does this happen, because it's not obvious. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r311040270 ## 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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List indexTaskSpecs; + + @Nullable + private volatile IndexTask currentRunningTaskSpec = null; Review comment: The added Javadoc doesn't answer the question "Why the semantics of volatile field reads and writes (as defined in the Java Memory Model) are required for the field?" from the referenced checklist item. It's actually likely that just adding `volatile` to the field is either not enough to achieve certain concurrency goal (which is also not clarified in the Javadoc), or not needed. "by an HTTP thread when `{@link #stopGracefully}` is called." - please point to a specific class and method where does this happen, because it's not obvious. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r311232377 ## 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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List indexTaskSpecs; + + @Nullable + private volatile IndexTask currentRunningTaskSpec = null; Review comment: > Do you want me to use the terms defined in the JMM? Yes, I think the term "happens-before" should usually appear in this type of explanatory comments. > So do you think volatile here is necessary or not? I don't know because I wasn't able to figure out the place you referred to in "by an HTTP thread when `{@link #stopGracefully}` is called." 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r311237120 ## 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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List indexTaskSpecs; + + @Nullable + private volatile IndexTask currentRunningTaskSpec = null; Review comment: But first of all, "This variable is updated by the main thread and read by an HTTP thread when `{@link #stopGracefully}` is called." doesn't explain the concurrency model in enough detail. Can `{@link #stopGracefully}` be called fully concurrently at whatever moment in parallel with `runTask()`, or it could be called only in response to some failure within `runTask()`, just from a different thread? In the first case, `volatile` is likely needed to ensure *safe publication* (see JCIP 3.5, or [here](https://shipilev.net/blog/2014/safe-public-construction/#_safe_publication)) of the `IndexTask` object. In the second case, volatile is almost definitely redundant, if the thread from which `stopGracefully()` is called "gets informed" about the failure in `runTask()` in any way that is different from something like a spin-loop read of a non-volatile field (yes, this is very obscure and never actually happens in practice). 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r311608563 ## 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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List indexTaskSpecs; + + @Nullable + private volatile IndexTask currentRunningTaskSpec = null; Review comment: So if `stopGracefully()` may be called at any time concurrently with `run()`, I think the concurrency property that is worth ensuring is not starting running the next task if `stopGracefully()` is already stopping the previous task. To achieve this, `volatile` is not enough. This property could be achieved the following way: - `currentRunningTaskSpec` is `AtomicReference` - The stopping callback: ```java Object currentRunningTask = currentRunningTaskSpec.getAndSet(SPECIAL_VALUE_STOPPED); if (currentRunningTask != null) { ((IndexTask) currentRunningTask).stopGracefull(config); } ``` - The code in the loop in `runTask()` does something like ```java Object prevSpec = currentRunningTaskSpec.get(); if (prevTask == SPECIAL_VALUE_STOPPED || !currentRunningTaskSpec.compareAndSet(prevSpec, eachSpec)) { log.info("Stopped concurrently"); ... } ``` 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r311608563 ## 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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List indexTaskSpecs; + + @Nullable + private volatile IndexTask currentRunningTaskSpec = null; Review comment: So if `stopGracefully()` may be called at any time concurrently with `run()`, I think the concurrency property that is worth ensuring is not starting running the next task if `stopGracefully()` is already stopping the previous task. To achieve this, `volatile` is not enough. This property could be achieved the following way: - `currentRunningTaskSpec` is `AtomicReference` - The stopping callback: ```java Object currentRunningTask = currentRunningTaskSpec.getAndSet(SPECIAL_VALUE_STOPPED); if (currentRunningTask != null) { ((IndexTask) currentRunningTask).stopGracefull(config); } ``` - The code in the loop in `runTask()` does something like ```java Object prevSpec = currentRunningTaskSpec.get(); if (prevSpec == SPECIAL_VALUE_STOPPED || !currentRunningTaskSpec.compareAndSet(prevSpec, eachSpec)) { log.info("Stopped concurrently"); ... } ``` 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List 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 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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 indexTaskSpecs; + private final AppenderatorsManager appenderatorsManager; @JsonIgnore - private AppenderatorsManager appenderatorsManager; + private List 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r312194391 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java ## @@ -162,15 +162,21 @@ default int getPriority() Review comment: Could you please add an overview section to `Task`'s class-level Javadoc, describing which methods of this interface could be called concurrently with which other methods and which could not? In other words, provide an overview [concurrent access documentation](https://github.com/code-review-checklists/java-concurrency#justify-document) for this interface. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r312195277 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java ## @@ -162,15 +162,21 @@ default int getPriority() /** * Asks a task to arrange for its "run" method to exit promptly. Tasks that take too long to stop gracefully will be - * terminated with extreme prejudice. Note that this method can be called at any time while {@link #run} is called. - * Its implementations should handle potential concurreny issues properly. + * terminated with extreme prejudice. + * + * This method can be called at any time while {@link #run} is being called when the task is killed. Review comment: Could you specify what should or should not happen if `stopGracefull()` is called concurrently (e. g. "before") `run()`? Does it guarantee that the task won't even start? 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r312556516 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java ## @@ -164,7 +164,10 @@ default int getPriority() * Asks a task to arrange for its "run" method to exit promptly. Tasks that take too long to stop gracefully will be * terminated with extreme prejudice. * - * This method can be called at any time while {@link #run} is being called when the task is killed. + * This method can be called at any time while {@link #run} is being called when the task is killed. If this task + * is not started yet, that is {@link #run} is not called yet, this method will be never called. + * Once this task is started, this method can be called even after {@link #run} returns. Implementations of this + * method may want to avoid unnecessary work if {@link #run} already returned. * Depending on the task executor type, one of the two cases below can happen when the task is killed. Review comment: It would be clearer if there was an empty line above this line and no empty line below this line. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r312558173 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java ## @@ -164,7 +164,10 @@ default int getPriority() * Asks a task to arrange for its "run" method to exit promptly. Tasks that take too long to stop gracefully will be * terminated with extreme prejudice. * - * This method can be called at any time while {@link #run} is being called when the task is killed. + * This method can be called at any time while {@link #run} is being called when the task is killed. If this task + * is not started yet, that is {@link #run} is not called yet, this method will be never called. Review comment: This text sort of doesn't make sense, because the words like "yet", "before", "after" are not definable in terms of JMM which doesn't deal with time. Yet, in practical terms, currently, `stopGracefully()` *can* be called "before" `run()` in `CompactionTask` on one of the eachSpecs. So this contract is violated. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r313083513 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java ## @@ -164,7 +164,10 @@ default int getPriority() * Asks a task to arrange for its "run" method to exit promptly. Tasks that take too long to stop gracefully will be * terminated with extreme prejudice. * - * This method can be called at any time while {@link #run} is being called when the task is killed. + * This method can be called at any time while {@link #run} is being called when the task is killed. If this task + * is not started yet, that is {@link #run} is not called yet, this method will be never called. Review comment: > I think the contract now should say "this method can be called at any time no matter when run() is executed". I agree. > I don't know what it should be to address [your comment](https://github.com/apache/incubator-druid/pull/8236#discussion_r312195277). What is your suggestion? I would just put the phrase "Regardless when `stopGracefully()` is called w. r. t. `run()`, the implementation must not allow a resource leak or lingering executions (local or remote)." 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] leventov commented on a change in pull request #8236: Add TaskResourceCleaner; fix a couple of concurrency bugs in batch tasks
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_r313113092 ## File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java ## @@ -164,7 +164,10 @@ default int getPriority() * Asks a task to arrange for its "run" method to exit promptly. Tasks that take too long to stop gracefully will be * terminated with extreme prejudice. * - * This method can be called at any time while {@link #run} is being called when the task is killed. + * This method can be called at any time while {@link #run} is being called when the task is killed. If this task + * is not started yet, that is {@link #run} is not called yet, this method will be never called. Review comment: Missed the first sentence: "this method can be called at any time no matter when run() is executed". Other than that, I don't have more comments about this PR. I didn't review it in full but don't block it. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org