abdullah alamoudi has posted comments on this change. Change subject: Add Test NodeController and Test Data Generator ......................................................................
Patch Set 7: (27 comments) 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 in Done 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? Done 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? Done 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 Done https://asterix-gerrit.ics.uci.edu/#/c/962/7/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/FrameStack.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/FrameStack.java: Line 87: ByteBuffer aBuffer = emptyBuffers.isEmpty() ? ByteBuffer.allocate(frameSize) : emptyBuffers.pop(); > If you change to poll(), you can avoid two calls to emptyBuffers in case of Done Line 114: * > MAJOR SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/962/7/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 83: file.getParentFile().mkdirs(); > Note, this can fail in case two threads try to create the dir at same time. Done Line 98: } catch (Throwable th) { > BLOCKER SonarQube violation: Done 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? Done 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? Done 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? Done 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? Done 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? Done 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? Done 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. Done 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 pack Done Line 617: if (!shuttedDown) { > Could we fix the spelling here? Done 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 modifi Done 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, "); > I don't understand this. This is just for debugging purposes. I sometime want to see what kind of message is going through an execution pipeline. Line 63: aString.append("Snapshot, "); > Should we align this with the case label? Done 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 rig I am pretty sure we need both of them. This is not a finished change yet. I don't remember the exact detail and so I removed it. we can get it from here again if we need to. 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/freepage/LinkedMetaDataPageManager.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/freepage/LinkedMetaDataPageManager.java: Line 507: if (!appendOnly || (appendOnly && confiscatedMetaNode == null)) { > Well, this SonarQube observation is pretty broken. yes. I noticed that and it is very strange. I am afraid that we both don't see it and SonarQube is right. 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 sta Done 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/impls/AbstractLSMComponent.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMComponent.java: Line 42: this(null, -1L); > Can this call this(null) as before, to avoid the -1L in two places? Done 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/impls/AbstractMemoryLSMComponent.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractMemoryLSMComponent.java: Line 35: public AbstractMemoryLSMComponent(IVirtualBufferCache vbc, boolean isActive, ILSMComponentFilter filter, > Can we keep the old ctor around, then we can eliminate changes to subclasse Done https://asterix-gerrit.ics.uci.edu/#/c/962/7/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndexMemoryComponent.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndexMemoryComponent.java: Line 36: super(vbc, isActive, filter, -1L); > Can we keep the old super ctor around, then we can eliminate this change. Done https://asterix-gerrit.ics.uci.edu/#/c/962/7/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeMemoryComponent.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeMemoryComponent.java: Line 36: super(vbc, isActive, filter, -1L); > Can we keep the old super ctor around, then we can eliminate this change. Done -- 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
