Till Westmann has posted comments on this change. Change subject: Configuration Revamp ......................................................................
Patch Set 38: (14 comments) https://asterix-gerrit.ics.uci.edu/#/c/1487/38//COMMIT_MSG Commit Message: PS38, Line 9: of keep width to 77 chars 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())) { > MAJOR SonarQube violation: indeed PS38, Line 210: registerVirtualNode Why are we registering a virtual node here? I think that I don't understand the lifecycle here. 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 file, existence of options in the file)? 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 avoid that (while decreasing the code size)? https://asterix-gerrit.ics.uci.edu/#/c/1487/35/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(); > MAJOR SonarQube violation: One more? 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(); > MAJOR SonarQube violation: Can we indeed remove this? 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? 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) { > MAJOR SonarQube violation: This is a little confusing. The method is private and the parameter is not used. 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? PS38, Line 99: getStringArray unused? PS38, Line 177: camelToConstantCase unused? PS38, Line 194: findOptionByName unused? 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; > CRITICAL SonarQube violation: Seems to be a good suggestion. -- 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
