You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/09/13 16:12:05 UTC
[GitHub] [druid] kfaraz commented on a diff in pull request #13072: Faster fix for dangling tasks upon supervisor termination
kfaraz commented on code in PR #13072:
URL: https://github.com/apache/druid/pull/13072#discussion_r969688470
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -128,6 +128,7 @@
import java.util.stream.Stream;
/**
+ *
Review Comment:
Nit: Extra newline?
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -1486,6 +1489,15 @@ public void runInternal()
}
}
+ private void generateReport()
Review Comment:
Nit: Rename to `logReport()` or `generateAndLogReport()` as the existing `generateReport()` method also returns the generated report. We shouldn't overload that method if the behaviour is different from it.
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorStateManager.java:
##########
@@ -120,10 +120,18 @@ public SupervisorStateManager(SupervisorStateManagerConfig supervisorStateManage
/**
* Certain states are only valid if the supervisor hasn't had a successful iteration. This method checks if there's
- * been at least one successful iteration, and if applicable sets supervisor state to an appropriate new state.
+ * been at least one successful iteration, and if applicable, sets supervisor state to an appropriate new state.
+ * A STOPPING supervisor must not transition to any other state.
+ * (It is used to prevent a deadlock due to lock contention in SeekableStreamSupervisor#runInternal)
+ * This method is synchronized since multiple threads may be calling it and the above condition needs to be enforced.
Review Comment:
```suggestion
* A STOPPING supervisor cannot transition to any other state as this state is final.
* This method must be thread-safe as multiple threads trying to update may lead to an invalid state.
```
Nit: We are doing the synchronization here to avoid an invalid state of this class.
Avoiding deadlocks is a responsibility of the caller, and not this class itself.
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorStateManager.java:
##########
@@ -120,10 +120,18 @@ public SupervisorStateManager(SupervisorStateManagerConfig supervisorStateManage
/**
* Certain states are only valid if the supervisor hasn't had a successful iteration. This method checks if there's
- * been at least one successful iteration, and if applicable sets supervisor state to an appropriate new state.
+ * been at least one successful iteration, and if applicable, sets supervisor state to an appropriate new state.
+ * A STOPPING supervisor must not transition to any other state.
+ * (It is used to prevent a deadlock due to lock contention in SeekableStreamSupervisor#runInternal)
+ * This method is synchronized since multiple threads may be calling it and the above condition needs to be enforced.
*/
- public void maybeSetState(State proposedState)
+ public synchronized void maybeSetState(State proposedState)
{
+ // Steady states can be achieved after remove with create
Review Comment:
This comment seems vague. Please clarify or remove this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org