dlmarion commented on code in PR #5743:
URL: https://github.com/apache/accumulo/pull/5743#discussion_r2215952364


##########
core/src/main/java/org/apache/accumulo/core/file/rfile/RFileOperations.java:
##########
@@ -136,10 +137,45 @@ protected FileSKVWriter openWriter(FileOptions options) 
throws IOException {
       String file = options.getFilename();
       FileSystem fs = options.getFileSystem();
 
-      if (options.dropCacheBehind) {
+      var ecEnable = EcEnabled.valueOf(
+          
options.getTableConfiguration().get(Property.TABLE_ENABLE_ERASURE_CODES).toUpperCase());
+
+      if (fs instanceof DistributedFileSystem) {
+        var builder = ((DistributedFileSystem) fs).createFile(new 
Path(file)).bufferSize(bufferSize)
+            .blockSize(block).overwrite(false);
+
+        if (options.dropCacheBehind) {
+          builder = builder.syncBlock();
+        }
+
+        switch (ecEnable) {
+          case ENABLE:
+            String ecPolicyName =
+                
options.getTableConfiguration().get(Property.TABLE_ERASURE_CODE_POLICY);
+            builder = builder.ecPolicyName(ecPolicyName);
+            break;
+          case DISABLE:
+            // force replication
+            builder = builder.replication((short) rep).replicate();
+            break;
+          case DIR:

Review Comment:
   I'm thinking we could get rid of this case, and also remove the 
`replicate()` call from the DISABLE case.



##########
server/base/src/main/java/org/apache/accumulo/server/util/PropUtil.java:
##########
@@ -74,7 +77,28 @@ protected static void validateProperties(final ServerContext 
context,
               "Unable to resolve classloader for context: " + prop.getValue());
         }
       }
+
+      if (prop.getKey().equals(Property.TABLE_ERASURE_CODE_POLICY.getKey())) {

Review Comment:
   Should we fail if TABLE_FILE_REPLICATION is set to a non-zero value?



##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1412,6 +1412,19 @@ public enum Property {
           + "constraint.",
       "2.0.0"),
 
+  TABLE_ENABLE_ERASURE_CODES("table.file.ec", "dir", PropertyType.EC,

Review Comment:
   I wonder if we need `DIR` for the user facing property. For whatever backing 
store is being used, there is a default behavior. For HDFS, it's replication. 
`enable` here is overriding the default behavior and opting-in to EC. `disable` 
here allows the user to change the value at runtime, but `disable` I think just 
means use the default behavior.



-- 
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-unsubscr...@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to