Till Westmann has posted comments on this change. Change subject: Configuration Revamp ......................................................................
Patch Set 30: (32 comments) Submitting (incomplete) comments for patch set 30. https://asterix-gerrit.ics.uci.edu/#/c/1487/30/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java: PS30, Line 126: getCCConfig Does the ICCApplicationEntryPoint know about the config manager? https://asterix-gerrit.ics.uci.edu/#/c/1487/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/AsterixNCConfig.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/AsterixNCConfig.java: PS5, Line 25: Do we still have an app section? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/ClusterStateDefaultParameterTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/ClusterStateDefaultParameterTest.java: PS30, Line 68: /node/asterix_nc1/config How did the HTTP API change here? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/asterixdb/asterix-app/src/test/resources/runtimets/results/api/cluster_state_1/cluster_state_1.1.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/api/cluster_state_1/cluster_state_1.1.adm: PS30, Line 7: config Where do we find the missing info? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/CompilerProperties.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/CompilerProperties.java: PS30, Line 21: .*; Resolve '*' includes? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/ExternalProperties.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/ExternalProperties.java: PS30, Line 63: CC Is this correct? https://asterix-gerrit.ics.uci.edu/#/c/1487/5/asterixdb/asterix-doc/src/site/markdown/ncservice.md File asterixdb/asterix-doc/src/site/markdown/ncservice.md: PS5, Line 141: storage.subdir=storage I'm confused. I thought we wanted to put those in the nc-section. https://asterix-gerrit.ics.uci.edu/#/c/1487/30/asterixdb/asterix-events/src/main/resources/events/node_join/nc_join.sh File asterixdb/asterix-events/src/main/resources/events/node_join/nc_join.sh: PS30, Line 24: fix WS? PS30, Line 25: fix WS? PS30, Line 31: TODO(mblow) file an issue? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/asterixdb/asterix-events/src/main/resources/events/node_restart/nc_restart.sh File asterixdb/asterix-events/src/main/resources/events/node_restart/nc_restart.sh: PS30, Line 26: fix WS? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/algebricks/algebricks-tests/src/test/java/org/apache/hyracks/algebricks/tests/util/AlgebricksHyracksIntegrationUtil.java File hyracks-fullstack/algebricks/algebricks-tests/src/test/java/org/apache/hyracks/algebricks/tests/util/AlgebricksHyracksIntegrationUtil.java: PS30, Line 66: data line too long? PS30, Line 78: data line too long? https://asterix-gerrit.ics.uci.edu/#/c/1487/5/hyracks-fullstack/hyracks/hyracks-api/pom.xml File hyracks-fullstack/hyracks/hyracks-api/pom.xml: PS5, Line 104: Not a big fan of the args4j dependency in hyracks-api. https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/config/IOption.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/config/IOption.java: PS30, Line 24: IOption I think that we need some javadoc here - especially on "hidden" and "virtual". PS30, Line 52: defaultValue Not a big fan of default implementations - especially if they get complicated. I think that the goal of default methods is to enable interface additions without modifying all implementations and not to put a lot of code into the interface. I prefer if interface files are rarely touched and putting a lot of code into them seems to make that more unlikely. PS30, Line 67: usage Not a big fan of default implementations - especially if they get complicated. https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/context/ICCContext.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/context/ICCContext.java: PS30, Line 37: getConfigManager Do we need to get the config manager from the ICCContext? It seems that the only use of this is to get the AppConfig which might be easily (and cleanly) achievable without modifying this interface. https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/ClusterControllerService.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/ClusterControllerService.java: PS30, Line 147: aep Validate that the now necessary AEP is not null? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/OptionMarker.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/OptionMarker.java: PS30, Line 32: setter Add an example? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/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: PS30, Line 169: aep It seems that the application entry point cannot be null anymore. Shoudl we check that? Line 245: messagingNetManager = new MessagingNetworkManager(this, ncConfig.messagingListenAddress, ncConfig.messagingListenPort, > MAJOR SonarQube violation: Indeed :) https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-control/hyracks-nc-service/src/main/java/org/apache/hyracks/control/nc/service/NCService.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-nc-service/src/main/java/org/apache/hyracks/control/nc/service/NCService.java: PS30, Line 195: printStackTrace Should this stay in here or should we add an issue to remove it? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-examples/btree-example/btreehelper/src/main/java/org/apache/hyracks/examples/btree/helper/NCApplicationEntryPoint.java File hyracks-fullstack/hyracks/hyracks-examples/btree-example/btreehelper/src/main/java/org/apache/hyracks/examples/btree/helper/NCApplicationEntryPoint.java: PS30, Line 58: getNCConfig A little confused about the scopes here. The NCConfig seems to be about the NC, but this is provided by the AppContext, which should be for the App running inside the NC. https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/AbstractIntegrationTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/AbstractIntegrationTest.java: PS30, Line 107: data line too long? PS30, Line 118: data line too long? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/PredistributedJobsTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/PredistributedJobsTest.java: PS30, Line 80: data line too long? PS30, Line 92: data line too long? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-ipc/src/main/java/org/apache/hyracks/ipc/impl/IPCConnectionManager.java File hyracks-fullstack/hyracks/hyracks-ipc/src/main/java/org/apache/hyracks/ipc/impl/IPCConnectionManager.java: PS30, Line 326: SEVERE Why is this severe? Don't we intend to continue? (I guess we should agree on what those log levels mean to resolve this question ...) https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-server/docs/README File hyracks-fullstack/hyracks/hyracks-server/docs/README: PS30, Line 22: alignment seems to be broken https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-server/src/main/java/org/apache/hyracks/server/process/HyracksCCProcess.java File hyracks-fullstack/hyracks/hyracks-server/src/main/java/org/apache/hyracks/server/process/HyracksCCProcess.java: Line 44: //cList.add("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005"); > MAJOR SonarQube violation: Just revert the file? https://asterix-gerrit.ics.uci.edu/#/c/1487/30/hyracks-fullstack/hyracks/hyracks-server/src/test/java/org/apache/hyracks/server/test/NCServiceIT.java File hyracks-fullstack/hyracks/hyracks-server/src/test/java/org/apache/hyracks/server/test/NCServiceIT.java: PS30, Line 80: TODO File an issue and revert the file? -- To view, visit https://asterix-gerrit.ics.uci.edu/1487 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I95b7e0bd4538ef42817c8826e76412150074b754 Gerrit-PatchSet: 30 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Michael Blow <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
