Michael Blow has posted comments on this change. Change subject: Add Test NodeController, Test Data Generator, and Marker Logs ......................................................................
Patch Set 10: (9 comments) https://asterix-gerrit.ics.uci.edu/#/c/962/10/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/DatasetLifecycleManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/DatasetLifecycleManager.java: Line 733: : storageProperties.getMemoryComponentNumPages(); not a fan, i think the original version is much easier to read Line 759: int numPages = not a fan, i think the original version is much easier to read https://asterix-gerrit.ics.uci.edu/#/c/962/10/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ILogMarkerCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ILogMarkerCallback.java: Line 28: public static final String KEY_MARKER_CALLBACK = "MARKER_CALLBACK"; Should we remove the redundant modifiers (public static final) given that this is a new interface? Line 36: public void before(ByteBuffer buffer); remove public Line 43: public void after(long lsn); remove public https://asterix-gerrit.ics.uci.edu/#/c/962/10/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java: Line 258: switch (logType) { > MAJOR SonarQube violation: Can we restore the default case? Even if a no-op, this is good documentation that other case should do nothing. If we are handling all cases, we should add a default case that throws IllegalStateException() https://asterix-gerrit.ics.uci.edu/#/c/962/10/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/StoragePathUtil.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/StoragePathUtil.java: Line 92: file.getParentFile().mkdirs(); Isn't the issue still present, (reported in patch 7) https://asterix-gerrit.ics.uci.edu/#/c/962/10/hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/io/MessagingFrameTupleAppender.java File hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/io/MessagingFrameTupleAppender.java: Line 119: int messageSize = message.getBuffer().limit() - message.getBuffer().position(); Can we add a local final variable for message.getBuffer()? Seems nice since we call it 3 times (from 4 places). https://asterix-gerrit.ics.uci.edu/#/c/962/10/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 476: return (metadataPageNum * bufferCache.getPageSize()) + LIFOMetaDataFrame.LSN_OFFSET; > CRITICAL SonarQube violation: +1- I would cast bufferCache.getPageSize() to long -- To view, visit https://asterix-gerrit.ics.uci.edu/962 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b9aa8de758b7d26ca34868b16e5ce693e0c0243 Gerrit-PatchSet: 10 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[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
