keith-turner commented on a change in pull request #1276: Refactor how
constraint violations are handled
URL: https://github.com/apache/accumulo/pull/1276#discussion_r309225557
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1115,53 +1114,52 @@ private synchronized CommitSession
finishPreparingMutations(long time) {
return commitSession;
}
- public CommitSession prepareMutationsForCommit(TservConstraintEnv cenv,
List<Mutation> mutations)
- throws TConstraintViolationException {
-
- ConstraintChecker cc = constraintChecker.derive();
+ public TabletMutationPrepAttempt prepareMutationsForCommit(final
TservConstraintEnv cenv,
+ final List<Mutation> mutations) {
+ cenv.setExtent(extent);
+ final ConstraintChecker constraints = constraintChecker.derive();
+ final TabletMutationPrepAttempt attempt = new TabletMutationPrepAttempt();
+ // Check each mutation for any constraint violations.
+ Violations violations = null;
List<Mutation> violators = null;
- Violations violations = new Violations();
- cenv.setExtent(extent);
for (Mutation mutation : mutations) {
- Violations more = cc.check(cenv, mutation);
- if (more != null) {
- violations.add(more);
+ Violations mutationViolations = constraints.check(cenv, mutation);
+ if (mutationViolations != null) {
+ if (violations == null) {
+ violations = new Violations();
+ }
if (violators == null) {
violators = new ArrayList<>();
}
+ violations.add(mutationViolations);
violators.add(mutation);
}
}
+ attempt.setViolations(violations);
+ attempt.setViolators(violators);
- long time = tabletTime.setUpdateTimes(mutations);
-
- if (!violations.isEmpty()) {
-
- HashSet<Mutation> violatorsSet = new HashSet<>(violators);
- ArrayList<Mutation> nonViolators = new ArrayList<>();
-
+ // If there are no violations, use the original list for non-violators.
+ if (violations == null) {
+ attempt.setNonViolators(mutations);
+ } else if (violators.size() != mutations.size()) {
+ // Otherwise, find all non-violators.
+ List<Mutation> nonViolators = new ArrayList<>((mutations.size() -
violators.size()));
for (Mutation mutation : mutations) {
- if (!violatorsSet.contains(mutation)) {
+ if (violators.contains(mutation)) {
Review comment:
Every time contains is called it may have to search the entire list. I
think it would be better if `violators` were a `HashSet`. Could then make
`TabletMutationPrepAttempt.setViolators()` take `Collection`.
I ran the ContraintIT and it failed, need a not here.
I made these changes in the PR I submitted to your branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
