zentol commented on code in PR #24309:
URL: https://github.com/apache/flink/pull/24309#discussion_r1499093498
##########
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:
This never returns false after `start()` was called because the
mainThreadExecutor isn't nulled anywhere.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/FineGrainedSlotManager.java:
##########
@@ -243,7 +253,7 @@ private void registerSlotManagerMetrics() {
/** Suspends the component. This clears the internal state of the slot
manager. */
@Override
public void suspend() {
Review Comment:
Is this not called from the main thread? If not then there are a bunch of
other concurrency issues in here.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/FineGrainedSlotManager.java:
##########
@@ -108,27 +110,30 @@ public class FineGrainedSlotManager implements
SlotManager {
/** ResourceManager's id. */
@Nullable private ResourceManagerId resourceManagerId;
- /** Executor for future callbacks which have to be "synchronized". */
- @Nullable private Executor mainThreadExecutor;
+ private final Object lock = new Object();
+ /**
+ * Executor for future callbacks which have to be "synchronized". This
field being {@code null}
+ * indicates that the component isn't started.
+ */
+ @GuardedBy("lock")
+ @Nullable
+ private volatile ComponentMainThreadExecutor mainThreadExecutor;
Review Comment:
It's odd that this is both guarded by a lock and 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
