You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/08/03 12:57:08 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
dlmarion opened a new pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221
Closes #2219
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682867529
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
Review comment:
I was just doing what the others do here (mostly copy/paste). LZO does it [here](https://github.com/apache/accumulo/blob/1.10/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java#L159) and Snappy does it [here](https://github.com/apache/accumulo/blob/1.10/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java#L387)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682878545
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
Seems reasonable. Perhaps we can throw an exception instead of `return null`, so this error is explicit? Would want to make sure it is thrown early.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682884434
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
Review comment:
Oh wow. That existing code seems very broken. I'm fine with following suit with what the rest of the code does and fixing it later. It's especially concerning that we completely ignore the value in the Hadoop config when it's set there, and use our property when it's not set there. I'm not sure why we even bothered to read the value from the Hadoop config at all in that case, if it has no bearing on what we're going to do.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682882769
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
On the other hand, we should weigh the user's intent to use GZ, whether or not the optimization is available. The user has two intents, to use GZ, and to use QAT. The current behavior says if QAT isn't available, then GZ isn't available at all. I'm not sure that's a good experience for users either.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682879988
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
Review comment:
That's fine. I just wasn't sure what the "right way" to load a class anymore is. :smiley_cat: I haven't thought about classloaders in a bit. 2.1 code looks similar.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682889035
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
I'd rather fail fast, rather than fail silently. From a deployment perspective, if I configure something to run a specific way, then I want it to do that or fail so I can fix it. It's not fun or easy to diagnose why something isn't working the way you expect it to. If you specify `-Xmx8g` for the JVM, you expect it to use up to that much memory. If the JVM can't allocate that much memory it doesn't fall back to allocating whatever it can. I'd rather throw an error here and move on.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682893173
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
Yeah, I understand the desire to fail fast instead of diagnose when things aren't working as expected. I'm just thinking we have to infer what the user is expecting. I'm sure some users might expect "I want QAT's impl of GZ only". But other users might expect "I want GZ, QAT to make it faster, if available". Personally, I'd be somebody in the latter camp.
Could support both sets of user expectations with values of `.enabled=[true,false,required/force]`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682810271
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
Review comment:
Is `Class.forName` the right way to load this class for the VFS Classloader in Accumulo in 1.x? What about 2.1 with the new classloader changes there?
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -219,7 +220,21 @@ public OutputStream createCompressionStream(OutputStream downStream, Compressor
GZ(COMPRESSION_GZ) {
- private transient DefaultCodec codec = null;
+ private static final String USE_QAT_PROPERTY = "use.qat";
Review comment:
`use.qat` is succinct, but it might be better to have a more explicit name in the `accumulo.` namespace because this is for affecting Accumulo behavior, something like:
```suggestion
private static final String USE_QAT_PROPERTY = "accumulo.rfile.compression.quickassist.enabled";
```
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -231,14 +246,26 @@ public OutputStream createCompressionStream(OutputStream downStream, Compressor
*/
private static final int DEFAULT_BUFFER_SIZE = 32 * 1024;
+ private final Boolean USE_QAT =
+ Boolean.valueOf(System.getProperty(USE_QAT_PROPERTY, "false"));
Review comment:
Since this will return something non-null, you can assign it to a primitive `boolean`. 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`.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
If this is null, should we fall back to the default impl? This seems like it would result in NPEs and break things in weird ways. The QuickAssist impl should probably be an optional optimization that works when available, but is ignored when not available.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
Review comment:
I'm a little confused by this logic. This reads:
If `io.compression.codec.qat.class` is not set in the Hadoop config file, then use the class name defined by `-Dorg.apache.hadoop.io.compress.QatCodec.class=????`. This part makes sense, although I don't know why we'd want to be able to override the QAT codec class name, since it seems like it would be a well-known value.
If it **is** set in the Hadoop config file, then just set the class name to `null`, which will result in us using the default class name we've hard-coded. This part doesn't make sense to me. Why wouldn't we use the class name from the Hadoop config file in this case?
Also, shouldn't we prioritize our explicit `-D` override instead of only using it when the Hadoop config doesn't set it? The implementation here will prioritize what's in the the default value if the Hadoop config is set, and ignore the override.
I think the logic should be:
1. Check if system property is set to non-null. Use it if set, otherwise
2. check if Hadoop config is set to non-null. Use it if set, otherwise
3. use the default class name that we've hard-coded.
You could also use the first one in that order where the class isn't just non-null, but also the class loads.
Also, minor nit: avoiding unnecessary negations in ternary statements helps readability:
```java
String clazz = extClazz == null ? DEFAULT_QAT_CLASS : extClazz;
```
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
Review comment:
Is CNFE is the only ReflectiveOperationException that can be thrown here? I'm surprised if that's the case, but perhaps this is because the rest are caught in Hadoop's ReflectionUtils? (Is that really public API for Hadoop? That seems like an internal utility class that we should probably avoid using ourselves... but it's outside the scope of this PR.)
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
+ } else {
+ DefaultCodec myCodec = new DefaultCodec();
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
+ myCodec.setConf(myConf);
+ return myCodec;
+ }
}
@Override
public InputStream createDecompressionStream(InputStream downStream,
Decompressor decompressor, int downStreamBufferSize) throws IOException {
- // Set the internal buffer size to read from down stream.
- CompressionCodec decomCodec = codec;
- // if we're not using the default, let's pull from the loading cache
- if (DEFAULT_BUFFER_SIZE != downStreamBufferSize) {
- Entry<Algorithm,Integer> sizeOpt = Maps.immutableEntry(GZ, downStreamBufferSize);
- try {
- decomCodec = codecCache.get(sizeOpt);
- } catch (ExecutionException e) {
- throw new IOException(e);
+
+ if (USE_QAT) {
Review comment:
This conditional is checking that 'use.qat' system property is set to true, but not whether the QAT codec successfully loaded. The conditional should probably depend on the success of loading the QAT library, especially if we fall back to the regular codec if it fails to load, as I suggested elsewhere.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -297,7 +362,7 @@ public OutputStream createCompressionStream(OutputStream downStream, Compressor
@Override
public boolean isSupported() {
- return true;
+ return codec != null;
Review comment:
We should always support GZ, even if the QAT library fails to load.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
Seems reasonable. Perhaps we can throw an exception instead of `return null`, so this error is explicit? Would want to make sure it is thrown early.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
Review comment:
That's fine. I just wasn't sure what the "right way" to load a class anymore is. :smiley_cat: I haven't thought about classloaders in a bit. 2.1 code looks similar.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
On the other hand, we should weigh the user's intent to use GZ, whether or not the optimization is available. The user has two intents, to use GZ, and to use QAT. The current behavior says if QAT isn't available, then GZ isn't available at all. I'm not sure that's a good experience for users either.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
Review comment:
Oh wow. That existing code seems very broken. I'm fine with following suit with what the rest of the code does and fixing it later. It's especially concerning that we completely ignore the value in the Hadoop config when it's set there, and use our property when it's not set there. I'm not sure why we even bothered to read the value from the Hadoop config at all in that case, if it has no bearing on what we're going to do.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
Yeah, I understand the desire to fail fast instead of diagnose when things aren't working as expected. I'm just thinking we have to infer what the user is expecting. I'm sure some users might expect "I want QAT's impl of GZ only". But other users might expect "I want GZ, QAT to make it faster, if available". Personally, I'd be somebody in the latter camp.
Could support both sets of user expectations with values of `.enabled=[true,false,required/force]`.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
Yeah, I understand the desire to fail fast instead of diagnose when things aren't working as expected. I'm just thinking we have to infer what the user is expecting. I'm sure some users might expect "I want QAT's impl of GZ only". But other users might expect "I want GZ, QAT to make it faster, if available". Personally, I'd be somebody in the latter camp.
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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#issuecomment-1044422240
Closing w/out merging, see #2219 for details.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682865385
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
Review comment:
I just copied what the other implementations were doing. For Hadoop to be able to use the Codec it has to be on the CLASSPATH.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682864525
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
I was thinking that if someone specified `-Duse.qat`, then they were intending on using the device and expected it to work. The fact that it was not working in this case would be an error as maybe they have a configuration issue. Falling back to the default implementation and logging it is not honoring the users intent.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#issuecomment-892886656
I commented on a few things above, but it seems that some of your suggestions apply also to code that is in this class but outside of this PR. I'm wondering if we should first implement some of your suggestions in the class, then update this PR accordingly.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#issuecomment-892886656
I commented on a few things above, but it seems that some of your suggestions apply also to code that is in this class but outside of this PR. I'm wondering if we should first implement some of your suggestions in the class, then update this PR accordingly.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion edited a comment on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#issuecomment-893391587
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. I also made some other mods based on the other comments. 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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#issuecomment-892008577
@ctubbsii suggested re-working this PR to update the gzip codec to try and use the QAT if it's available instead of creating a new codec iff the QAT only performs gzip compression.
According to the codec [documentation](https://github.com/Intel-bigdata/IntelQATCodec) the QATCodec depends on the QATZip library. [QATZip](https://github.com/intel/QATzip) says it "produces data using the standard gzip* format (RFC1952) with extended headers. The data can be decompressed with a compliant gzip* implementation"
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682810271
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
Review comment:
Is `Class.forName` the right way to load this class for the VFS Classloader in Accumulo in 1.x? What about 2.1 with the new classloader changes there?
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -219,7 +220,21 @@ public OutputStream createCompressionStream(OutputStream downStream, Compressor
GZ(COMPRESSION_GZ) {
- private transient DefaultCodec codec = null;
+ private static final String USE_QAT_PROPERTY = "use.qat";
Review comment:
`use.qat` is succinct, but it might be better to have a more explicit name in the `accumulo.` namespace because this is for affecting Accumulo behavior, something like:
```suggestion
private static final String USE_QAT_PROPERTY = "accumulo.rfile.compression.quickassist.enabled";
```
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -231,14 +246,26 @@ public OutputStream createCompressionStream(OutputStream downStream, Compressor
*/
private static final int DEFAULT_BUFFER_SIZE = 32 * 1024;
+ private final Boolean USE_QAT =
+ Boolean.valueOf(System.getProperty(USE_QAT_PROPERTY, "false"));
Review comment:
Since this will return something non-null, you can assign it to a primitive `boolean`. 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`.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
If this is null, should we fall back to the default impl? This seems like it would result in NPEs and break things in weird ways. The QuickAssist impl should probably be an optional optimization that works when available, but is ignored when not available.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
Review comment:
I'm a little confused by this logic. This reads:
If `io.compression.codec.qat.class` is not set in the Hadoop config file, then use the class name defined by `-Dorg.apache.hadoop.io.compress.QatCodec.class=????`. This part makes sense, although I don't know why we'd want to be able to override the QAT codec class name, since it seems like it would be a well-known value.
If it **is** set in the Hadoop config file, then just set the class name to `null`, which will result in us using the default class name we've hard-coded. This part doesn't make sense to me. Why wouldn't we use the class name from the Hadoop config file in this case?
Also, shouldn't we prioritize our explicit `-D` override instead of only using it when the Hadoop config doesn't set it? The implementation here will prioritize what's in the the default value if the Hadoop config is set, and ignore the override.
I think the logic should be:
1. Check if system property is set to non-null. Use it if set, otherwise
2. check if Hadoop config is set to non-null. Use it if set, otherwise
3. use the default class name that we've hard-coded.
You could also use the first one in that order where the class isn't just non-null, but also the class loads.
Also, minor nit: avoiding unnecessary negations in ternary statements helps readability:
```java
String clazz = extClazz == null ? DEFAULT_QAT_CLASS : extClazz;
```
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
Review comment:
Is CNFE is the only ReflectiveOperationException that can be thrown here? I'm surprised if that's the case, but perhaps this is because the rest are caught in Hadoop's ReflectionUtils? (Is that really public API for Hadoop? That seems like an internal utility class that we should probably avoid using ourselves... but it's outside the scope of this PR.)
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
+ } else {
+ DefaultCodec myCodec = new DefaultCodec();
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
+ myCodec.setConf(myConf);
+ return myCodec;
+ }
}
@Override
public InputStream createDecompressionStream(InputStream downStream,
Decompressor decompressor, int downStreamBufferSize) throws IOException {
- // Set the internal buffer size to read from down stream.
- CompressionCodec decomCodec = codec;
- // if we're not using the default, let's pull from the loading cache
- if (DEFAULT_BUFFER_SIZE != downStreamBufferSize) {
- Entry<Algorithm,Integer> sizeOpt = Maps.immutableEntry(GZ, downStreamBufferSize);
- try {
- decomCodec = codecCache.get(sizeOpt);
- } catch (ExecutionException e) {
- throw new IOException(e);
+
+ if (USE_QAT) {
Review comment:
This conditional is checking that 'use.qat' system property is set to true, but not whether the QAT codec successfully loaded. The conditional should probably depend on the success of loading the QAT library, especially if we fall back to the regular codec if it fails to load, as I suggested elsewhere.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -297,7 +362,7 @@ public OutputStream createCompressionStream(OutputStream downStream, Compressor
@Override
public boolean isSupported() {
- return true;
+ return codec != null;
Review comment:
We should always support GZ, even if the QAT library fails to load.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682893173
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
Yeah, I understand the desire to fail fast instead of diagnose when things aren't working as expected. I'm just thinking we have to infer what the user is expecting. I'm sure some users might expect "I want QAT's impl of GZ only". But other users might expect "I want GZ, QAT to make it faster, if available". Personally, I'd be somebody in the latter camp.
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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion closed pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion closed pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#discussion_r682864525
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
I was thinking that if someone specified `-Duse.qat`, then they were intending on using the device and expected it to work. The fact that it was not working in this case would be an error as maybe they have a configuration issue. Falling back to the default implementation and logging it is not honoring the users intent.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
Review comment:
I just copied what the other implementations were doing. For Hadoop to be able to use the Codec it has to be on the CLASSPATH.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
Review comment:
I was just doing what the others do here (mostly copy/paste). LZO does it [here](https://github.com/apache/accumulo/blob/1.10/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java#L159) and Snappy does it [here](https://github.com/apache/accumulo/blob/1.10/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java#L387)
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
+ LOG.error("Unable to create QAT codec", e);
+ }
+ return null;
Review comment:
I'd rather fail fast, rather than fail silently. From a deployment perspective, if I configure something to run a specific way, then I want it to do that or fail so I can fix it. It's not fun or easy to diagnose why something isn't working the way you expect it to. If you specify `-Xmx8g` for the JVM, you expect it to use up to that much memory. If the JVM can't allocate that much memory it doesn't fall back to allocating whatever it can. I'd rather throw an error here and move on.
##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -250,33 +277,71 @@ public void initializeDefaultCodec() {
*/
@Override
protected CompressionCodec createNewCodec(final int bufferSize) {
- DefaultCodec myCodec = new DefaultCodec();
- Configuration myConf = new Configuration(conf);
- // only use the buffersize if > 0, otherwise we'll use
- // the default defined within the codec
- if (bufferSize > 0)
- myConf.setInt(BUFFER_SIZE_OPT, bufferSize);
- myCodec.setConf(myConf);
- return myCodec;
+ if (USE_QAT) {
+ String extClazz =
+ (conf.get(CONF_QAT_CLASS) == null ? System.getProperty(CONF_QAT_CLASS) : null);
+ String clazz = (extClazz != null) ? extClazz : DEFAULT_QAT_CLASS;
+ try {
+ LOG.info("Trying to load qat codec class: " + clazz);
+
+ Configuration myConf = new Configuration(conf);
+ // only use the buffersize if > 0, otherwise we'll use
+ // the default defined within the codec
+ if (bufferSize > 0)
+ myConf.setInt(QAT_BUFFER_SIZE_OPT, bufferSize);
+
+ CompressionCodec c =
+ (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), myConf);
+ if (c instanceof Configurable) {
+ ((Configurable) c).setConf(myConf);
+ }
+ return c;
+ } catch (ClassNotFoundException e) {
Review comment:
ReflectionUtils is public. The newInstance method actually catches all Exceptions and throws a RuntimeException. The CNFE exception here is being thrown from the Class.forName() call.
```
@InterfaceAudience.Public
@InterfaceStability.Evolving
public class ReflectionUtils
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented 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
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2221: Add support for Intel® QuickAssist Hadoop Codec
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2221:
URL: https://github.com/apache/accumulo/pull/2221#issuecomment-891835852
In case there are any questions about the default buffer size being 256K, I used the default value defined in [QATConfigurationKeys](https://github.com/Intel-bigdata/IntelQATCodec/blob/master/hadoop_qat_wrapper/src/main/java/org/apache/hadoop/io/compress/conf/QatConfigurationKeys.java)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org