You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2023/06/18 19:02:49 UTC

[GitHub] [accumulo] cshannon opened a new pull request, #3506: Validate file metadata in MetadataConstraints

cshannon opened a new pull request, #3506:
URL: https://github.com/apache/accumulo/pull/3506

   MetadataConstraints will now validate mutations for updates of columns that store file metadata. A StoredTabletFile is created which will parse the metadata and eagerly load a ReferencedTabletFile to perform the validation checks.
   
   Tests have been added to verify that `DataFileColumnFamily`, `ScanFileColumnFamily`, and `DataFileColumnFamily` are all validated on updates.
   
   I added a flag to the StoredTabletFile class to allow an easy way to eagerly load the ReferencedTabletFile as I thought this may come in handy in the future for other use cases where we know we want to load the file immediately and validate. The other option of course is to not provide that flag and instead just call the getter on the constructed StoredTabletFile. If people prefer that approach instead that is fine too.
   
   There is some more work to be done for validating files but I figure more can be added for the follow on issue #3505 ,including GC delete markers.


-- 
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] cshannon commented on a diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1247989164


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,23 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata by creating a StoredTabletFile for the metadata string that
+   * will eagerly load the contained ReferencedTabletFile which performs validation.
+   *
+   * We have to catch a generic Exception as various errors can occur including
+   * NullPointerException, IllegalArgumentException, etc.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      new StoredTabletFile(metadata);

Review Comment:
   #3540 is merged so I can look at creating a validate method and updating this PR.



-- 
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 diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1236993772


