dmvk commented on a change in pull request #18169:
URL: https://github.com/apache/flink/pull/18169#discussion_r783014729
##########
File path: flink-yarn-tests/src/test/resources/log4j2-test.properties
##########
@@ -18,7 +18,7 @@
# Set root logger level to OFF to not flood build logs
# set manually to INFO for debugging purposes
-rootLogger.level = OFF
+rootLogger.level = INFO
Review comment:
these should be only changed for local testing
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnResourceManagerDriver.java
##########
@@ -117,6 +118,8 @@
private TaskExecutorProcessSpecContainerResourcePriorityAdapter
taskExecutorProcessSpecContainerResourcePriorityAdapter;
+ private Phaser trackerOfReleasedResources;
Review comment:
```suggestion
private final Phaser trackerOfReleasedResources;
```
All the instance variables should be final, unless necessary for mutations
(which may require additional synchronization).
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/state/TaskExecutorStateChangelogStoragesManager.java
##########
@@ -124,10 +124,12 @@ public void releaseStateChangelogStorageForJob(@Nonnull
JobID jobId) {
}
public void shutdown() {
- if (closed) {
- return;
+ synchronized (this) {
Review comment:
atomic boolean?
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerRunner.java
##########
@@ -106,6 +107,8 @@
private static final int SUCCESS_EXIT_CODE = 0;
@VisibleForTesting static final int FAILURE_EXIT_CODE = 1;
+ private static Thread shutdownHook;
Review comment:
The shutdown hook lifecycle seems bit off. We store the reference in the
static variable, which is not tied to a particular instances of
`TaskManagerRunner`, but we tie the cleanup with a particular instance.
Maybe should we follow the same pattern as with other components (eg.
`FileExecutionGraphInfoStore`)?
This would basically mean, creating a shutdown hook in a constructor,
releasing it in the close method and storing the reference with the instance in
a final variable.
One more not on the current solution, the property is actually accessed from
different threads, so it should be marked as volatile (this is not a case for
the outlined approach as we can store it in a final variable then).
--
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]
