abdullah alamoudi has posted comments on this change. Change subject: [NO ISSUE][STO] Fix memory leaks in storage ......................................................................
Patch Set 8: (22 comments) https://asterix-gerrit.ics.uci.edu/#/c/2115/8/hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/DataUtils.java File hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/DataUtils.java: Line 31: * > add missing param Done Line 44: * > add missing param Done https://asterix-gerrit.ics.uci.edu/#/c/2115/8/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/MemoryComponentMetadata.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/MemoryComponentMetadata.java: PS8, Line 77: > () -> This is a static string. https://asterix-gerrit.ics.uci.edu/#/c/2115/8/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/VirtualBufferCache.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/VirtualBufferCache.java: Line 45: > @ThreadSafe Not sure it is... At least definitely not completely I would like to make sure it is before I add that. PS8, Line 59: budget > pages Done PS8, Line 63: this > no need This is for the case where budget is passed as 0... but I agree with you... I will change it PS8, Line 65: pageBudget > if at any point we are exceeding this queue capacity by allocating an extra Which works as designed. the idea here is that if you exceed, you should get rid of the extra. PS8, Line 65: this > no need This is needed as ArrayBlockingQueue has a fixed size array that must be passed when initialized. PS8, Line 124: logStats > Remove this or make the logging level lower This is very infrequent as it only happens when opening a file or closing a file.. Basically on flushes. PS8, Line 132: openFile > shouldn't this throw unsupportedOp? Then everything will break. This is called whenever we reset a memory component. PS8, Line 153: reclaimedPages > This was for debugging, it isn't needed anymore I want to keep it. It doesn't happen frequently, costs nothing. And will help with debugging if something breaks. PS8, Line 163: curr.getFrameSizeMultiplier() > 1 > define a static method isLargePage and use it here and below. Possibly in I Done PS8, Line 187: if (LOGGER.isLoggable(Level.INFO)) { : LOGGER.log(Level.INFO, "Reclaimed pages = " + reclaimedPages); : } : logStats(); > This was for debugging, it isn't needed anymore Can we keep it? PS8, Line 193: curr > page Done PS8, Line 197: curr.getFrameSizeMultiplier() == 1 > !isLargePage Done PS8, Line 199: freePages.offer(curr) > Check the return value of offer if it is possible to exceed the fixed capac This is intentional. If that happens, we want to get rid of the additional page PS8, Line 205: null > not needed Done PS8, Line 306: > synchronized I don't think that it is intended for the vbc open and close methods to be thread safe... I think design is for those to be handled by the caller. Let's discuss this/ do this in a different change PS8, Line 308: throw HyracksDataException.create(ErrorCode.VBC_ALREADY_OPEN); > This should be IllegalStateException Not sure about this... Let's discuss this/ do this in a different change PS8, Line 344: > synchronized See above. PS8, Line 346: throw HyracksDataException.create(ErrorCode.VBC_ALREADY_CLOSED); > This should be IllegalStateException See above. PS8, Line 445: numPages > update Done -- To view, visit https://asterix-gerrit.ics.uci.edu/2115 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ae9736c9b5fdba5795245bdf835c023e3f73b15 Gerrit-PatchSet: 8 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Luo Chen <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
