Murtadha Hubail has posted comments on this change. Change subject: [NO ISSUE][STO] Fix memory leaks in storage ......................................................................
Patch Set 2: (24 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 Line 44: * add missing param 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: () -> https://asterix-gerrit.ics.uci.edu/#/c/2115/6/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 357: } > CRITICAL SonarQube violation: replace those \ by % while you are at it 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: public class VirtualBufferCache implements IVirtualBufferCache { @ThreadSafe PS8, Line 59: pages PS8, Line 63: che(ICacheMemoryAlloc a comment saying what this is would be helpful. I know it's not your fault PS8, Line 63: ic V no need PS8, Line 65: ; if at any point we are exceeding this queue capacity by allocating an extra page, the freePages.offer(.) will return false. Isn't this possible? PS8, Line 65: ger( no need PS8, Line 124: } Remove this or make the logging level lower PS8, Line 132: shouldn't this throw unsupportedOp? PS8, Line 153: --end; This was for debugging, it isn't needed anymore PS8, Line 163: < end && firstUnused.dpid() != - define a static method isLargePage and use it here and below. Possibly in ICachedPage PS8, Line 187: VirtualPage page = null; : int hash = hash(dpid); : CacheBucket bucket = buckets[hash]; : This was for debugging, it isn't needed anymore PS8, Line 193: page PS8, Line 197: !isLargePage PS8, Line 199: } Check the return value of offer if it is possible to exceed the fixed capacity of this queue. If you are %100 sure that this won't happen, use add instead of offer, which throws an exception if the capacity is exceeded. Otherwise, I would remove the capacity restriction. PS8, Line 205: not needed PS8, Line 306: g synchronized PS8, Line 308: (int i = 0; i < numPages; i++) { This should be IllegalStateException PS8, Line 344: synchronized PS8, Line 346: ppend(String.format("Page size = %d\n", pageSize)); This should be IllegalStateException PS8, Line 445: update -- 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: 2 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
