MINOR: AbstractIndex.close should unmap

[ijuma]

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[ijuma]

@lindong28 @junrao Is there a reason why we don't do this?

[lindong28]

@ijuma Thanks for fixing this. LGTM.

AbstractIndex.closeHandler was added in the JBOD KIP-112. In case a disk is considered faulty due to IOException, we can not call close() because close() does IO operation on the disk. In order to unmount the fault disk without shutting down the broker, we need to unmap the reference to all files on the disk by calling closeHandler(). Given the motivation of adding closeHandler(), we can see that its motivation is separate from the usage of close(), which means that we do not need to change the implementation of close() assuming our Kafka server implementation is already working well in the non-JBOD mode. This is why we didn't do this previously.

If I understand it right, it is still safe (with current Kafka implementation) not to close handler in close(). But it is more elegant and intuitive to call closeHandler() in close().

[ijuma]

@lindong28, my claim is that close should have been updated when we added the unmap call to deleteIfExists. To make this more obvious, I have updated deleteIfExists to call closeHandler as well and moved the comment previously existing there to closeHandler.

[lindong28]

@ijuma Yeah I certainly agree with your observation and the updated code is better.

[ijuma]

Thanks @lindong28. I'll wait for @junrao to confirm before merging.

[junrao]

@ijuma : Thanks for the patch. LGTM

[ijuma]

Merging to trunk and 2.1 branches.