abdullah alamoudi 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 Done PS4, Line 105: FILE_NAME > FILENAME Done 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) Done PS4, Line 40: String absFileName > refactor to accept java.io.File? Done 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 ove problem is that quietly doesn't throw an exception on failure PS4, Line 57: .getAbsoluteFile(). > this should not be needed Done 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/ Done PS4, Line 88: file name > /filename/ Done 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 Done 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 s Done. I hated SPLITTER too but didn't want to use SEPARATOR since it might be confused with Files.Separator. DELIMITER seems perfect 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? Done 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 Done 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? Done PS4, Line 260: deleteFile > why don't we increase the metric here? Done 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 Done PS4, Line 113: > WS Done 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? had the same discussion with Murtadha. The reason is that if a String value in the map is actually a JSON Object String or a JSON array, Jackson will treat it as a String and will surround it with "" because the object is of type String. Many of our toString methods return a json string and with this, you can create a cleaner json. -- 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
