Chris Hillery has posted comments on this change. Change subject: Overhaul of Hyracks configuration management. ......................................................................
Patch Set 4: (5 comments) https://asterix-gerrit.ics.uci.edu/#/c/336/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/application/IApplicationConfig.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/application/IApplicationConfig.java: Line 26: int getInt(String section, String key); > Do we have a convention for parameter names? I think camel-case is pretty This is referencing default_value, which should be defaultValue. https://asterix-gerrit.ics.uci.edu/#/c/336/4/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: Line 283: // QQQ reasonable? > Is IPv6 not supported by Hyracks? Perhaps this should be InetAddress.getLo probably InetAddress.getLoopbackAddress().getHostAddress(). This problem exists in a few other places I introduced that have "127.0.0.1" as a fallback. https://asterix-gerrit.ics.uci.edu/#/c/336/4/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/work/TriggerNCWork.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/work/TriggerNCWork.java: Line 57: oos.writeUTF("hyncmagic"); > Constant? Move this to a constant field in this file, and include comment that it must match corresponding constant in nc proper. https://asterix-gerrit.ics.uci.edu/#/c/336/4/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NCDriver.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NCDriver.java: Line 38: System.err.println(e.getMessage()); > In general, Throwable.getMessage() does not yield useful information. Best Probably should just remove this line, since e.printStrackTrace() does all that. https://asterix-gerrit.ics.uci.edu/#/c/336/4/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: Line 93: // QQQ inheriting probably isn't right > What will you do with the output? Is there any use of stdin by the NC? NC output needs to go somewhere. Probably best to allow (via nc service config file) the option to put this into a file, but lacking that, stdout and stderr should be inherited so that the ncservice's own output is the output of the NC. -- To view, visit https://asterix-gerrit.ics.uci.edu/336 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie3027c8c839f25ea858790bd3340187f4b11f212 Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Chris Hillery <c...@lambda.nu> Gerrit-Reviewer: Chris Hillery <c...@lambda.nu> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <michael.b...@couchbase.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-HasComments: Yes
