ctubbsii commented on code in PR #5865:
URL: https://github.com/apache/accumulo/pull/5865#discussion_r2357230298
##########
assemble/bin/accumulo-cluster:
##########
@@ -305,8 +298,19 @@ function parse_config() {
trap 'rm -rf -- "$AC_TMP_DIR"' EXIT
fi
+ RG_FILE="$AC_TMP_DIR/ZooInfoViewer.out"
+ if isDebug; then
+ debug "Printing resource groups to $RG_FILE"
+ fi
+ "$accumulo_cmd" "zoo-info-viewer" "--print-resource-groups" >"$RG_FILE" ||
exit 1
+ read -r -a all_resource_groups <<<"$(grep "Resource Groups: " "$RG_FILE" |
sed "s/Resource Groups: //")"
+ if isDebug; then
+ debug "All resource groups: ${all_resource_groups[*]}"
+ fi
Review Comment:
```suggestion
debug "All resource groups: ${all_resource_groups[*]}"
```
##########
assemble/bin/accumulo-cluster:
##########
@@ -305,8 +298,19 @@ function parse_config() {
trap 'rm -rf -- "$AC_TMP_DIR"' EXIT
fi
+ RG_FILE="$AC_TMP_DIR/ZooInfoViewer.out"
+ if isDebug; then
+ debug "Printing resource groups to $RG_FILE"
+ fi
+ "$accumulo_cmd" "zoo-info-viewer" "--print-resource-groups" >"$RG_FILE" ||
exit 1
+ read -r -a all_resource_groups <<<"$(grep "Resource Groups: " "$RG_FILE" |
sed "s/Resource Groups: //")"
Review Comment:
cut is faster than sed here, and so is `-F` since you don't need a regex:
```suggestion
read -r -a all_resource_groups <<<"$(grep -F 'Resource Groups: '
"$RG_FILE" | cut -c18-)"
```
directly setting the array is probably fastest, but shellcheck might
complain about this:
```suggestion
all_resource_groups=($(grep -F 'Resource Groups: ' "$RG_FILE" | cut -c18-))
```
I guess you could always suppress the complaint if it did.
Also, this assumes space-separated input. The output emits comma-separated
as currently written.
##########
assemble/bin/accumulo-cluster:
##########
@@ -305,8 +298,19 @@ function parse_config() {
trap 'rm -rf -- "$AC_TMP_DIR"' EXIT
fi
+ RG_FILE="$AC_TMP_DIR/ZooInfoViewer.out"
+ if isDebug; then
+ debug "Printing resource groups to $RG_FILE"
+ fi
Review Comment:
isDebug is redundant if all you're doing is echoing using the debug function
```suggestion
debug "Printing resource groups to $RG_FILE"
```
##########
assemble/bin/accumulo-cluster:
##########
@@ -305,8 +298,19 @@ function parse_config() {
trap 'rm -rf -- "$AC_TMP_DIR"' EXIT
fi
+ RG_FILE="$AC_TMP_DIR/ZooInfoViewer.out"
+ if isDebug; then
+ debug "Printing resource groups to $RG_FILE"
+ fi
+ "$accumulo_cmd" "zoo-info-viewer" "--print-resource-groups" >"$RG_FILE" ||
exit 1
Review Comment:
A small improvement, but single-quote avoids unnecessary bash work if you
don't have any variable interpolation, or you can omit quotes entirely if there
are no special characters to worry about:
```suggestion
"$accumulo_cmd" zoo-info-viewer --print-resource-groups >"$RG_FILE" ||
exit 1
```
##########
core/src/main/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParser.java:
##########
@@ -250,42 +215,25 @@ public static void outputShellVariables(ServerContext
ctx, Map<String,String> co
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN",
justification = "Path provided for output file is intentional")
public static void main(String[] args) throws IOException {
- if (args == null || args.length < 1 || args.length > 3) {
- System.err.println(
- "Usage: ClusterConfigParser <createMissingResourceGroups>
<configFile> [<outputFile>]");
+
+ if (args == null || args.length < 1 || args.length > 2) {
+ System.err.println("Usage: ClusterConfigParser <configFile>
[<outputFile>]");
System.exit(1);
}
- if (siteConf == null) {
- siteConf = SiteConfiguration.auto();
- }
- try (var context = new ServerContext(siteConf)) {
- // Login as the server on secure HDFS
- if (siteConf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
- SecurityUtil.serverLogin(siteConf);
- }
- final Set<String> zkGroups = new HashSet<>();
- context.resourceGroupOperations().list().forEach(rg ->
zkGroups.add(rg.canonical()));
- if (!zkGroups.contains(ResourceGroupId.DEFAULT.canonical())) {
- throw new IllegalStateException("Default resource group not found in
ZooKeeper");
- }
- boolean createMissingRG = args[0].equals("true") ? true : false;
- try {
- if (args.length == 3) {
- // Write to a file instead of System.out if provided as an argument
- try (OutputStream os = Files.newOutputStream(Path.of(args[2]));
- PrintStream out = new PrintStream(os)) {
- outputShellVariables(context,
parseConfiguration(Path.of(args[1])), zkGroups,
- createMissingRG, out);
- }
- } else {
- outputShellVariables(context, parseConfiguration(Path.of(args[1])),
zkGroups,
- createMissingRG, System.out);
+ try {
+ if (args.length == 2) {
+ // Write to a file instead of System.out if provided as an argument
+ try (OutputStream os = Files.newOutputStream(Path.of(args[1]));
+ PrintStream out = new PrintStream(os)) {
+ outputShellVariables(parseConfiguration(Path.of(args[0])), out);
}
- } catch (Exception e) {
- System.err.println("Processing error: " + e.getMessage());
- throw e;
+ } else {
+ outputShellVariables(parseConfiguration(Path.of(args[0])), System.out);
}
+ } catch (Exception e) {
+ System.err.println("Processing error: " + e.getMessage());
+ System.exit(1);
Review Comment:
I prefer throwing, though I suppose in this class, it probably doesn't
matter. But, it's definitely harder to debug without the stack trace.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooInfoViewer.java:
##########
@@ -328,6 +334,14 @@ private void printInstanceIds(final Map<String,InstanceId>
instanceIdMap, PrintW
writer.println();
}
+ private void printResourceGroups(ServerContext context, PrintWriter writer) {
+ Set<String> groups = new TreeSet<>();
+
context.getZooCache().getChildren(Constants.ZRESOURCEGROUPS).forEach(groups::add);
+ String rgs = String.join(",", groups.toArray(new String[] {}));
Review Comment:
This looks like it prints comma-separated. The script is looking for
space-separated.
##########
assemble/bin/accumulo-cluster:
##########
@@ -305,8 +298,19 @@ function parse_config() {
trap 'rm -rf -- "$AC_TMP_DIR"' EXIT
fi
+ RG_FILE="$AC_TMP_DIR/ZooInfoViewer.out"
+ if isDebug; then
+ debug "Printing resource groups to $RG_FILE"
+ fi
+ "$accumulo_cmd" "zoo-info-viewer" "--print-resource-groups" >"$RG_FILE" ||
exit 1
+ read -r -a all_resource_groups <<<"$(grep "Resource Groups: " "$RG_FILE" |
sed "s/Resource Groups: //")"
+ if isDebug; then
+ debug "All resource groups: ${all_resource_groups[*]}"
+ fi
+ rm -f "$RG_FILE"
Review Comment:
You don't need to delete this file. It's in the temp directory, which gets
cleaned up by the `trap` (or left behind for debugging). I noticed that the
`ClusterConfigParser.out` file is also being deleted the same way, and it
doesn't need to be (probably shouldn't be, so we can debug if the script fails
to exit normally).
```suggestion
```
##########
assemble/bin/accumulo-cluster:
##########
@@ -328,6 +332,10 @@ function parse_config() {
echo "$(yellow WARN): No compactor groups configured"
else
for group in $COMPACTOR_GROUPS; do
+ if ! ${all_resource_groups[$group]}; then
Review Comment:
You can't test array contents this way. You have to create a function like:
```bash
function arrayContains() {
local m=$1
shift
local x
for x in "$@"; do [[ $x == $m ]] && return 0; done
return 1
}
```
Then, you can call it like:
```bash
if ! arrayContains "$group" "${all_resource_groups[@]}"; then
echo not found message here
fi
```
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -537,6 +562,11 @@ public void execute(final String[] args) {
if (success && opts.addVolumes) {
success = addVolumes(fs, initConfig, serverDirs);
}
+ if (success && opts.resourceGroups != null) {
+ try (var zk = new ZooSession(getClass().getSimpleName(), siteConfig)) {
+ success = addResourceGroups(zk.asReaderWriter(),
opts.resourceGroups);
Review Comment:
This is not a chrooted ZooSession. The paths are going to be wrong. This
should probably use a ServerContext (see the `resetSecurity` method for a
similar requirement and similar validation checks to ensure that the system,
especially ZK, is already initialized).
##########
assemble/bin/accumulo-cluster:
##########
@@ -305,8 +298,19 @@ function parse_config() {
trap 'rm -rf -- "$AC_TMP_DIR"' EXIT
fi
+ RG_FILE="$AC_TMP_DIR/ZooInfoViewer.out"
+ if isDebug; then
+ debug "Printing resource groups to $RG_FILE"
+ fi
+ "$accumulo_cmd" "zoo-info-viewer" "--print-resource-groups" >"$RG_FILE" ||
exit 1
+ read -r -a all_resource_groups <<<"$(grep "Resource Groups: " "$RG_FILE" |
sed "s/Resource Groups: //")"
+ if isDebug; then
+ debug "All resource groups: ${all_resource_groups[*]}"
+ fi
+ rm -f "$RG_FILE"
+
CONFIG_FILE="$AC_TMP_DIR/ClusterConfigParser.out"
- "$accumulo_cmd" org.apache.accumulo.server.conf.cluster.ClusterConfigParser
"$ARG_CREATE_MISSING_RG" "$conf/cluster.yaml" "$CONFIG_FILE" || parse_fail
+ "$accumulo_cmd" org.apache.accumulo.core.conf.cluster.ClusterConfigParser
"$conf/cluster.yaml" "$CONFIG_FILE" || parse_fail
Review Comment:
I guess it doesn't really matter where this lives, but the cluster scripts
are really a server-side thing, since they start/stop server processes. It's
not something that really needs to be in the core module for sharing. I think
it's fine to leave it in server/base, where most of the other server/admin
utils live.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
