Core: Add finalizer to ResolvingFileIO #7536
Conversation
| @@ -83,21 +90,22 @@ public Map<String, String> properties() { | |||
|
|
|||
| @Override | |||
| public void initialize(Map<String, String> newProperties) { | |||
| close(); // close and discard any existing FileIO instances | |||
There was a problem hiding this comment.
@rdblue do you remember why this was necessary? I added a flag for detecting whether close() was already called to be used in the finalize() method. Given that close() is being called here and then later again during a proper close() call, I ended up removing this call here
There was a problem hiding this comment.
It was just in case initialize was called again. Technically, that invalidates the configuration and we should close/reopen any FileIO instances that are held.
We could just reset isClosed in this method instead of removing this.
There was a problem hiding this comment.
I've reset isClosed here for now but I think we should consider removing this close() call from initialize(..) in a follow-up
|
|
||
| @SuppressWarnings("checkstyle:NoFinalizer") | ||
| @Override | ||
| protected void finalize() throws Throwable { |
There was a problem hiding this comment.
Sorry @nastra, but stong -100 on using finalize() at all. See JDK-8165641.
There was a problem hiding this comment.
@snazy, we know.
Iceberg uses this pattern to ensure that close is called. We also log the stack trace at WARN so that we can track down leaks. It is better to close in a finalizer than to not close at all.
There was a problem hiding this comment.
@snazy I agree that finalize() is a pattern we shouldn't rely on, but on Spark executor notes we don't have any other way of closing resources unfortunately
nastra
force-pushed
the
add-finalizer-to-resolving-fileio
branch
from
a2a6682
to
f54428a
Compare
No description provided.