Michael Blow has posted comments on this change. Change subject: ASTERIXDB-1479: Change storage valid int and add explicit version ......................................................................
Patch Set 8: (6 comments) https://asterix-gerrit.ics.uci.edu/#/c/919/8/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/frames/BTreeFieldPrefixNSMLeafFrame.java File hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/frames/BTreeFieldPrefixNSMLeafFrame.java: Line 464: strBuilder.append("TUPLE_COUNT_OFF: " + tupleCountOff + "\n"); Should we revert this? Not sure why we want to change the name of the displayed keys only. If we're keeping, we probably should fix the spacing so that the values align with the others. https://asterix-gerrit.ics.uci.edu/#/c/919/8/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/frames/TreeIndexNSMFrame.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/frames/TreeIndexNSMFrame.java: Line 284: strBuilder.append("TUPLE_COUNT_OFF: " + tupleCountOff + "\n"); same comment as before https://asterix-gerrit.ics.uci.edu/#/c/919/8/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/freepage/LinkedMetaDataPageManager.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/freepage/LinkedMetaDataPageManager.java: Line 475: return (long)(metadataPageNum * bufferCache.getPageSize()) + LIFOMetaDataFrame.LSN_OFF; Should one of the operands to the multiply be casted to a long rather than the product? I think otherwise we risk overflow. https://asterix-gerrit.ics.uci.edu/#/c/919/8/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: Line 53: public enum TREE_INDEX_STATE { enums should be named like classes (e.g. TreeIndexState)- not sure why this isn't caught by SonarQube Line 55: WRONG_VERS, Not loving this abbreviation- can we have WRONG_VERSION or perhaps VERSION_MISMATCH? Line 105: return TREE_INDEX_STATE.WRONG_VERS; We may have to revisit this in the future- we're going to need to know which specific version the index file has in order to provide the detailed error message Till was proposing, I think. -- To view, visit https://asterix-gerrit.ics.uci.edu/919 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I888ff0eacf5b3cb6ad7ec002c74f113c6ffcd496 Gerrit-PatchSet: 8 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
