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