ctubbsii commented on code in PR #5924:
URL: https://github.com/apache/accumulo/pull/5924#discussion_r2373648105
##########
core/src/main/java/org/apache/accumulo/core/data/constraints/DefaultKeySizeConstraint.java:
##########
@@ -68,6 +72,9 @@ public List<Short> check(Environment env, Mutation mutation) {
if (size > maxSize) {
violations.add(MAX__KEY_SIZE_EXCEEDED_VIOLATION);
+ Key k = new Key(mutation.getRow(), cu.getColumnFamily(),
cu.getColumnQualifier(),
+ cu.getColumnVisibility(), cu.getTimestamp(), cu.isDeleted());
+ LOG.info("Constraint violation. Key {} (size = {}) larger than 1MB",
k, size);
Review Comment:
I'm not very comfortable allowing client data to be able to trigger
server-side log spam, and I've vigorously argued against doing that in the
past. This would do that, and I'm very sensitive to the idea that a user could
send data that files up a disk drive with logs and topples Accumulo that way.
However, maybe it's okay in this case because part of the user's choice to
use this constraint is accepting its behavior. This constraint is pluggable,
and not required for users to use. Then again, I think we do set it as default,
so that's something to consider.
Also, the logger being used is in the deprecated class that shouldn't be
used anymore. Merging forward will change the name of the logger. It's probably
not a good idea to introduce a new log message for users to get used to seeing,
only to completely rename it after they've set up filters/alerts/etc. to look
for its appearance in the logs.
I think it's okay if this doesn't log, since it already sends some
information back to the user. However, if we do keep it, it'd be nice to use
the new class name, so it doesn't churn on upgrade, and to set it at debug.
##########
core/src/main/java/org/apache/accumulo/core/constraints/DefaultKeySizeConstraint.java:
##########
@@ -38,43 +36,18 @@
public class DefaultKeySizeConstraint extends
org.apache.accumulo.core.data.constraints.DefaultKeySizeConstraint
implements Constraint {
- protected static final short MAX__KEY_SIZE_EXCEEDED_VIOLATION = 1;
- protected static final long maxSize = 1048576; // 1MB default size
+ protected static final short MAX__KEY_SIZE_EXCEEDED_VIOLATION =
+
org.apache.accumulo.core.data.constraints.DefaultKeySizeConstraint.MAX__KEY_SIZE_EXCEEDED_VIOLATION;
+ protected static final long maxSize =
+
org.apache.accumulo.core.data.constraints.DefaultKeySizeConstraint.maxSize;
@Override
public String getViolationDescription(short violationCode) {
-
- switch (violationCode) {
- case MAX__KEY_SIZE_EXCEEDED_VIOLATION:
- return "Key was larger than 1MB";
- }
-
- return null;
+ return super.getViolationDescription(violationCode);
Review Comment:
If you're just doing passthrough, you don't need to override the method at
all. However, this is going to be frustrating on merge, because the point of
the code being copied here was that the previous implementation was deprecated.
This class is now the canonical implementation going forward.
--
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]
