This is an automated email from the ASF dual-hosted git repository.
kturner pushed a commit to branch elasticity
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/elasticity by this push:
new 79337889f7 ensure only single location is set in conditional location
check (#4620)
79337889f7 is described below
commit 79337889f7ab4a0040973a26e8709c09b053c280
Author: Keith Turner <ktur...@apache.org>
AuthorDate: Thu May 30 15:29:17 2024 -0400
ensure only single location is set in conditional location check (#4620)
Accumulo's metadata schema for locations allows multiple locations
to be set for a tablet even though only one is expected. The
conditional mutation check for a location would succeed if the
required location was present, even if other locations were also
present. Updated the check to make it more strict and only pass
if the required location is present AND no other locations are
present.
This update removes a specialized iterator for locations and uses
the more general SetEncodingIterator which allows treating a column
family like a map for comparison purposes. Using this we can easily
check the more strict requirements using well tested code.
Added to ITs related to location conditional check to exercise the
situation of multiple locations being set on a tablet.
---
.../core/metadata/schema/TabletMutatorBase.java | 13 +++
.../metadata/ConditionalTabletMutatorImpl.java | 55 ++++++++++--
.../metadata/iterators/LocationExistsIterator.java | 68 ---------------
.../metadata/iterators/SetEncodingIterator.java | 11 ++-
.../metadata/iterators/TabletExistsIterator.java | 2 +-
.../test/functional/AmpleConditionalWriterIT.java | 99 ++++++++++++++++++++++
6 files changed, 169 insertions(+), 79 deletions(-)
diff --git
a/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMutatorBase.java
b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMutatorBase.java
index ab0d6490a3..25db5e1c16 100644
---
a/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMutatorBase.java
+++
b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMutatorBase.java
@@ -165,6 +165,19 @@ public abstract class TabletMutatorBase<T extends
Ample.TabletUpdates<T>>
}
}
+ protected Text getLocationFamilyText(LocationType type) {
+ switch (type) {
+ case CURRENT:
+ return CurrentLocationColumnFamily.NAME;
+ case FUTURE:
+ return FutureLocationColumnFamily.NAME;
+ case LAST:
+ return LastLocationColumnFamily.NAME;
+ default:
+ throw new IllegalArgumentException();
+ }
+ }
+
@Override
public T putLocation(Location location) {
Preconditions.checkState(updatesEnabled, "Cannot make updates after
calling mutate.");
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/metadata/ConditionalTabletMutatorImpl.java
b/server/base/src/main/java/org/apache/accumulo/server/metadata/ConditionalTabletMutatorImpl.java
index 208fe4954c..33f746c599 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/metadata/ConditionalTabletMutatorImpl.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/metadata/ConditionalTabletMutatorImpl.java
@@ -34,6 +34,7 @@ import java.util.Objects;
import java.util.Set;
import java.util.function.BiConsumer;
import java.util.function.Consumer;
+import java.util.function.Function;
import org.apache.accumulo.core.client.IteratorSetting;
import org.apache.accumulo.core.client.admin.TabletAvailability;
@@ -54,12 +55,12 @@ import
org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.Us
import org.apache.accumulo.core.metadata.schema.TabletMetadata;
import org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType;
import org.apache.accumulo.core.metadata.schema.TabletMetadata.Location;
+import org.apache.accumulo.core.metadata.schema.TabletMetadata.LocationType;
import org.apache.accumulo.core.metadata.schema.TabletMutatorBase;
import org.apache.accumulo.core.metadata.schema.TabletOperationId;
import org.apache.accumulo.core.tabletserver.log.LogEntry;
import org.apache.accumulo.core.util.Pair;
import org.apache.accumulo.server.ServerContext;
-import org.apache.accumulo.server.metadata.iterators.LocationExistsIterator;
import org.apache.accumulo.server.metadata.iterators.PresentIterator;
import org.apache.accumulo.server.metadata.iterators.SetEncodingIterator;
import org.apache.accumulo.server.metadata.iterators.TabletExistsIterator;
@@ -101,21 +102,57 @@ public class ConditionalTabletMutatorImpl extends
TabletMutatorBase<Ample.Condit
@Override
public Ample.ConditionalTabletMutator requireAbsentLocation() {
Preconditions.checkState(updatesEnabled, "Cannot make updates after
calling mutate.");
- IteratorSetting is = new IteratorSetting(INITIAL_ITERATOR_PRIO,
LocationExistsIterator.class);
- Condition c = new Condition("", "").setIterators(is);
- mutation.addCondition(c);
+
+ // It is not expected the encoder will actually be called, so throw an
exception if it is.
+ Function<Location,byte[]> encoder = l -> {
+ throw new UnsupportedOperationException();
+ };
+
+ // The column families for each location type should conceptually be an
empty set, so create
+ // conditions that check for this.
+ var condition1 = SetEncodingIterator.createCondition(Set.of(), encoder,
+ getLocationFamilyText(LocationType.FUTURE));
+ var condition2 = SetEncodingIterator.createCondition(Set.of(), encoder,
+ getLocationFamilyText(LocationType.CURRENT));
+
+ // Add the conditions for both location column families, both conditions
must be met for the
+ // mutation to be applied.
+ mutation.addCondition(condition1);
+ mutation.addCondition(condition2);
+
return this;
}
@Override
public Ample.ConditionalTabletMutator requireLocation(Location location) {
Preconditions.checkState(updatesEnabled, "Cannot make updates after
calling mutate.");
- Preconditions.checkArgument(location.getType() ==
TabletMetadata.LocationType.FUTURE
- || location.getType() == TabletMetadata.LocationType.CURRENT);
+ Preconditions.checkArgument(
+ location.getType() == LocationType.FUTURE || location.getType() ==
LocationType.CURRENT);
sawOperationRequirement = true;
- Condition c = new Condition(getLocationFamily(location.getType()),
location.getSession())
- .setValue(location.getHostPort());
- mutation.addCondition(c);
+
+ Function<Location,Pair<byte[],byte[]>> encoder =
+ l -> new Pair<>(location.getSession().getBytes(UTF_8),
+ location.getHostPort().getBytes(UTF_8));
+
+ // The location column family can have multiple column qualifiers set.
When requiring a location
+ // we want to check the location is set AND that no other location
qualifiers are set on the
+ // column family. So the condition should conceptually check that the
column family is a map of
+ // size one with only our expected location set in the map.
+ var condition1 =
SetEncodingIterator.createConditionWithVal(Set.of(location), encoder,
+ getLocationFamilyText(location.getType()));
+
+ // Conceptually the column family for the other location type should be an
empty map, so create
+ // a condition that checks this.
+ var otherLocType =
+ location.getType() == LocationType.CURRENT ? LocationType.FUTURE :
LocationType.CURRENT;
+ var condition2 = SetEncodingIterator.createConditionWithVal(Set.of(),
encoder,
+ getLocationFamilyText(otherLocType));
+
+ // Add the conditions for both location column families, both conditions
must be met for the
+ // mutation to be applied.
+ mutation.addCondition(condition1);
+ mutation.addCondition(condition2);
+
return this;
}
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/LocationExistsIterator.java
b/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/LocationExistsIterator.java
deleted file mode 100644
index bd4396b33b..0000000000
---
a/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/LocationExistsIterator.java
+++ /dev/null
@@ -1,68 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.accumulo.server.metadata.iterators;
-
-import java.io.IOException;
-import java.util.Collection;
-import java.util.Set;
-
-import org.apache.accumulo.core.data.ArrayByteSequence;
-import org.apache.accumulo.core.data.ByteSequence;
-import org.apache.accumulo.core.data.Key;
-import org.apache.accumulo.core.data.PartialKey;
-import org.apache.accumulo.core.data.Range;
-import org.apache.accumulo.core.iterators.WrappingIterator;
-import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection;
-import
org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.CurrentLocationColumnFamily;
-import
org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.FutureLocationColumnFamily;
-import org.apache.hadoop.io.Text;
-
-import com.google.common.base.Preconditions;
-
-/**
- * A specialized iterator used for conditional mutations to check if a
location is present in a
- * tablet.
- */
-public class LocationExistsIterator extends WrappingIterator {
- private static final Collection<ByteSequence> LOC_FAMS =
- Set.of(new ArrayByteSequence(FutureLocationColumnFamily.STR_NAME),
- new ArrayByteSequence(CurrentLocationColumnFamily.STR_NAME));
-
- @Override
- public void seek(Range range, Collection<ByteSequence> columnFamilies,
boolean inclusive)
- throws IOException {
-
- Text tabletRow = getTabletRow(range);
- Key startKey = new Key(tabletRow, FutureLocationColumnFamily.NAME);
- Key endKey =
- new Key(tabletRow,
CurrentLocationColumnFamily.NAME).followingKey(PartialKey.ROW_COLFAM);
-
- Range r = new Range(startKey, true, endKey, false);
-
- super.seek(r, LOC_FAMS, true);
- }
-
- static Text getTabletRow(Range range) {
- var row = range.getStartKey().getRow();
- // expecting this range to cover a single metadata row, so validate the
range meets expectations
- TabletsSection.validateRow(row);
- Preconditions.checkArgument(row.equals(range.getEndKey().getRow()));
- return range.getStartKey().getRow();
- }
-}
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/SetEncodingIterator.java
b/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/SetEncodingIterator.java
index facd747b47..b0456afd32 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/SetEncodingIterator.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/SetEncodingIterator.java
@@ -42,6 +42,7 @@ import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iterators.IteratorEnvironment;
import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.metadata.schema.MetadataSchema;
import org.apache.accumulo.core.util.Pair;
import org.apache.accumulo.server.metadata.ConditionalTabletMutatorImpl;
import org.apache.hadoop.io.Text;
@@ -73,11 +74,19 @@ public class SetEncodingIterator implements
SortedKeyValueIterator<Key,Value> {
private Value topValue = null;
private boolean concat = false;
+ static Text getTabletRow(Range range) {
+ var row = range.getStartKey().getRow();
+ // expecting this range to cover a single metadata row, so validate the
range meets expectations
+ MetadataSchema.TabletsSection.validateRow(row);
+ Preconditions.checkArgument(row.equals(range.getEndKey().getRow()));
+ return range.getStartKey().getRow();
+ }
+
@Override
public void seek(Range range, Collection<ByteSequence> columnFamilies,
boolean inclusive)
throws IOException {
- Text tabletRow = LocationExistsIterator.getTabletRow(range);
+ Text tabletRow = getTabletRow(range);
Text family = range.getStartKey().getColumnFamily();
Preconditions.checkArgument(
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/TabletExistsIterator.java
b/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/TabletExistsIterator.java
index 4b9672f8dc..a258f19e6d 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/TabletExistsIterator.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/metadata/iterators/TabletExistsIterator.java
@@ -37,7 +37,7 @@ public class TabletExistsIterator extends WrappingIterator {
public void seek(Range range, Collection<ByteSequence> columnFamilies,
boolean inclusive)
throws IOException {
- Text tabletRow = LocationExistsIterator.getTabletRow(range);
+ Text tabletRow = SetEncodingIterator.getTabletRow(range);
Range r = new Range(tabletRow, true, tabletRow, false);
diff --git
a/test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java
b/test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java
index 3c51f2db47..6e7df27e2b 100644
---
a/test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java
+++
b/test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java
@@ -164,6 +164,7 @@ public class AmpleConditionalWriterIT extends
AccumuloClusterHarness {
assertEquals(Location.future(ts1),
context.getAmple().readTablet(e1).getLocation());
+ // test require absent with a future location set
ctmi = new ConditionalTabletsMutatorImpl(context);
ctmi.mutateTablet(e1).requireAbsentOperation().requireAbsentLocation()
.putLocation(Location.future(ts2)).submit(tm -> false);
@@ -188,6 +189,15 @@ public class AmpleConditionalWriterIT extends
AccumuloClusterHarness {
assertEquals(Location.current(ts1),
context.getAmple().readTablet(e1).getLocation());
+ // test require absent with a current location set
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().requireAbsentLocation()
+ .putLocation(Location.future(ts2)).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.REJECTED, results.get(e1).getStatus());
+
+ assertEquals(Location.current(ts1),
context.getAmple().readTablet(e1).getLocation());
+
try (TabletsMetadata tablets =
context.getAmple().readTablets().forTable(tid).filter(new
HasCurrentFilter()).build()) {
List<KeyExtent> actual =
@@ -220,6 +230,95 @@ public class AmpleConditionalWriterIT extends
AccumuloClusterHarness {
assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
assertNull(context.getAmple().readTablet(e1).getLocation());
+
+ // Set two current locations, this puts the tablet in a bad state as its
only expected that
+ // single location should be set
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+
ctmi.mutateTablet(e1).requireAbsentOperation().putLocation(Location.current(ts1))
+ .putLocation(Location.current(ts2)).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+
+ // When a tablet has two locations reading it should throw an exception
+ assertThrows(IllegalStateException.class, () ->
context.getAmple().readTablet(e1));
+
+ // Try to update the tablet requiring one of the locations that is set
on the tablet. Even
+ // though the required location exists, the presence of the other
location in the tablet
+ // metadata should cause the update to fail.
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+
ctmi.mutateTablet(e1).requireAbsentOperation().requireLocation(Location.current(ts1))
+
.deleteLocation(Location.current(ts1)).deleteLocation(Location.current(ts2))
+ .submit(tm -> false);
+ results = ctmi.process();
+ var finalResult1 = results.get(e1);
+ // The update should be rejected because of the two locations. When a
conditional mutation is
+ // rejected an attempt is made to read the tablet metadata and examine.
This read of the
+ // tablet metadata will fail because the tablet has two locations.
+ assertThrows(IllegalStateException.class, finalResult1::getStatus);
+
+ // tablet should still have two location set, so reading it should fail
+ assertThrows(IllegalStateException.class, () ->
context.getAmple().readTablet(e1));
+
+ // Requiring an absent location should fail when two locations are set
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().requireAbsentLocation()
+
.deleteLocation(Location.current(ts1)).deleteLocation(Location.current(ts2))
+ .submit(tm -> false);
+ results = ctmi.process();
+ var finalResult2 = results.get(e1);
+ assertThrows(IllegalStateException.class, finalResult2::getStatus);
+
+ // tablet should still have two location set, so reading it should fail
+ assertThrows(IllegalStateException.class, () ->
context.getAmple().readTablet(e1));
+
+ // Change the tablet to have a futre and current location set.
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+
ctmi.mutateTablet(e1).requireAbsentOperation().deleteLocation(Location.current(ts1))
+ .putLocation(Location.future(ts1)).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+
+ // tablet should still have two location set, so reading it should fail
+ assertThrows(IllegalStateException.class, () ->
context.getAmple().readTablet(e1));
+
+ // Test requiring different locations. Some of the required locations
are actually set and
+ // some are not. All should fail because the tablet has multiple
locations set and/or the
+ // required location does not exist.
+ for (var loc : List.of(Location.current(ts1), Location.current(ts2),
Location.future(ts1),
+ Location.future(ts2))) {
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().requireLocation(loc)
+
.deleteLocation(Location.future(ts1)).deleteLocation(Location.current(ts2))
+ .submit(tm -> false);
+ results = ctmi.process();
+ var finalResult3 = results.get(e1);
+ // tablet should still have two location set, so reading it should fail
+ assertThrows(IllegalStateException.class, finalResult3::getStatus);
+ }
+
+ // Requiring an absent location should fail when a future and current
location are set
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().requireAbsentLocation()
+
.deleteLocation(Location.current(ts1)).deleteLocation(Location.current(ts2))
+ .submit(tm -> false);
+ results = ctmi.process();
+ var finalResult4 = results.get(e1);
+ assertThrows(IllegalStateException.class, finalResult4::getStatus);
+
+ // tablet should still have two location set, so reading it should fail
+ assertThrows(IllegalStateException.class, () ->
context.getAmple().readTablet(e1));
+
+ // Delete one of the locations w/o any location requirements, this
should succeed.
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+
ctmi.mutateTablet(e1).requireAbsentOperation().deleteLocation(Location.future(ts1))
+ .submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+
+ // This check validates the expected state of the tablet as some of the
previous test were
+ // catching an exception and making assumption about the state of the
tablet metadata based on
+ // the fact that an exception was thrown.
+ assertEquals(Location.current(ts2),
context.getAmple().readTablet(e1).getLocation());
}
}
