XComp commented on code in PR #24309:
URL: https://github.com/apache/flink/pull/24309#discussion_r1499457482
##########
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/FineGrainedSlotManager.java:
##########
@@ -890,13 +900,32 @@ public long getTaskManagerIdleSince(InstanceID
instanceId) {
//
---------------------------------------------------------------------------------------------
private void checkInit() {
- Preconditions.checkState(started, "The slot manager has not been
started.");
+ assertRunsInMainThreadExecutor();
Preconditions.checkNotNull(resourceManagerId);
- Preconditions.checkNotNull(mainThreadExecutor);
Preconditions.checkNotNull(resourceAllocator);
Preconditions.checkNotNull(resourceEventListener);
}
+ @GuardedBy("lock") // or executed in mainThreadExecutor
+ private boolean isStarted() {
+ return mainThreadExecutor != null;
+ }
Review Comment:
you're right. I missed unsetting the field again.
> [...] but I don't think we fixed the original issue yet?
> We never exit the started state again so all the scheduled executor stuff
still keeps running.
We still fixed FLINK-34427 (partially) because the ScheduledFutures are
cancelled. But you're correct, there are still edge cases where the issue of
FLINK-34427 could have appeared due to missing the unsetting of the field. I
fixed that and added a test to check that restarting the SlotManager after
suspending it wouldn't cause any issues.
--
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]
