Till Westmann has posted comments on this change. Change subject: Add Test NodeController and Test Data Generator ......................................................................
Patch Set 7: (19 comments) Mostly structural comments (I assume that the functionality works just fine). https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/CommitRuntime.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/CommitRuntime.java: Line 91: logRecord = new LogRecord(index == null ? null It seems strange to pass an IndexAccessor in here? Can we pass a smaller interface in the just provides what is needed here? https://asterix-gerrit.ics.uci.edu/#/c/962/7/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 81: logRecord = new LogRecord(null); Can we have an additional constructor and keep this file unmodified? https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/PrimaryIndexOperationTracker.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/PrimaryIndexOperationTracker.java: Line 124: LogRecord logRecord = new LogRecord(null); Can we have an additional constructor and keep this file unmodified? https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ILogRecord.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ILogRecord.java: Line 175: public long getPreviousMarkerLSN(); These 2 last methods look strangely asymmetric, also it is strange to have the LogRecord set something somewhere else. https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/CreateDataverseStatement.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/CreateDataverseStatement.java: Line 39: } How about this.format = format == null ? NonTaggedDataFormat.class.getName() : format; ? https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-replication/src/main/java/org/apache/asterix/replication/management/ReplicationChannel.java File asterixdb/asterix-replication/src/main/java/org/apache/asterix/replication/management/ReplicationChannel.java: Line 212: remoteLog = new LogRecord(null); Can we have an additional constructor and keep this file unmodified? https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/AbstractIndexModificationOperationCallback.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/AbstractIndexModificationOperationCallback.java: Line 53: logRecord = new LogRecord(null); Can we have an additional constructor and keep this file unmodified? https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/LockThenSearchOperationCallback.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/LockThenSearchOperationCallback.java: Line 51: this.logRecord = new LogRecord(null); Can we have an additional constructor and keep this file unmodified? https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogBufferTailReader.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogBufferTailReader.java: Line 34: logRecord = new LogRecord(null); Can we have an additional constructor and keep this file unmodified? https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogReader.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogReader.java: Line 67: this.logRecord = new LogRecord(null); Can we have an additional constructor and keep this file unmodified? https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/TransactionContext.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/TransactionContext.java: Line 112: logRecord = new LogRecord(null); Can we have an additional constructor and keep this file unmodified? https://asterix-gerrit.ics.uci.edu/#/c/962/7/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/comm/IFrameWriter.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/comm/IFrameWriter.java: Line 80: // No Op Why do we change the default here? Seems that that would impact other Hyracks consumers. https://asterix-gerrit.ics.uci.edu/#/c/962/7/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/context/IHyracksTaskContext.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/context/IHyracksTaskContext.java: Line 49: public void setSharedFrame(VSizeFrame frame); I don't understand the modification of this fundamental Hyracks interface. Couldn't we just have an Asterix-specific object that contains a VSizeFrame and a map that will get stored in the HyracksTaskContext? https://asterix-gerrit.ics.uci.edu/#/c/962/7/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NodeControllerService.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NodeControllerService.java: Line 602: * Method below are used for unit tests Could we put them into the test sources and make things that they need package accessible? Line 617: if (!shuttedDown) { Could we fix the spelling here? https://asterix-gerrit.ics.uci.edu/#/c/962/7/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/Task.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/Task.java: Line 399: public void setSharedFrame(VSizeFrame frame) { As in the IHyracksTaskContext I don't think that we should need this modification. https://asterix-gerrit.ics.uci.edu/#/c/962/7/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 63: aString.append("Snapshot, "); Should we align this with the case label? https://asterix-gerrit.ics.uci.edu/#/c/962/7/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IMetaDataPageManager.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IMetaDataPageManager.java: Line 176: public void setMostRecentMarkerLSN(long lsn) throws HyracksDataException; Do we need these 2 methods on this interface? The second one isn't used right now. https://asterix-gerrit.ics.uci.edu/#/c/962/7/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMHarness.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMHarness.java: Line 68: public void setMostRecentMarkerLSN(long lsn); The methods look strange on this interface. All other methods are about starting and stopping processes and these 2 are about setting and getting a value. -- 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: 7 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
