Michael Blow has posted comments on this change. Change subject: Cleanup Buffer Cache ......................................................................
Patch Set 4: (17 comments) https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/ErrorCode.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/ErrorCode.java: PS4, Line 66: EXISTS EXIST PS4, Line 105: FILE_NAME FILENAME https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/IoUtil.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/IoUtil.java: PS4, Line 37: getAbsolutePath getFile? (avoid reconstituting the filename / path) PS4, Line 40: String absFileName refactor to accept java.io.File? PS4, Line 42: File file = Paths.get(absFileName).toFile(); : if (file.isDirectory()) { : FileUtils.deleteDirectory(file); : } else { : Files.delete(file.toPath()); : } Why not just use FileUtils.deleteQuietly(new File(absFileName))? Seems overly complicated and constructs this intermediate nio Path object PS4, Line 57: .getAbsoluteFile(). this should not be needed https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-api/src/main/resources/errormsg/en.properties File hyracks-fullstack/hyracks/hyracks-api/src/main/resources/errormsg/en.properties: PS4, Line 49: doesn't exists /does not exist/ PS4, Line 88: file name /filename/ https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/FileHandle.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/FileHandle.java: PS4, Line 57: EXISTS EXIST https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndexFileManager.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndexFileManager.java: PS4, Line 60: SPLITTER Can we name this DELIMITER? A splitter indicates something that does the splitting, not the thing we split on (which is a delimiter)... https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/ondisk/OnDiskInvertedIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/ondisk/OnDiskInvertedIndex.java: PS4, Line 639: purge() synchronized? https://asterix-gerrit.ics.uci.edu/#/c/1840/4/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: PS4, Line 1004: IoUtil.delete(fileRef); placed in its own finally? if ioManager.close() fails, we won't delete the file. https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/DebugBufferCache.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/DebugBufferCache.java: PS4, Line 255: openFile why don't we increase the metric here? PS4, Line 260: deleteFile why don't we increase the metric here? https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-common-test/src/test/java/org/apache/hyracks/storage/common/BufferCacheTest.java File hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-common-test/src/test/java/org/apache/hyracks/storage/common/BufferCacheTest.java: PS4, Line 105: WS PS4, Line 113: WS https://asterix-gerrit.ics.uci.edu/#/c/1840/4/hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/JSONUtil.java File hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/JSONUtil.java: PS4, Line 180: /** : * Write map as a json string. if an object is a string and starts with a { or [ : * then it assumes that it is a json object or a json array and so it doesn't surround : * it with " : * : * @param map : * a map representing the json object : * @return : * a String representation of the json object : */ : public static String fromMap(Map<String, Object> map) { : StringBuilder aString = new StringBuilder(); : aString.append("{ "); : boolean first = true; : for (Entry<String, Object> entry : map.entrySet()) { : if (!first) { : aString.append(", "); : } : aString.append("\""); : aString.append(entry.getKey()); : aString.append("\""); : aString.append(" : "); : Object value = entry.getValue(); : if (value instanceof String) { : String strValue = (String) value; : if (strValue.startsWith("{") || strValue.startsWith("[")) { : aString.append(value); : } else { : aString.append("\""); : aString.append(value); : aString.append("\""); : } : } else { : aString.append(value); : } : first = false; : } : aString.append(" }"); : return aString.toString(); : } why DIY? should we leave JSON serialization to Jackson? -- 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: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
