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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