##########
server/base/src/test/java/org/apache/accumulo/server/constraints/MetadataConstraintsTest.java:
##########
@@ -202,60 +211,113 @@ public void testBulkFileCheck() {
 
     // two files w/ one active txid
     m = new Mutation(new Text("0;foo"));
-    m.put(BulkFileColumnFamily.NAME, new Text("/someFile"), new Value("5"));
-    m.put(DataFileColumnFamily.NAME, new Text("/someFile"),
+    m.put(BulkFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"),
+        new Value("5"));
+    m.put(DataFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"),
         new DataFileValue(1, 1).encodeAsValue());
-    m.put(BulkFileColumnFamily.NAME, new Text("/someFile2"), new Value("5"));
-    m.put(DataFileColumnFamily.NAME, new Text("/someFile2"),
+    m.put(BulkFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile2"),
+        new Value("5"));
+    m.put(DataFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile2"),
         new DataFileValue(1, 1).encodeAsValue());
     violations = mc.check(createEnv(), m);
     assertNull(violations);
 
     // two loaded w/ one active txid and one file
     m = new Mutation(new Text("0;foo"));
-    m.put(BulkFileColumnFamily.NAME, new Text("/someFile"), new Value("5"));
-    m.put(DataFileColumnFamily.NAME, new Text("/someFile"),
+    m.put(BulkFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"),
+        new Value("5"));
+    m.put(DataFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"),
         new DataFileValue(1, 1).encodeAsValue());
-    m.put(BulkFileColumnFamily.NAME, new Text("/someFile2"), new Value("5"));
+    m.put(BulkFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile2"),
+        new Value("5"));
     violations = mc.check(createEnv(), m);
     assertNotNull(violations);
     assertEquals(1, violations.size());
     assertEquals(Short.valueOf((short) 8), violations.get(0));
 
     // active txid, mutation that looks like split
     m = new Mutation(new Text("0;foo"));
-    m.put(BulkFileColumnFamily.NAME, new Text("/someFile"), new Value("5"));
+    m.put(BulkFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"),
+        new Value("5"));
     ServerColumnFamily.DIRECTORY_COLUMN.put(m, new Value("/t1"));
     violations = mc.check(createEnv(), m);
     assertNull(violations);
 
     // inactive txid, mutation that looks like split
     m = new Mutation(new Text("0;foo"));
-    m.put(BulkFileColumnFamily.NAME, new Text("/someFile"), new Value("12345"));
+    m.put(BulkFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"),
+        new Value("12345"));
     ServerColumnFamily.DIRECTORY_COLUMN.put(m, new Value("/t1"));
     violations = mc.check(createEnv(), m);
     assertNull(violations);
 
     // active txid, mutation that looks like a load
     m = new Mutation(new Text("0;foo"));
-    m.put(BulkFileColumnFamily.NAME, new Text("/someFile"), new Value("5"));
+    m.put(BulkFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"),
+        new Value("5"));
     m.put(CurrentLocationColumnFamily.NAME, new Text("789"), new Value("127.0.0.1:9997"));
     violations = mc.check(createEnv(), m);
     assertNull(violations);
 
     // inactive txid, mutation that looks like a load
     m = new Mutation(new Text("0;foo"));
-    m.put(BulkFileColumnFamily.NAME, new Text("/someFile"), new Value("12345"));
+    m.put(BulkFileColumnFamily.NAME, new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"),
+        new Value("12345"));
     m.put(CurrentLocationColumnFamily.NAME, new Text("789"), new Value("127.0.0.1:9997"));
     violations = mc.check(createEnv(), m);
     assertNull(violations);
 
     // deleting a load flag
     m = new Mutation(new Text("0;foo"));
-    m.putDelete(BulkFileColumnFamily.NAME, new Text("/someFile"));
+    m.putDelete(BulkFileColumnFamily.NAME,
+        new Text("hdfs://nn1/a/accumulo/tables/2b/t-001/someFile"));
+    violations = mc.check(createEnv(), m);
+    assertNull(violations);
+
+  }
+
+  @Test
+  public void testDataFileCheck() {
+    testFileMetadataValidation(DataFileColumnFamily.NAME);
+  }
+
+  @Test
+  public void testScanFileCheck() {
+    testFileMetadataValidation(ScanFileColumnFamily.NAME);
+  }
+
+  private void testFileMetadataValidation(Text columnFamily) {

Review Comment:
   you aren't using the input argument in the method body.



-- 
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] cshannon commented on a diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1240712618


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,23 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata by creating a StoredTabletFile for the metadata string that
+   * will eagerly load the contained ReferencedTabletFile which performs validation.
+   *
+   * We have to catch a generic Exception as various errors can occur including
+   * NullPointerException, IllegalArgumentException, etc.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      new StoredTabletFile(metadata);

Review Comment:
   It may simplify it a little but not really that much, it's essentially one line of code either way, we either pass the metadata to StoredTabletFile to parse it or pass it to parsePath().  I don't know that it is worth it as I think that change actually makes it more error prone. 
   
   The idea here is we are validating the metadata for a data file. While currently metadata is a path, it doesn't have to be, and if it changes later things break at runtime with no warning ahead of time. If we keep this as StoredTabletFile it is future proof...no matter what happens to the format of the metadata this will always work because StoredTabletFile will know how to handle it. 
   
   If we wanted to try and avoid the construction/GC of an object we could try and create a utility method that can parse/handle metadata. I am thinking maybe I could create a static method on StoredTabletFile to handle the validation so it's a little more clear and maybe we can avoid the object creation. So then it just becomes something like:
   
   ```java
   try {
     StoredTabletFile.validate(metadata);
   } catch (Exception e) {
     violations = addViolation(violations, 9);
   }
   ```
   



-- 
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] cshannon commented on pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#issuecomment-1614859026

   @dlmarion  and @ctubbsii - I just pushed up a new commit that adds a static validate method to `StoredTabletFile` so we don't need to construct the `StoredTabletFile` object just to throw it away in `MetadataConstraints`. This (currently) performs the same validation as inside the `StoredTabletFile` constructor, but the downside is that it is possible for the validation here to get out of sync with the constructor sync it's different code paths if someone only changed the constructor or the validate method. The constructor can't really use the validate method without more refactoring or we'd end up parsing the path twice.
   
   So I don't know if this is ideal or if we should just keeping the previous change of just constructing the new `StoredTabletFile` object for validation so we always ensure we go through one code path.


-- 
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 diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1237297558


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,23 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata by creating a StoredTabletFile for the metadata string that
+   * will eagerly load the contained ReferencedTabletFile which performs validation.
+   *
+   * We have to catch a generic Exception as various errors can occur including
+   * NullPointerException, IllegalArgumentException, etc.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      new StoredTabletFile(metadata);

Review Comment:
   If / when #3509 is merged into 2.1, then in `main` you could change the `parsePath(Path)` method to `parsePath(URI)` and call `parsePath(URI.create(metadata))` here.



-- 
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] cshannon commented on pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#issuecomment-1601163139

   I have updated this PR after reverting the changes from #3502 


-- 
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] cshannon commented on a diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1240724372


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,23 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata by creating a StoredTabletFile for the metadata string that
+   * will eagerly load the contained ReferencedTabletFile which performs validation.
+   *
+   * We have to catch a generic Exception as various errors can occur including
+   * NullPointerException, IllegalArgumentException, etc.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      new StoredTabletFile(metadata);

Review Comment:
   I actually went to try and add a static validate and noticed there was an existing validate method that isn't used anywhere going back to 2.1 so I created a PR to remove that first: https://github.com/apache/accumulo/pull/3540
   
   Also, the one downside if we do the validation somewhere else (like a static validate method) vs the constructor is potentially an issue with different validation being done in different code paths (constructor could end up getting out of sync with the static method) but hopefully if someone is changing the StoredTabletFile class they would make sure that doesn't happen.



