Luo Chen has posted comments on this change. Change subject: [ASTERIXDB-2367][STO] Various Fixes for BufferCache ......................................................................
Patch Set 8: (7 comments) https://asterix-gerrit.ics.uci.edu/#/c/2604/8/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: PS8, Line 612: > any idea why it was like that? any case where it is expected that some thre I think the main reason (maybe) is that previously we adopted a lazy strategy - when a file is deleted, we simply mark this bit while the file is still in fileInfoMap. PS8, Line 610: if (fInfo == null) { : throw HyracksDataException.create(ErrorCode.FILE_DOES_NOT_EXIST); : } > Let's make this an illegal state exception? Done PS8, Line 637: assert buf.capacity() == (pageSize * totalPages); > can we replace the assert with a check and throw an illegal state exceptio Done PS8, Line 852: > > why the change from >= to > ? The logic was slightly different on how do we manage fileInfoMap. Previously we first check the size of the map, and then add the new file. Right now because of concurrency control, we first add a new file handle to the map, then check its size. Thus, this needs to be changed accordingly (otherwise it fails some tests on maxOpenFiles...) PS8, Line 877: erenceCount() <= 0 > This is sad.. it should never be less than 0. Done PS8, Line 889: > > should this also be ==? Same explanation above... This happens we all files are in use and we cannot evict a file. There is a test on this as well... PS8, Line 998: if (mapped) { : deleteFile(fileId); : } else { : IoUtil.delete(fileRef); : } > since we're doing these out of the synchronized block, doesn't that create Not sure concurrent creation/deletion is an allowed operation...Right now we don't have such scenario, and I think this scenario would be invalid as well. Suppose we have a file created. If two threads create/delete the file concurrently (even with previous implementation), the result would be undefined. It's possible that file is still there or the file is gone. But anyway, callers in this case should synchronize on these operations? -- To view, visit https://asterix-gerrit.ics.uci.edu/2604 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8a199d3e83592425ab5055bd12bde519e80deb13 Gerrit-PatchSet: 8 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Luo Chen <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Luo Chen <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
