dlmarion edited a comment on pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#issuecomment-894263638
> This doesn't quite work as I'd expect. I didn't get a chance to thoroughly
review the updates yet, but what I saw looked like it was
now simply checking that these properties were null or non-null. This will
not work as the user intends if the user says
-Daccumulo.rfile.compression.quickassist.enabled=false. That's non-null, but
clearly it shouldn't activate in that case. I think it should be activated if
it's something like value != null && (value.isEmpty() ||
Boolean.parseBoolean(value)). This should work if you just do -Dprop.enabled or
-Dprop.enabled=true, but will not work if it's not set at all, or if
-Dprop.enabled=false.
I think I got confused by your two previous statements:
> Also, to support -Duse.qat in addition to -Duse.qat=true, this logic will
need to be adjusted a bit. Could see how Maven does it, because it supports
similar flags like -DskipTests where you don't have to specify -DskipTests=true.
> Could support both sets of user expectations with values of
.enabled=[true,false,required/force]. However, considering the
forwards-compatible requirement of 1.10.2 with 1.10.1/1.10.0, it's impossible
for a user of "1.10.x" versions to expect that it will always fail if QAT isn't
available. The user might set '.enabled=force', but then need to downgrade to
1.10.1 for whatever reason, and the option is completely ignored. So, at best,
the user can only reasonably expect 1.10 versions to treat this as an
optimization, and never a requirement, unless they know for sure they are a
sufficiently recent 1.10 version.
If I'm reading this correctly now, you are saying that there should be one
property (`accumulo.rfile.compression.use.quickassist`), that could have
multiple values. I'm thinking the values should be:
* false (default)
* bestEffort - falls back to `gz` if QAT unavailable or error in setting
it up
* required - fails if QAT unavailable or error in setting it up
Edit: I guess `false` is not really useful after thinking about it. False is
the lack of one of the other values.
--
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]