-- 
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] cshannon commented on a diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1237322218


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,23 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata by creating a StoredTabletFile for the metadata string that
+   * will eagerly load the contained ReferencedTabletFile which performs validation.
+   *
+   * We have to catch a generic Exception as various errors can occur including
+   * NullPointerException, IllegalArgumentException, etc.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      new StoredTabletFile(metadata);

Review Comment:
   So that would be true if this was staying a path but it won't for long. In the future this metadata won't be a path, it will be json with a path and range with the changes with #3480. So we we would need to have another utility to validate or still use StoredTabletFile so it can read and parse the json



-- 
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 diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1237753748


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,23 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata by creating a StoredTabletFile for the metadata string that
+   * will eagerly load the contained ReferencedTabletFile which performs validation.
+   *
+   * We have to catch a generic Exception as various errors can occur including
+   * NullPointerException, IllegalArgumentException, etc.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      new StoredTabletFile(metadata);
+    } catch (Exception e) {

Review Comment:
   can this catch clause be narrowed, so it doesn't hide future unexpected checked exceptions that might be thrown?
   
   Example: `} catch (KnownCheckedException | RuntimeException e) {`



##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,23 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata by creating a StoredTabletFile for the metadata string that
+   * will eagerly load the contained ReferencedTabletFile which performs validation.
+   *
+   * We have to catch a generic Exception as various errors can occur including
+   * NullPointerException, IllegalArgumentException, etc.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      new StoredTabletFile(metadata);

Review Comment:
   If there's an opportunity to simplify something now, even if it changes later, it might still be worth doing, so the subsequent changes are modifying a simpler implementation. That's better for code reviews, and to see what is actually being accomplished in the subsequent changes. It can also help avoid technical debt, because sometimes when we have more complex code, and we improve a portion of it, we leave behind layers from the original implementation that aren't needed anymore. Simplifying now, whenever possible, helps with that.
   
   That said, I don't know how much this would really simplify things here (haven't looked).



-- 
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 diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1248270420


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,19 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata is valid for a StoredDataFile.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      StoredTabletFile.validate(metadata);
+    } catch (RuntimeException e) {
+      violations = addViolation(violations, 9);
+    }
+    return violations;

Review Comment:
   This API design pattern smells bad to me, and I've been trying to think of why. I think it's because it's not clear to calling code how to correctly call the method:
   
   ```java
   var violations = new ArrayList<Short>();
   violations = validateDataFilePath(violations, someString);
   validateDataFilePath(violations, someString);
   ```
   
   It's not clear from the API which one is correct... if you saw both in the same code, you'd probably think "oh, one of these is a bug", but actually both are correct.
   
   Looking at the class as a whole, I realize that you didn't start this pattern... it already exists in other methods in this class. It looks like it was done to lazily construct the ArrayList. So, in one of these methods, it might return the parameter's object reference, but it might construct a new object and return that instead.
   
   I think there's probably a better way of doing this, but fixing it is outside the scope of this PR. I am bringing it up to ask if you think I should go ahead and change that now, as a prerequisite PR for this PR, or if we should just finish this PR, and I can proceed to fix this in a subsequent PR. I'm fine with 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] cshannon commented on a diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1240728853


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,23 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata by creating a StoredTabletFile for the metadata string that
+   * will eagerly load the contained ReferencedTabletFile which performs validation.
+   *
+   * We have to catch a generic Exception as various errors can occur including
+   * NullPointerException, IllegalArgumentException, etc.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      new StoredTabletFile(metadata);
+    } catch (Exception e) {

Review Comment:
   We should be able to narrow this down now after @dlmarion changes in #3509. The parsePath() doesn't throw checked exceptions (just various runtime on errors like IllegalArgumentException) so I can change this to just RuntimeException.



-- 
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] cshannon commented on a diff in pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3506:
URL: https://github.com/apache/accumulo/pull/3506#discussion_r1248285731


##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -128,6 +129,19 @@ private static ArrayList<Short> addIfNotPresent(ArrayList<Short> lst, int intVio
     return lst;
   }
 
+  /*
+   * Validates the data file metadata is valid for a StoredDataFile.
+   */
+  private static ArrayList<Short> validateDataFilePath(ArrayList<Short> violations,
+      String metadata) {
+    try {
+      StoredTabletFile.validate(metadata);
+    } catch (RuntimeException e) {
+      violations = addViolation(violations, 9);
+    }
+    return violations;

Review Comment:
   I agree, the pattern is definitely not good. @keith-turner already created another issue which is intended to be a follow on to improve this class and the validation https://github.com/apache/accumulo/issues/3505
   
   I think it might make sense to just finish this PR and then fix things in a follow up in line with the Issue created.



-- 
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] cshannon merged pull request #3506: Validate file metadata in MetadataConstraints

Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #3506:
URL: https://github.com/apache/accumulo/pull/3506


-- 
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