You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@asterixdb.apache.org by "Till Westmann (Code Review)" <do...@asterixdb.incubator.apache.org> on 2016/07/01 00:48:25 UTC
Change in asterixdb[master]: Add Test NodeController and Test Data Generator
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 <ba...@gmail.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mi...@couchbase.com>
Gerrit-Reviewer: Till Westmann <ti...@apache.org>
Gerrit-Reviewer: abdullah alamoudi <ba...@gmail.com>
Gerrit-HasComments: Yes