dlmarion commented on code in PR #5111: URL: https://github.com/apache/accumulo/pull/5111#discussion_r1865944886
########## assemble/conf/default-group-env.sh: ########## Review Comment: Addressing some concerns here... > I think I liked the https://github.com/apache/accumulo/pull/5099 solution to add the feature to the cluster yaml, rather than have multiple config files filling the config directory. The issue that I ran into with #5099 was that it took a lot of code changes to add one more thing. For example, in #5099 the changes in cluster.yaml enabled the user to set the conf directory and extra arguments per group. One item I received as feedback was that it would be nice to override the JVM args as well. You can see that #5099 modified 11 files, so adding this one thing caused a lot of changes. >>> I'm also wondering if this capability is already possible (or nearly possible, with slight tweaks) using the existing accumulo-env.sh, which optionally could source additional files, if the user wanted to break it out by resource group, without hard-coding that behavior and file naming conventions into that script. >> Maybe? I think we would need to change the way that JAVA_OPTS is built in accumulo-env.sh, appending ACCUMULO_JAVA_OPTS to JAVA_OPTS later in the script. > I'm not sure how doing it later in the script changes anything. After it is set, all other options are prepended, so its contents are always at the end anyway. The intent was always to make ACCUMULO_JAVA_OPTS in the environment override what's in the script by ensuring its content is at the end. I think the issue is that `ACCUMULO_JAVA_OPTS` has to be set in `accumulo-service` **before** calling `accumulo` because both `accumulo-service` and `accumulo` source the `accumulo-env.sh` script (although I'm not actually sure why `accumulo-service` sources it). We could move the get_group function from `accumulo-service` to `accumulo`, and if called before `accumulo-env.sh` is source we would then have both the group and the service being started ($cmd in `accumulo`) . This would allow the user to modify JAVA_OPTS however they wanted in `accumulo-env.sh`. We would still need a way to provide extra arguments to the command, and you can't push more items into the `$@` array. > For the property overrides, I don't really understand the per-group use case for that Enabling the user to provide per-group per-process JVM options and additional arguments allows them to have some base configuration in $ACCUMULO_CONF_DIR from which to build. The approach that I started off with in #5099 was to have separate $ACCUMULO_CONF_DIR's, possibly one for each resource group. While #5099 seemed like a good idea at first, it became apparent that it would be hard to diff the configurations and easy for configuration drift to occur. -- 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]
