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 3e2ed1be89 improve code and docs related to operation id column (#3364) 3e2ed1be89 is described below commit 3e2ed1be897936c192214c2f08f7b2cfc6c1eeea Author: Keith Turner <ktur...@apache.org> AuthorDate: Mon May 1 10:54:32 2023 -0400 improve code and docs related to operation id column (#3364) --- .../accumulo/core/metadata/schema/Ample.java | 10 ++--- .../core/metadata/schema/MetadataSchema.java | 9 +++++ .../core/metadata/schema/TabletMetadata.java | 15 +++----- .../metadata/{ => schema}/TabletOperationId.java | 37 +++++++++++++++++- ...bletOperation.java => TabletOperationType.java} | 2 +- .../server/constraints/MetadataConstraints.java | 14 ++++++- .../metadata/ConditionalTabletMutatorImpl.java | 8 ++-- .../server/metadata/TabletMutatorBase.java | 7 ++-- .../constraints/MetadataConstraintsTest.java | 19 ++++++++++ .../test/functional/AmpleConditionalWriterIT.java | 44 ++++++++++------------ 10 files changed, 112 insertions(+), 53 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java b/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java index 438e78b9e1..d7a7177da2 100644 --- a/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java +++ b/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java @@ -37,7 +37,6 @@ import org.apache.accumulo.core.metadata.ScanServerRefTabletFile; import org.apache.accumulo.core.metadata.StoredTabletFile; import org.apache.accumulo.core.metadata.TServerInstance; import org.apache.accumulo.core.metadata.TabletFile; -import org.apache.accumulo.core.metadata.TabletOperationId; import org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType; import org.apache.accumulo.core.metadata.schema.TabletMetadata.Location; import org.apache.accumulo.core.tabletserver.log.LogEntry; @@ -321,7 +320,7 @@ public interface Ample { T deleteHostingRequested(); - T putOperation(TabletOperation operation, TabletOperationId opId); + T putOperation(TabletOperationId opId); T deleteOperation(); @@ -347,7 +346,9 @@ public interface Ample { * A tablet operation is a mutually exclusive action that is running against a tablet. Its very * important that every conditional mutation specifies requirements about operations in order to * satisfy the mutual exclusion goal. This interface forces those requirements to specified by - * making it the only choice avialable before specifying other tablet requirements or mutations. + * making it the only choice available before specifying other tablet requirements or mutations. + * + * @see MetadataSchema.TabletsSection.ServerColumnFamily#OPID_COLUMN */ interface OperationRequirements { @@ -355,8 +356,7 @@ public interface Ample { * Require a specific operation with a unique id is present. This would be normally be called by * the code executing that operation. */ - ConditionalTabletMutator requireOperation(TabletOperation operation, - TabletOperationId operationId); + ConditionalTabletMutator requireOperation(TabletOperationId operationId); /** * Require that no mutually exclusive operations are runnnig against this tablet. diff --git a/core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java b/core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java index f387c6b20c..793d36d41d 100644 --- a/core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java +++ b/core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java @@ -249,6 +249,15 @@ public class MetadataSchema { public static final String LOCK_QUAL = "lock"; public static final ColumnFQ LOCK_COLUMN = new ColumnFQ(NAME, new Text(LOCK_QUAL)); + /** + * This column is used to indicate an operation is running that needs exclusive access to read + * and write to a tablet. The value uniquely identifies a FATE operation that is running and + * needs the exclusive access. All tablet updates must either ensure this column is absent or + * in the case of a FATE operation that set it ensure the value contains their FATE + * transaction id. When a FATE operation wants to set this column it must ensure its absent + * before setting it. Once a FATE operation has successfully set the column then no other + * tablet update should succeed. + */ public static final String OPID_QUAL = "opid"; public static final ColumnFQ OPID_COLUMN = new ColumnFQ(NAME, new Text(OPID_QUAL)); } diff --git a/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java index 69e85a1139..6a158cd612 100644 --- a/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java +++ b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java @@ -61,7 +61,6 @@ import org.apache.accumulo.core.metadata.SuspendingTServer; import org.apache.accumulo.core.metadata.TServerInstance; import org.apache.accumulo.core.metadata.TabletFile; import org.apache.accumulo.core.metadata.TabletLocationState; -import org.apache.accumulo.core.metadata.TabletOperationId; import org.apache.accumulo.core.metadata.TabletState; import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.BulkFileColumnFamily; import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.ChoppedColumnFamily; @@ -119,7 +118,6 @@ public class TabletMetadata { private boolean chopped = false; private TabletHostingGoal goal = TabletHostingGoal.ONDEMAND; private boolean onDemandHostingRequested = false; - private TabletOperation operation; private TabletOperationId operationId; public enum LocationType { @@ -404,11 +402,10 @@ public class TabletMetadata { return extCompactions; } - public TabletOperation getOperation() { - ensureFetched(ColumnType.OPID); - return operation; - } - + /** + * @return the operation id if it exist, null otherwise + * @see MetadataSchema.TabletsSection.ServerColumnFamily#OPID_COLUMN + */ public TabletOperationId getOperationId() { ensureFetched(ColumnType.OPID); return operationId; @@ -485,9 +482,7 @@ public class TabletMetadata { te.compact = OptionalLong.of(Long.parseLong(val)); break; case OPID_QUAL: - String[] tokens = val.split(":", 2); - te.operation = TabletOperation.valueOf(tokens[0]); - te.operationId = new TabletOperationId(tokens[1]); + te.operationId = TabletOperationId.from(val); break; } break; diff --git a/core/src/main/java/org/apache/accumulo/core/metadata/TabletOperationId.java b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperationId.java similarity index 50% rename from core/src/main/java/org/apache/accumulo/core/metadata/TabletOperationId.java rename to core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperationId.java index 322c7e0f1f..49b7cf169a 100644 --- a/core/src/main/java/org/apache/accumulo/core/metadata/TabletOperationId.java +++ b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperationId.java @@ -16,9 +16,12 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.core.metadata; +package org.apache.accumulo.core.metadata.schema; import org.apache.accumulo.core.data.AbstractId; +import org.apache.accumulo.core.fate.FateTxId; + +import com.google.common.base.Preconditions; /** * Intended to contain a globally unique id that identifies an operation running against a tablet. @@ -28,7 +31,37 @@ public class TabletOperationId extends AbstractId<TabletOperationId> { private static final long serialVersionUID = 1L; - public TabletOperationId(String canonical) { + public static String validate(String opid) { + var fields = opid.split(":"); + Preconditions.checkArgument(fields.length == 2, "Malformed operation id %s", opid); + try { + TabletOperationType.valueOf(fields[0]); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Malformed operation id " + opid, e); + } + + if (!FateTxId.isFormatedTid(fields[1])) { + throw new IllegalArgumentException("Malformed operation id " + opid); + } + + return opid; + } + + private TabletOperationId(String canonical) { super(canonical); } + + public TabletOperationType getType() { + var fields = canonical().split(":"); + Preconditions.checkState(fields.length == 2); + return TabletOperationType.valueOf(fields[0]); + } + + public static TabletOperationId from(String opid) { + return new TabletOperationId(validate(opid)); + } + + public static TabletOperationId from(TabletOperationType type, long txid) { + return new TabletOperationId(type + ":" + FateTxId.formatTid(txid)); + } } diff --git a/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperation.java b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperationType.java similarity index 96% rename from core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperation.java rename to core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperationType.java index fd58ddb61b..4565f126b5 100644 --- a/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperation.java +++ b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletOperationType.java @@ -21,6 +21,6 @@ package org.apache.accumulo.core.metadata.schema; /** * Used to specify what kind of mutually exclusive operation is currently running against a tablet. */ -public enum TabletOperation { +public enum TabletOperationType { SPLITTING, MERGING, DELETING } diff --git a/server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java b/server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java index 26f55b3119..084b1fd932 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java +++ b/server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java @@ -53,6 +53,7 @@ import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.Sc import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.ServerColumnFamily; import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.SuspendLocationColumn; import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.TabletColumnFamily; +import org.apache.accumulo.core.metadata.schema.TabletOperationId; import org.apache.accumulo.core.util.ColumnFQ; import org.apache.accumulo.core.util.cleaner.CleanerUtil; import org.apache.accumulo.server.ServerContext; @@ -224,8 +225,15 @@ public class MetadataConstraints implements Constraint { try { TabletHostingGoalUtil.fromValue(new Value(columnUpdate.getValue())); } catch (IllegalArgumentException e) { - violations = addViolation(violations, 4); + violations = addViolation(violations, 10); } + } else if (ServerColumnFamily.OPID_COLUMN.equals(columnFamily, columnQualifier)) { + try { + TabletOperationId.validate(new String(columnUpdate.getValue(), UTF_8)); + } catch (IllegalArgumentException e) { + violations = addViolation(violations, 9); + } + } else if (columnFamily.equals(BulkFileColumnFamily.NAME)) { if (!columnUpdate.isDeleted() && !checkedBulk) { // splits, which also write the time reference, are allowed to write this reference even @@ -357,6 +365,10 @@ public class MetadataConstraints implements Constraint { return "Lock not held in zookeeper by writer"; case 8: return "Bulk load transaction no longer running"; + case 9: + return "Malformed operation id"; + case 10: + return "Malformed hosting goal"; } return null; } 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 517e0b6a36..bd9effe468 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 @@ -31,13 +31,12 @@ import org.apache.accumulo.core.data.ConditionalMutation; import org.apache.accumulo.core.dataImpl.KeyExtent; import org.apache.accumulo.core.metadata.StoredTabletFile; import org.apache.accumulo.core.metadata.TabletFile; -import org.apache.accumulo.core.metadata.TabletOperationId; import org.apache.accumulo.core.metadata.schema.Ample; import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.BulkFileColumnFamily; import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.DataFileColumnFamily; import org.apache.accumulo.core.metadata.schema.TabletMetadata; import org.apache.accumulo.core.metadata.schema.TabletMetadata.Location; -import org.apache.accumulo.core.metadata.schema.TabletOperation; +import org.apache.accumulo.core.metadata.schema.TabletOperationId; import org.apache.accumulo.server.ServerContext; import org.apache.accumulo.server.metadata.iterators.LocationExistsIterator; import org.apache.accumulo.server.metadata.iterators.PresentIterator; @@ -133,11 +132,10 @@ public class ConditionalTabletMutatorImpl extends TabletMutatorBase<Ample.Condit } @Override - public Ample.ConditionalTabletMutator requireOperation(TabletOperation operation, - TabletOperationId opid) { + public Ample.ConditionalTabletMutator requireOperation(TabletOperationId opid) { Preconditions.checkState(updatesEnabled, "Cannot make updates after calling mutate."); Condition c = new Condition(OPID_COLUMN.getColumnFamily(), OPID_COLUMN.getColumnQualifier()) - .setValue(operation.name() + ":" + opid.canonical()); + .setValue(opid.canonical()); mutation.addCondition(c); sawOperationRequirement = true; return this; diff --git a/server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java b/server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java index c56923292b..02bb9b8f62 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java +++ b/server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java @@ -29,7 +29,6 @@ import org.apache.accumulo.core.metadata.StoredTabletFile; import org.apache.accumulo.core.metadata.SuspendingTServer; import org.apache.accumulo.core.metadata.TServerInstance; import org.apache.accumulo.core.metadata.TabletFile; -import org.apache.accumulo.core.metadata.TabletOperationId; import org.apache.accumulo.core.metadata.schema.Ample; import org.apache.accumulo.core.metadata.schema.DataFileValue; import org.apache.accumulo.core.metadata.schema.ExternalCompactionId; @@ -50,7 +49,7 @@ import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.Ta import org.apache.accumulo.core.metadata.schema.MetadataTime; 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.TabletOperation; +import org.apache.accumulo.core.metadata.schema.TabletOperationId; import org.apache.accumulo.core.tabletserver.log.LogEntry; import org.apache.accumulo.server.ServerContext; import org.apache.hadoop.io.Text; @@ -256,8 +255,8 @@ public abstract class TabletMutatorBase<T extends Ample.TabletUpdates<T>> } @Override - public T putOperation(TabletOperation top, TabletOperationId opId) { - ServerColumnFamily.OPID_COLUMN.put(mutation, new Value(top.name() + ":" + opId.canonical())); + public T putOperation(TabletOperationId opId) { + ServerColumnFamily.OPID_COLUMN.put(mutation, new Value(opId.canonical())); return getThis(); } diff --git a/server/base/src/test/java/org/apache/accumulo/server/constraints/MetadataConstraintsTest.java b/server/base/src/test/java/org/apache/accumulo/server/constraints/MetadataConstraintsTest.java index 63d9a7a6a0..c1c3168141 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/constraints/MetadataConstraintsTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/constraints/MetadataConstraintsTest.java @@ -258,4 +258,23 @@ public class MetadataConstraintsTest { } + @Test + public void testOperationId() { + MetadataConstraints mc = new TestMetadataConstraints(); + Mutation m; + List<Short> violations; + + m = new Mutation(new Text("0;foo")); + ServerColumnFamily.OPID_COLUMN.put(m, new Value("bad id")); + violations = mc.check(createEnv(), m); + assertNotNull(violations); + assertEquals(1, violations.size()); + assertEquals(Short.valueOf((short) 9), violations.get(0)); + + m = new Mutation(new Text("0;foo")); + ServerColumnFamily.OPID_COLUMN.put(m, new Value("MERGING:FATE[123abc]")); + violations = mc.check(createEnv(), m); + assertNull(violations); + } + } 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 2c897200c4..ed99f59686 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 @@ -38,11 +38,11 @@ import org.apache.accumulo.core.metadata.MetadataTable; import org.apache.accumulo.core.metadata.RootTable; import org.apache.accumulo.core.metadata.StoredTabletFile; import org.apache.accumulo.core.metadata.TServerInstance; -import org.apache.accumulo.core.metadata.TabletOperationId; import org.apache.accumulo.core.metadata.schema.DataFileValue; 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.TabletOperation; +import org.apache.accumulo.core.metadata.schema.TabletOperationId; +import org.apache.accumulo.core.metadata.schema.TabletOperationType; import org.apache.accumulo.core.security.TablePermission; import org.apache.accumulo.harness.AccumuloClusterHarness; import org.apache.accumulo.server.metadata.ConditionalTabletsMutatorImpl; @@ -343,52 +343,46 @@ public class AmpleConditionalWriterIT extends AccumuloClusterHarness { var context = cluster.getServerContext(); - var opid1 = new TabletOperationId("1234"); - var opid2 = new TabletOperationId("5678"); + var opid1 = TabletOperationId.from("SPLITTING:FATE[1234]"); + var opid2 = TabletOperationId.from("MERGING:FATE[5678]"); var ctmi = new ConditionalTabletsMutatorImpl(context); - ctmi.mutateTablet(e1).requireAbsentOperation().putOperation(TabletOperation.SPLITTING, opid1) - .submit(); - ctmi.mutateTablet(e2).requireAbsentOperation().putOperation(TabletOperation.MERGING, opid2) - .submit(); - ctmi.mutateTablet(e3).requireOperation(TabletOperation.SPLITTING, opid1).deleteOperation() - .submit(); + ctmi.mutateTablet(e1).requireAbsentOperation().putOperation(opid1).submit(); + ctmi.mutateTablet(e2).requireAbsentOperation().putOperation(opid2).submit(); + ctmi.mutateTablet(e3).requireOperation(opid1).deleteOperation().submit(); var results = ctmi.process(); assertEquals(Status.ACCEPTED, results.get(e1).getStatus()); assertEquals(Status.ACCEPTED, results.get(e2).getStatus()); assertEquals(Status.REJECTED, results.get(e3).getStatus()); - assertEquals(TabletOperation.SPLITTING, context.getAmple().readTablet(e1).getOperation()); + assertEquals(TabletOperationType.SPLITTING, + context.getAmple().readTablet(e1).getOperationId().getType()); assertEquals(opid1, context.getAmple().readTablet(e1).getOperationId()); - assertEquals(TabletOperation.MERGING, context.getAmple().readTablet(e2).getOperation()); + assertEquals(TabletOperationType.MERGING, + context.getAmple().readTablet(e2).getOperationId().getType()); assertEquals(opid2, context.getAmple().readTablet(e2).getOperationId()); - assertEquals(null, context.getAmple().readTablet(e3).getOperation()); assertEquals(null, context.getAmple().readTablet(e3).getOperationId()); ctmi = new ConditionalTabletsMutatorImpl(context); - ctmi.mutateTablet(e1).requireOperation(TabletOperation.MERGING, opid2).deleteOperation() - .submit(); - ctmi.mutateTablet(e2).requireOperation(TabletOperation.SPLITTING, opid1).deleteOperation() - .submit(); + ctmi.mutateTablet(e1).requireOperation(opid2).deleteOperation().submit(); + ctmi.mutateTablet(e2).requireOperation(opid1).deleteOperation().submit(); results = ctmi.process(); assertEquals(Status.REJECTED, results.get(e1).getStatus()); assertEquals(Status.REJECTED, results.get(e2).getStatus()); - assertEquals(TabletOperation.SPLITTING, context.getAmple().readTablet(e1).getOperation()); - assertEquals(TabletOperation.MERGING, context.getAmple().readTablet(e2).getOperation()); + assertEquals(TabletOperationType.SPLITTING, + context.getAmple().readTablet(e1).getOperationId().getType()); + assertEquals(TabletOperationType.MERGING, + context.getAmple().readTablet(e2).getOperationId().getType()); ctmi = new ConditionalTabletsMutatorImpl(context); - ctmi.mutateTablet(e1).requireOperation(TabletOperation.SPLITTING, opid1).deleteOperation() - .submit(); - ctmi.mutateTablet(e2).requireOperation(TabletOperation.MERGING, opid2).deleteOperation() - .submit(); + ctmi.mutateTablet(e1).requireOperation(opid1).deleteOperation().submit(); + ctmi.mutateTablet(e2).requireOperation(opid2).deleteOperation().submit(); results = ctmi.process(); assertEquals(Status.ACCEPTED, results.get(e1).getStatus()); assertEquals(Status.ACCEPTED, results.get(e2).getStatus()); - assertEquals(null, context.getAmple().readTablet(e1).getOperation()); assertEquals(null, context.getAmple().readTablet(e1).getOperationId()); - assertEquals(null, context.getAmple().readTablet(e2).getOperation()); assertEquals(null, context.getAmple().readTablet(e2).getOperationId()); } }