ctubbsii commented on pull request #2221: URL: https://github.com/apache/accumulo/pull/2221#issuecomment-894018608
> I modified the code to attempt to load the QATCodec if the property `accumulo.rfile.compression.quickassist.enabled` exists. If there is an issue loading the codec, then it will fall back to the default codec if the property `accumulo.rfile.compression.quickassist.fallback.on.failure` exists. 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`. Also, I'm not sure it's good to require users want to optionally use it to set two properties. It should be easy for users who just want to use it as an optional optimization to do so with just one property. > I did not fix or improve the code where I used the same pattern that already exists in other places in the class. I think that should be a different issue on the main branch. That seems reasonable. However, given that there seem to be many existing problems in this class, I'm thinking we should focus on polishing it in the main branch to solidify the behavior before backporting it. I found a few other problems, such as our code trying to defer to the property "io.compression.codec.snappy.class" in the Hadoop config file... but there's no such property in Hadoop's config. LZO is the only codec whose implementation seems to be pluggable by class name in a property. So, it also doesn't make sense to look for a corresponding "io.compression.codec.qat.class" property either (also, it seems like it should have been an "io.compression.codec.gz.class" property set to the QAT implementation... but that property also doesn't exist in Hadoop... if it did, users activating it would be super simple and easy to do out of the box without our needing to do anything special). -- 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]
