Yingyi Bu has posted comments on this change. Change subject: Cleanup and bug fixes in Feeds pipeline ......................................................................
Patch Set 9: (9 comments) https://asterix-gerrit.ics.uci.edu/#/c/1523/9//COMMIT_MSG Commit Message: PS9, Line 7: fixes Make the commit message more concrete? E.g., cleanup what part & which bugs (ASTERIXDB-xxx) are fixed? https://asterix-gerrit.ics.uci.edu/#/c/1523/9/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IAdapterFactory.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IAdapterFactory.java: PS9, Line 87: IExternalDataSourceFactory Document the new method in the interface, e.g., why do we need this? https://asterix-gerrit.ics.uci.edu/#/c/1523/9/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedIntakeOperatorNodePushable.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedIntakeOperatorNodePushable.java: PS9, Line 72: message What's the purpose of the null message? Add some comments to explain that? https://asterix-gerrit.ics.uci.edu/#/c/1523/9/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java: PS9, Line 591: AlgebricksException Error code? https://asterix-gerrit.ics.uci.edu/#/c/1523/9/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java: PS9, Line 199: lsmAccessor enter/exist used to happen for each individual modification. What's the motivation of doing that for a frame batch? Exposing enter/exit to arbitrary callers seem dangerous to me. https://asterix-gerrit.ics.uci.edu/#/c/1523/9/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/runtime/UpsertCommitRuntime.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/runtime/UpsertCommitRuntime.java: PS9, Line 30: protected why protected? https://asterix-gerrit.ics.uci.edu/#/c/1523/9/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/PartitionDataWriter.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/PartitionDataWriter.java: PS9, Line 41: protected why protected? https://asterix-gerrit.ics.uci.edu/#/c/1523/9/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: PS9, Line 70: enter What's the motivation of exposing enter and exit? I think they're implementation details within each primitive operations, e.g., search/modify etc. Exposing them for arbitrary callers seem dangerous to me, how do we guarantee an external caller to follow the contract? https://asterix-gerrit.ics.uci.edu/#/c/1523/9/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMTreeIndexAccessor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMTreeIndexAccessor.java: PS9, Line 169: enterMemoryComponent expose enter/exist as public methods seem dangerous? You don't have the control at the caller side how enter/exit are called. -- To view, visit https://asterix-gerrit.ics.uci.edu/1523 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie97b2133ebecb7380cf0ba336e60ed714d06f8ee Gerrit-PatchSet: 9 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: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
