Yingyi Bu has posted comments on this change. Change subject: Introduce IStorageManager ......................................................................
Patch Set 1: (3 comments) My high-level comments: 1. What's the role of IStorageManager? Compile-time thing or runtime thing? To me, it should be a compile-time thing. In that sense, StorageManager shouldn't be instantiated at NC, rather the behavior at NC are controlled by the factories the storage manager provides. Right now, it is instantiated at NC as well, why? 2. StorageManager should only be instantiated in one call site per extension implementation, rather than instantiated here and there. https://asterix-gerrit.ics.uci.edu/#/c/1451/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java: Line 260: compactionInfo.first, compactionInfo.second); How do you capture isPartitioned in the new code? https://asterix-gerrit.ics.uci.edu/#/c/1451/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/ValidateUtil.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/ValidateUtil.java: Line 31: import org.apache.hyracks.algebricks.common.exceptions.AlgebricksException; I think the exception used here should be CompilationException, with error code..:-) https://asterix-gerrit.ics.uci.edu/#/c/1451/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/file/StorageManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/file/StorageManager.java: Line 25: public class StorageManager implements IStorageManager { The storage manager is instantiated at a number of places in the code base, CC, NC, various tests. IMO, it should be instantiated at one place, per extension. -- To view, visit https://asterix-gerrit.ics.uci.edu/1451 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: If86750cdb2436c713f6598e54d4aaaf23d9f7bbf Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
