MINOR: Unmap index on close follow-up#5778
Conversation
|
I noticed after merging the unmap on close change that maybe more needs to be done @lindong28. |
Update comment, isMemoryMappedBufferClosed variable and handle `close` followed by `delete`.
ijuma
force-pushed
the
unmap-index-on-close-follow-up
branch
from
af42b12 to
23e97f0
Compare
|
retest this please |
|
@lindong28 does this seem OK to you? The previous change was incomplete so I think we need this in 2.1 to avoid issues. |
|
@ijuma Sure, I will review it today. |
| // (the clean shutdown file is written after the logs are all closed). | ||
| producerStateManager.takeSnapshot() | ||
| logSegments.foreach(_.close()) | ||
| isMemoryMappedBufferClosed = true |
There was a problem hiding this comment.
Now that we also set isMemoryMappedBufferClosed to true in close(), the first sentence in its comment, The memory mapped buffer for index files of this log will be closed with either delete() or closeHandlers(), is no longer accurate.
Can we update its comment to e.g. After memory mapped buffer is closed, all disk IO operation other than delete() on this log should throw KafkaStorageException.
| maybeHandleIOException(s"Error while deleting log for $topicPartition in dir ${dir.getParent}") { | ||
| lock synchronized { | ||
| checkIfMemoryMappedBufferClosed() | ||
| removeLogMetrics() |
There was a problem hiding this comment.
I am wondering if it would be more intuitive not to set isMemoryMappedBufferClosed = true in delete().
The reason is that, if we don't call checkIfMemoryMappedBufferClosed() in delete(), it suggests that delete() can be done anytime after close() is called, which is actually the case in logManager.replaceCurrentWithFutureLog() which calls addLogToBeDeleted(sourceLog) after sourceLog.close(). Then the intuition is that, if delete() can be done without considering the state (i.e. isMemoryMappedBufferClosed) of log, it probably should not change the state of log as well.
Another intuition is that, it seems weird if delete() sets isMemoryMappedBufferClosed to true and yet delete() can be called again without triggering KafkaStorageException.
There was a problem hiding this comment.
It's a good question. It's a little unclear what's the best path as we don't generally require close to be called before delete for the various log/index classes. As a result, we have one case where we don't call Log.close at all (the LogManager.asyncDelete path) and rely on the Log.delete method to release resources. If we don't set isMemoryMappedBufferClosed in the delete method, then it would stay false for this path.
As it happens, the fact that we don't close the file handles before we rename the directory in LogManager.asyncDelete is one reason why things break on Windows.
There was a problem hiding this comment.
Regarding the design for the Log.java API, it seems intuitive to only allow delete() after close() or closeHandlers() is invoked. delete() will only delete files on disk without reading or writing anything and thus it does not require any handler to be open for this log.
And it seems reasonable to enforce the rule that we always close a log before deleting it in e.g. LogManager.asyncDelete(). And delete() can throw IllegalStateException if isMemoryMappedBufferClosed is false.
There was a problem hiding this comment.
Seems like this needs a bit more discussion. I propose we revert the change in 2.1 to close the index during Log.close so that the behaviour remains the same as in 2.0. In trunk, we can make the changes after we agree to them. How does that sound?
There was a problem hiding this comment.
Sure. Sounds good. Thanks for the patch! The previous PR has been reverted in 2.1 branch.
|
This PR is being marked as stale since it has not had any activity in 90 days. If you If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact). If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
|
This PR has been closed since it has not had any activity in 120 days. If you feel like this |
2 participants
Update comment, isMemoryMappedBufferClosed variable and
handle
closefollowed bydelete.Committer Checklist (excluded from commit message)