[GitHub] [flink] tillrohrmann commented on a change in pull request #13375: [FLINK-18815][filesystems][tests] Join SafetyNetCloseableRegistry's reaper thread before assertion

[GitBox <gi...@apache.org>]

tillrohrmann commented on a change in pull request #13375:
URL: https://github.com/apache/flink/pull/13375#discussion_r497298143



##########
File path: flink-core/src/main/java/org/apache/flink/core/fs/SafetyNetCloseableRegistry.java
##########
@@ -124,8 +124,9 @@ protected boolean doUnRegister(
 		return null != innerCloseable && closeableMap.remove(innerCloseable) != null;
 	}
 
-	@Override
-	public void close() throws IOException {
+	@VisibleForTesting
+	CloseableReaperThread closeInternally() throws IOException {

Review comment:
       Instead of introducing this method, couldn't we use the `SafetyNetCloseableRegistry(Supplier<CloseableReaperThread> reaperThreadSupplier)` constructor to pass in a thread over which we hold control? I think this would be a bit cleaner.

##########
File path: flink-core/src/main/java/org/apache/flink/core/fs/SafetyNetCloseableRegistry.java
##########
@@ -134,10 +135,17 @@ public void close() throws IOException {
 				--GLOBAL_SAFETY_NET_REGISTRY_COUNT;
 				if (0 == GLOBAL_SAFETY_NET_REGISTRY_COUNT) {
 					REAPER_THREAD.interrupt();

Review comment:
       I am wondering whether we shouldn't join on the reaper thread when the last `CloseableRegistry` closes? From a resource lifecycle management one could argue that the reaper thread belongs to the `CloseableRegistries` and when the last closes, then we must also make sure that the thread properly terminates.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org