Yingyi Bu has posted comments on this change. Change subject: ASTERIXDB-1945 [STO] Cleanup Buffer Cache API ......................................................................
Patch Set 16: (3 comments) I think most of my concerns are addressed. My only concern now is the thread-safety contract of FileMapManager. https://asterix-gerrit.ics.uci.edu/#/c/1840/12//COMMIT_MSG Commit Message: PS12, Line 7: ASTERIXDB-1945 [ASTERIXDB-1945] https://asterix-gerrit.ics.uci.edu/#/c/1840/16/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java: PS16, Line 798: return fileMapManager.registerFile(fileRef); You probably have to synchronized on fileInfoMap here? See my comments in FileMapManager. https://asterix-gerrit.ics.uci.edu/#/c/1840/16/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/file/FileMapManager.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/file/FileMapManager.java: PS16, Line 45: lookupFileId I still doubt the synchronization model of this class. All call sites from BufferCache used to be based on fileInfoMap, and all call sites from VirtualBufferCache used to be used on fileMapManager (i.e., this). With the change, it's a bit unclear to see the thread-safety contract this class provides. My point is that: (1) if we say this class provides thread safety, we make sure it is the case for every method in this class; (2) if we say this class doesn't provide thread safety, we don't do it at all --- it's like the old approach; Previously all call sites are synchronized fine. As far as I can tell, the non-synchronized(fileInfoMap) call site in BufferCache is introduced by this change, i.e., BufferCache.createFile(...) -- it doesn't synchronized on fileInfoMap like elsewhere in BufferCache. Either of (1) or (2) would work for me. Currently this class seems 50%-baked -- there are some synchronizations but there could be inconsistency between id2nameMap and name2IdMap. For example, register/unregister are not atomic --a caller can lookupFileId(...) while the file hasn't fully registered or unregistered, and possibly open an non-existent file? -- 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: 16 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
