Michael Blow has posted comments on this change. Change subject: Configuration Revamp ......................................................................
Patch Set 38: (12 comments) https://asterix-gerrit.ics.uci.edu/#/c/1487/38/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplicationEntryPoint.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplicationEntryPoint.java: Line 162: if (systemState == SystemState.PERMANENT_DATA_LOSS && (nodeProperties.isInitialRun() || nodeProperties.isVirtualNc())) { > indeed Stupid rebasing. Done. PS38, Line 210: registerVirtualNode > Why are we registering a virtual node here? I think that I don't understand That makes two of us- but this is in the block where the node is a substitute node, not a known node. I only got to this code path due to a bug- this is virtual in that it is not started by way of an NCService. The terminology is not great. https://asterix-gerrit.ics.uci.edu/#/c/1487/38/asterixdb/asterix-app/src/test/java/org/apache/asterix/common/config/ConfigUsageTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/common/config/ConfigUsageTest.java: PS38, Line 49: generateUsageCSV > Could we add some kind of result validation (e.g. number of lines in the fi someday. (added TODO) https://asterix-gerrit.ics.uci.edu/#/c/1487/38/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/MetadataProperties.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/MetadataProperties.java: PS38, Line 36: Option > There seems to be a lot of boilerplate in the methods. Is there a way to av We could consider moving the state out from the enum instance, enabling us to relocate the boilerplate methods to the interface if there is ever a need to extend, but I think we should safe that for a future change. https://asterix-gerrit.ics.uci.edu/#/c/1487/38/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/utils/AppContextInfo.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/utils/AppContextInfo.java: Line 114: final ClusterControllerService controllerService = (ClusterControllerService) ccAppCtx.getControllerService(); > Can we indeed remove this? Done https://asterix-gerrit.ics.uci.edu/#/c/1487/38/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/config/IConfigManager.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/config/IConfigManager.java: PS38, Line 26: IConfigManager > Some javadocs would be nice, maybe file an issue? Done https://asterix-gerrit.ics.uci.edu/#/c/1487/38/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigManager.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigManager.java: Line 148: private Map<IOption, Object> newNode(String nodeId) { > This is a little confusing. The method is private and the parameter is not Done https://asterix-gerrit.ics.uci.edu/#/c/1487/38/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigUtils.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigUtils.java: PS38, Line 83: getIniArray > unused? Done PS38, Line 99: getStringArray > unused? Done PS38, Line 177: camelToConstantCase > unused? Done PS38, Line 194: findOptionByName > unused? Done https://asterix-gerrit.ics.uci.edu/#/c/1487/38/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/controllers/CCConfig.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/controllers/CCConfig.java: Line 166: private final IApplicationConfig appConfig; > Seems to be a good suggestion. Done -- 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: 38 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Michael Blow <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
