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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
