ctubbsii commented on code in PR #5111:
URL: https://github.com/apache/accumulo/pull/5111#discussion_r1864376710
##########
assemble/conf/default-group-env.sh:
##########
Review Comment:
> to something like:
>
> ```
> if [[ -f "${conf}/group-overrides-env.sh" ]]; then
> #shellcheck source=../conf/group-overrides-env.sh
> source "${conf}/group-overrides-env.sh" "${group}" "${service}"
> fi
> ```
Just moving the word "group" to the front of the filename would help with
organization. Having a single file helps further by avoiding clutter entirely,
so that's definitely a big improvement.
> > 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.
(For what it's worth, I would prefer we use a more standard
`JDK_JAVA_OPTIONS` and get rid of both of these two, but that annoyingly prints
a "Picked up..." message whenever java is run.)
I was actually thinking that, for the java opts, the accumulo-service script
already sets up several environment variables to inject into the existing
accumulo-env.sh script. So, a simple solution would be to have it get the
group, and export it prior to sourcing accumulo-env.sh, and then let the user
use that information in their existing accumulo-env.sh, just like they can do
today with the service name. In fact, it's already part of the
ACCUMULO_SERVICE_INSTANCE that is exported prior to sourcing the
accumulo-env.sh, so technically, accumulo-env.sh already has the information it
needs to do this, and can already use the group information to set up
additional group-specific options.
For the property overrides, I don't really understand the per-group use case
for that, but when the property override feature was originally implemented, I
had asked for it to be implemented using Java system properties, which could be
parsed directly by commons-configuration as a composite configuration that
overrides the accumulo.properties (aka "SiteConfiguration"). However, Mike
chose to implement it as command-line arguments instead, using the `-o
propname=value`. If it had used Java system properties, then those property
overrides could have been done with `-Daccumumulo.propname=value` and used as
part of the regular `JAVA_OPTS` anywhere on the command-line, rather than being
required to be after the main class as main's arguments. It would make it much
easier to implement this if it had been done using system properties. That's
still something that we could do, and I think it would make this better. After
all, env config is much more flexible than command-line arguments, be
cause env can be injected in more ways.
Another possibility for the property overrides is to realize that
commons-configuration can use include statements and can interpolate
environment variables and system properties. So, instead of using command-line
overrides, you can probably just do something like this in your
`accumulo.properties` file:
```ini
includeOptional = group-${sys:accumulo.group}.properties
```
See:
*
https://commons.apache.org/proper/commons-configuration/userguide/howto_basicfeatures.html
*
https://commons.apache.org/proper/commons-configuration/userguide/howto_properties.html
Of course, this would mean that property overrides would be handled in
commons-config config files, and env overrides would be handled in the
accumulo-env.sh script... but I think that's okay, since that's where things go
now, and it works nicely into our existing system.
--
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]
