abdullah alamoudi has posted comments on this change. Change subject: Add Test NodeController, Test Data Generator, and Marker Logs ......................................................................
Patch Set 10: (16 comments) https://asterix-gerrit.ics.uci.edu/#/c/962/10/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 89: logRecord = new LogRecord(callback); > This is a nice improvement. Thank you :) https://asterix-gerrit.ics.uci.edu/#/c/962/10/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/bootstrap/TestNodeController.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/app/bootstrap/TestNodeController.java: Line 275: AsterixRuntimeComponentsProvider.RUNTIME_PROVIDER, LSMBTreeIOOperationCallbackFactory.INSTANCE, 0.01, > What is the meaning of "0.01"? Could we have a symbolic name? Done Line 288: AsterixRuntimeComponentsProvider.RUNTIME_PROVIDER, LSMBTreeIOOperationCallbackFactory.INSTANCE, 0.01, > s.a. Done 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 Done Line 759: int numPages = > not a fan, i think the original version is much easier to read Done https://asterix-gerrit.ics.uci.edu/#/c/962/10/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/AsterixLSMInsertDeleteOperatorNodePushable.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/AsterixLSMInsertDeleteOperatorNodePushable.java: Line 49: // we already have an index field!! should we add another pointer to the same object? > Could you elaborate more on the comment? Done 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 t Done Line 36: public void before(ByteBuffer buffer); > remove public Done Line 43: public void after(long lsn); > remove public Done 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 134: switch (logType) { > Much better with the switch. :) Line 258: switch (logType) { > Can we restore the default case? Even if a no-op, this is good documentati Done 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) That is right. I just added a comment because I don't think it should be taken care of in this static method. It is the responsibility of the caller. https://asterix-gerrit.ics.uci.edu/#/c/962/10/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java: Line 220: // TODO (amoudi): find a better reactive way to do this > It might be helpful to add a JIRA issue for yourself for this that describe Will do once this makes it to master. As of now, this issue doesn't exist (yet!) :-) https://asterix-gerrit.ics.uci.edu/#/c/962/10/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 48: public void set(String key, Object value); > I'm confused about this interface change. The idea of the shared object was Done. Notice that we sometimes need to store multiple objects in the taskCtx. Now, take a look in the next patch as to which classes had to change because of this. 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 sinc Done 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; > +1- I would cast bufferCache.getPageSize() to long 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: 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
