abdullah alamoudi has posted comments on this change. Change subject: Cleanup Buffer Cache ......................................................................
Patch Set 7: (6 comments) https://asterix-gerrit.ics.uci.edu/#/c/1840/6//COMMIT_MSG Commit Message: PS6, Line 16: 7. open non existing file is not allowed. > I think this should be allowed if WRITE is part of the mode. Otherwise, I if it doesn't exist, one should call the create method first... The more defined the behavior is, the more robust the storage can get! https://asterix-gerrit.ics.uci.edu/#/c/1840/6/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: PS6, Line 47: fileId > I wonder whether all methods have to be synchronized? It doesn't make sens This doesn't need to be synchroinzed. This is accessed by buffer cache which synchronize on fileInfoMap. complicating this further seems wasteful to me... Even if we decide to change this, it should be outside the scope of this change. https://asterix-gerrit.ics.uci.edu/#/c/1840/6/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/IoUtil.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/IoUtil.java: PS6, Line 30: /** > A brief doc for the class and each public method? Done https://asterix-gerrit.ics.uci.edu/#/c/1840/6/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: PS6, Line 55: READ_WRITE: : if (!fileRef.getFile().exists()) { : throw HyracksDataException.create(ErrorCode.FILE_DOES_NOT_EXIST, fileRef.getAbsolutePath()); : } > Why only check file existence in READ_WRITE? the only check for read write is because read will fail automatically while read write will create the file. I think auto creation is bad and dangerous. If you want to create a file, create it and then open it. if open does open and create if not found, then you risk not knowing about problems and code bugs! For example, if you expect a file to be there and it is not there, this will let you know. while if it simply creates the file, then you might not know. https://asterix-gerrit.ics.uci.edu/#/c/1840/6/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: PS6, Line 81: /** : * @return true if the index dir exists, false otherwise : */ : public boolean exists(); > I don't think it is necessary if we auto-create dir if not exits. Auto creating the dir is bad! and this is needed because we want to check if someone calls index.create. If the index directory exists, then this means that the index exists and the new create should fail. https://asterix-gerrit.ics.uci.edu/#/c/1840/6/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: PS6, Line 43: /** : * Purge the index files out of the buffer cache. : * Can only be called if the caller is absolutely sure the files don't contain dirty pages : * : * @throws HyracksDataException : * if the index is active : */ : void purge() throws HyracksDataException; > Maybe I missed sth. Why can destory() take care of that internally? purge() has nothing to do with destry(). it is a quick way to tell the buffer cache to remove the file pointer since it is being closed and all of its pages are not dirty. -- 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: 7 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
