abdullah alamoudi has posted comments on this change. Change subject: ASTERIXDB-1945 STO Cleanup Buffer Cache API ......................................................................
Patch Set 8: (13 comments) IO Dirs are created explicitly. TXN log files don't go through the same interfaces as files created through the buffer cache. As for the cases of the leftover files, we do take care of it, otherwise, tests will fail. https://asterix-gerrit.ics.uci.edu/#/c/1840/8/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/FileMapManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/FileMapManager.java: PS8, Line 32: ConcurrentMap > Since the call sites from BufferCache is synchronized on fileInfoMap for an Not all call sites are! and synchronization on the fileInfoMap is even less in this change PS8, Line 65: synchronized > synchronized is not need since all call sites from BufferCache has synchron synchronization shouldn't be based on the call site and not all of them are synchronized PS8, Line 75: synchronized > synchronized is not need since all call sites from BufferCache has synchron synchronization shouldn't be based on the call site and not all of them are synchronized https://asterix-gerrit.ics.uci.edu/#/c/1840/8/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/IFileHandle.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/IFileHandle.java: PS8, Line 24: FileReference getFileReference(); > Remove corresponding casts that are no longer needed at call sites? Done! https://asterix-gerrit.ics.uci.edu/#/c/1840/8/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/FileHandle.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/FileHandle.java: PS8, Line 56: if (!fileRef.getFile().exists()) { : throw HyracksDataException.create(ErrorCode.FILE_DOES_NOT_EXIST, fileRef.getAbsolutePath()); : } > If we want to complain non existence, we probably should move this to be b Done https://asterix-gerrit.ics.uci.edu/#/c/1840/8/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/build/IndexBuilder.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/build/IndexBuilder.java: PS8, Line 94: // Index is not registered but the index file exists : // This is another big problem that we need to disallow soon : // We can only disallow this if we have a global cleanup after crash : // on reboot > Can this happen if drop index is interrupted (cancelled)? Yes https://asterix-gerrit.ics.uci.edu/#/c/1840/8/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndexFileManager.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndexFileManager.java: PS8, Line 84: public boolean exists(); > From the call site, it looks that this method doesn't need to be exposed? Done! https://asterix-gerrit.ics.uci.edu/#/c/1840/8/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java: PS8, Line 158: if (fileManager.exists()) { : throw HyracksDataException.create(ErrorCode.CANNOT_CREATE_EXISTING_INDEX); : } > >>1. For example, drop index was interrupted/cancelled; This shouldn't be a problem because we drop in the IndexBuilder and as of now, the metadata is the source of truth but if we add a global recovery cleanup, then we can guard better against accidental data loss! PS8, Line 235: destroyDiskComponents(); > Why do memory components not need to be destroyed any more? because turned out when we deactivate, we actually destroy them. However, because of the not well defined API, double destroy was okay and so we used to do double destroy. https://asterix-gerrit.ics.uci.edu/#/c/1840/8/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/api/IInvertedIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/api/IInvertedIndex.java: PS8, Line 50: void purge() throws HyracksDataException; > Why only inverted index needs to purge? Explained when replying to Till's comments https://asterix-gerrit.ics.uci.edu/#/c/1840/8/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/file/TransientFileMapManager.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/file/TransientFileMapManager.java: PS8, Line 31: ConcurrentMap<Integer, FileReference> id2nameMap = new ConcurrentHashMap<>(); : private ConcurrentMap< > Similar to FileMapManager, do we really need ConcurrentMap here? we do. we do. PS8, Line 64: synchronized > Is it necessary here? yes PS8, Line 74: synchronized > Is it necessary? yes -- To view, visit https://asterix-gerrit.ics.uci.edu/1840 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15565b07afdc94ac74c608bfe4480fa09dcf8f1c Gerrit-PatchSet: 8 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
