keith-turner commented on code in PR #4160: URL: https://github.com/apache/accumulo/pull/4160#discussion_r1470253964
########## core/src/main/java/org/apache/accumulo/core/fate/accumulo/StatusMappingIterator.java: ########## @@ -0,0 +1,148 @@ +/* + * 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.core.fate.accumulo; + +import static org.apache.accumulo.core.fate.accumulo.schema.FateSchema.TxColumnFamily.STATUS_COLUMN; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import org.apache.accumulo.core.client.IteratorSetting; +import org.apache.accumulo.core.data.ByteSequence; +import org.apache.accumulo.core.data.Condition; +import org.apache.accumulo.core.data.Key; +import org.apache.accumulo.core.data.Range; +import org.apache.accumulo.core.data.Value; +import org.apache.accumulo.core.fate.ReadOnlyFateStore; +import org.apache.accumulo.core.iterators.IteratorEnvironment; +import org.apache.accumulo.core.iterators.SortedKeyValueIterator; + +/** + * A specialized iterator that maps the value of the status column to "present" or "absent". This + * iterator allows for checking of the status column's value against a set of acceptable statuses + * within a conditional mutation. + */ +public class StatusMappingIterator implements SortedKeyValueIterator<Key,Value> { + + private static final String PRESENT = "present"; + private static final String ABSENT = "absent"; + private static final String STATUS_SET_KEY = "statusSet"; + + private SortedKeyValueIterator<Key,Value> source; + private final Set<String> acceptableStatuses = new HashSet<>(); + private Value mappedValue; + + /** + * The set of acceptable must be provided as an option to the iterator using the + * {@link #STATUS_SET_KEY} key. + */ + @Override + public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, + IteratorEnvironment env) throws IOException { + this.source = source; + if (options.containsKey(STATUS_SET_KEY)) { + String[] statuses = decodeStatuses(options.get(STATUS_SET_KEY)); + acceptableStatuses.addAll(Arrays.asList(statuses)); + } + } + + @Override + public boolean hasTop() { + return source.hasTop(); + } + + @Override + public void next() throws IOException { + source.next(); + mapValue(); + } + + @Override + public void seek(Range range, Collection<ByteSequence> columnFamilies, boolean inclusive) + throws IOException { + source.seek(range, columnFamilies, inclusive); + mapValue(); + } + + /** + * Maps the value of the status column to "present" or "absent" based on its presence within the + * set of statuses. + */ + private void mapValue() { + if (source.hasTop()) { + String currentValue = source.getTopValue().toString(); + mappedValue = + acceptableStatuses.contains(currentValue) ? new Value(PRESENT) : new Value(ABSENT); + } Review Comment: ```suggestion } else{ mappedValue = null; } ``` ########## test/src/main/java/org/apache/accumulo/test/fate/accumulo/FateMutatorImplIT.java: ########## @@ -0,0 +1,154 @@ +/* + * 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.test.fate.accumulo; + +import static org.apache.accumulo.core.util.LazySingletons.RANDOM; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.time.Duration; + +import org.apache.accumulo.core.client.Accumulo; +import org.apache.accumulo.core.client.AccumuloClient; +import org.apache.accumulo.core.client.admin.NewTableConfiguration; +import org.apache.accumulo.core.client.admin.TabletHostingGoal; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.fate.ReadOnlyFateStore; +import org.apache.accumulo.core.fate.accumulo.FateMutatorImpl; +import org.apache.accumulo.harness.SharedMiniClusterBase; +import org.apache.accumulo.test.fate.FateIT; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class FateMutatorImplIT extends SharedMiniClusterBase { + + Logger log = LoggerFactory.getLogger(FateMutatorImplIT.class); + final NewTableConfiguration ntc = + new NewTableConfiguration().withInitialHostingGoal(TabletHostingGoal.ALWAYS); + + @BeforeAll + public static void setup() throws Exception { + SharedMiniClusterBase.startMiniCluster(); + } + + @AfterAll + public static void tearDown() { + SharedMiniClusterBase.stopMiniCluster(); + } + + @Override + protected Duration defaultTimeout() { + return Duration.ofMinutes(5); + } + + @Test + public void putRepo() throws Exception { + final String table = getUniqueNames(1)[0]; + try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) { + client.tableOperations().create(table, ntc); + + ClientContext context = (ClientContext) client; + + final long tid = RANDOM.get().nextLong() & 0x7fffffffffffffffL; + + // add some repos in order + FateMutatorImpl<FateIT.TestEnv> fateMutator = new FateMutatorImpl<>(context, table, tid); + fateMutator.putRepo(100, new FateIT.TestRepo("test")).mutate(); + FateMutatorImpl<FateIT.TestEnv> fateMutator1 = new FateMutatorImpl<>(context, table, tid); + fateMutator1.putRepo(99, new FateIT.TestRepo("test")).mutate(); + FateMutatorImpl<FateIT.TestEnv> fateMutator2 = new FateMutatorImpl<>(context, table, tid); + fateMutator2.putRepo(98, new FateIT.TestRepo("test")).mutate(); + + // make sure we cant add a repo that has already been added + FateMutatorImpl<FateIT.TestEnv> fateMutator3 = new FateMutatorImpl<>(context, table, tid); + assertThrows(IllegalStateException.class, + () -> fateMutator3.putRepo(98, new FateIT.TestRepo("test")).mutate(), + "Repo in position 98 already exists. Expected to not be able to add it again."); + FateMutatorImpl<FateIT.TestEnv> fateMutator4 = new FateMutatorImpl<>(context, table, tid); + assertThrows(IllegalStateException.class, + () -> fateMutator4.putRepo(99, new FateIT.TestRepo("test")).mutate(), + "Repo in position 99 already exists. Expected to not be able to add it again."); + } + } + + @Test + public void requireStatus() throws Exception { + final String table = getUniqueNames(1)[0]; + try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) { + client.tableOperations().create(table, ntc); + + ClientContext context = (ClientContext) client; + + final long tid = RANDOM.get().nextLong() & 0x7fffffffffffffffL; + + // use require status passing all statuses. without the status column present this should fail + FateMutatorImpl<FateIT.TestEnv> fateMutator = new FateMutatorImpl<>(context, table, tid); + assertThrows(IllegalStateException.class, + () -> fateMutator.requireStatus(ReadOnlyFateStore.TStatus.values()) + .putStatus(ReadOnlyFateStore.TStatus.NEW).mutate()); + + logAllEntriesInTable(table, client); // this prints nothing on the table as we expect + + // use require status without passing any statuses to require that the status column is absent + FateMutatorImpl<FateIT.TestEnv> fateMutator0 = new FateMutatorImpl<>(context, table, tid); + fateMutator0.requireStatus().putStatus(ReadOnlyFateStore.TStatus.NEW).mutate(); + Review Comment: would be good to read the status after each change or attempted change to ensure its as expected ########## test/src/main/java/org/apache/accumulo/test/fate/accumulo/AccumuloStoreIT.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.test.fate.accumulo; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.Iterator; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; + +import org.apache.accumulo.core.client.Accumulo; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.fate.accumulo.AccumuloStore; +import org.apache.accumulo.harness.SharedMiniClusterBase; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class AccumuloStoreIT extends SharedMiniClusterBase { Review Comment: This is a nice test. If possible would be nice to eventually run the test against Zookeeper also like #4202. If it makes sense could be a follow on. ########## core/src/main/java/org/apache/accumulo/core/fate/accumulo/StatusMappingIterator.java: ########## @@ -0,0 +1,148 @@ +/* + * 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.core.fate.accumulo; + +import static org.apache.accumulo.core.fate.accumulo.schema.FateSchema.TxColumnFamily.STATUS_COLUMN; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import org.apache.accumulo.core.client.IteratorSetting; +import org.apache.accumulo.core.data.ByteSequence; +import org.apache.accumulo.core.data.Condition; +import org.apache.accumulo.core.data.Key; +import org.apache.accumulo.core.data.Range; +import org.apache.accumulo.core.data.Value; +import org.apache.accumulo.core.fate.ReadOnlyFateStore; +import org.apache.accumulo.core.iterators.IteratorEnvironment; +import org.apache.accumulo.core.iterators.SortedKeyValueIterator; + +/** + * A specialized iterator that maps the value of the status column to "present" or "absent". This + * iterator allows for checking of the status column's value against a set of acceptable statuses + * within a conditional mutation. + */ +public class StatusMappingIterator implements SortedKeyValueIterator<Key,Value> { + + private static final String PRESENT = "present"; + private static final String ABSENT = "absent"; + private static final String STATUS_SET_KEY = "statusSet"; + + private SortedKeyValueIterator<Key,Value> source; + private final Set<String> acceptableStatuses = new HashSet<>(); + private Value mappedValue; + + /** + * The set of acceptable must be provided as an option to the iterator using the + * {@link #STATUS_SET_KEY} key. + */ + @Override + public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, + IteratorEnvironment env) throws IOException { + this.source = source; + if (options.containsKey(STATUS_SET_KEY)) { + String[] statuses = decodeStatuses(options.get(STATUS_SET_KEY)); + acceptableStatuses.addAll(Arrays.asList(statuses)); + } + } + + @Override + public boolean hasTop() { + return source.hasTop(); + } + + @Override + public void next() throws IOException { + source.next(); + mapValue(); + } + + @Override + public void seek(Range range, Collection<ByteSequence> columnFamilies, boolean inclusive) + throws IOException { + source.seek(range, columnFamilies, inclusive); + mapValue(); + } + + /** + * Maps the value of the status column to "present" or "absent" based on its presence within the + * set of statuses. + */ + private void mapValue() { + if (source.hasTop()) { + String currentValue = source.getTopValue().toString(); + mappedValue = + acceptableStatuses.contains(currentValue) ? new Value(PRESENT) : new Value(ABSENT); + } + } + + @Override + public Key getTopKey() { + return source.getTopKey(); + } + + @Override + public Value getTopValue() { + return mappedValue; Review Comment: This makes things fail fast when mappedValue is not set ```suggestion return Objects.requireNonnull(mappedValue); ``` -- 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]
