Refactor part of IndexFileDeleter and ReplicaFileDeleter into a common utility class #12126
Conversation
| @@ -206,7 +206,7 @@ private synchronized void _transferAndCancel(CopyJob prevJob) throws IOException | |||
| if (Node.VERBOSE_FILES) { | |||
| dest.message("remove partial file " + prevJob.current.tmpName); | |||
| } | |||
| dest.deleter.deleteNewFile(prevJob.current.tmpName); | |||
| dest.deleter.deleteIfNoRef(prevJob.current.tmpName); | |||
There was a problem hiding this comment.
Seems like deleteIfNoRef is always safer than deleteNewFile. Do we still need the method deleteNewFile?
There was a problem hiding this comment.
Ah I actually want to do it the opposite way, I'm not 100% sure why we need a deleteNewFile (force delete) here rather than deleteIfNoRef but I don't want to introduce a (possibly) different behavior in this change so I kept it.
Altho the deleteNewFile is a bit misleading so I changed the name to forceDeleteFile
| } | ||
| } | ||
|
|
||
| deleteFiles(toDelete); | ||
| fileDeleter.deleteFilesIfNoRef(toDelete); |
There was a problem hiding this comment.
we get toDelete from fileDeleter as well. should this part of code be encaptulated into fileDeleter as a whole. like call fileDeleter.clean() or sth.
There was a problem hiding this comment.
Yeah I was struggling with this as well. The reason I put it like this is that I feel like the IllegalStateException here is quite important so I don't want to throw it away, but if I put it into say fileDeleter.clean(), then it does not always make sense too.
There was a problem hiding this comment.
Thank you for this refactor @zhaih! It is very hairy code that was doing nearly the same thing in two different places. The rote refactor is badly needed.
I left a few small comments... I think this is very close.
| @@ -154,7 +151,7 @@ public IndexFileDeleter( | |||
| || fileName.startsWith(IndexFileNames.PENDING_SEGMENTS))) { | |||
|
|
|||
| // Add this file to refCounts with initial count 0: | |||
| getRefCount(fileName); | |||
| fileDeleter.getRefCount(fileName); | |||
There was a problem hiding this comment.
Kinda weird that a method starting with get has this set-like side effect, sigh.
There was a problem hiding this comment.
We can maybe later / separately improve the naming, or add a dedicated initRefCount(fileName) method or so.
There was a problem hiding this comment.
Yeah I don't like that part either, it's quite tricky, good thing is we're not abusing the old get so I just followed your suggestion and created a initRefCount, feels better now LOL.
| @@ -610,76 +601,34 @@ public void checkpoint(SegmentInfos segmentInfos, boolean isCommit) throws IOExc | |||
| } | |||
| } | |||
|
|
|||
| private void logInfo(FileDeleter.MsgType msgType, String msg) { | |||
| if (msgType == FileDeleter.MsgType.REF && VERBOSE_REF_COUNTS == false) { | |||
There was a problem hiding this comment.
Probably you could have just moved the VERBOSE_REF_COUNTS down into FileDeleter, but since this is a rote refactor it's good to separate / do that later.
There was a problem hiding this comment.
I think we could leave VERBOSE_REF_COUNTS as is, since FileDeleter is just responsible for throwing message out with it's type, and here VERBOSE_REF_COUNTS is the switch on the IFD side so I think it makes sense to let it stay there.
There was a problem hiding this comment.
Thank you for refactoring such a hairy part of Lucene @zhaih!
Description
See #11885
Approach
I extracted mainly the ref counting part into the new
FileDeleterso that bothIndexFileDeleterandReplicaFileDeleterwill use it as a component. It does not provide any level of thread-safety since theIndexFileDeleteroriginally was implemented in a lock-free way, so the user of this newFileDeleteris responsible for the synchronization if necessary.Test
I haven't written any specific test for this since I feel like the existing tests (mainly
TestIndexFileDeleter) should already provide a good coverage