abdullah alamoudi has posted comments on this change. Change subject: [ASTERIXDB-2367][STO] Various Fixes for BufferCache ......................................................................
Patch Set 8: (7 comments) I am a bit worried about concurrent create/deletes of files.. Seems like they were serialized correctly before.. are we sure this is still the case? If test cases don't already exist, we should add some. If we think synchronizing of deletes/creates should be the responsibility of the user of the buffer cache, then we should discuss/ make that clear 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 thread will delete while this thread tries to write? PS8, Line 610: if (fInfo == null) { : throw HyracksDataException.create(ErrorCode.FILE_DOES_NOT_EXIST); : } Let's make this an illegal state exception? PS8, Line 637: assert buf.capacity() == (pageSize * totalPages); can we replace the assert with a check and throw an illegal state exception? we probably should never use asserts. Thoughts? PS8, Line 852: > why the change from >= to > ? This will exceed by 1. No? PS8, Line 877: erenceCount() <= 0 This is sad.. it should never be less than 0. should we throw an exception if that is ever the case? PS8, Line 889: > should this also be ==? 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 a problem when someone else tries to create the file at the same time? -- 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
