asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Add Test NodeController, Test Data Generator, and Marker Logs
Date Thu, 21 Jul 2016 12:08:27 GMT
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 <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <michael.blow@couchbase.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message