keith-turner commented on code in PR #3642:
URL: https://github.com/apache/accumulo/pull/3642#discussion_r1270943008
##########
test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java:
##########
@@ -382,6 +397,125 @@ public void testSelectedFiles() throws Exception {
assertNull(context.getAmple().readTablet(e1).getSelectedFiles());
}
+ /**
+ * Verifies that if selected files have been manually changed in the
metadata, the files will be
+ * re-ordered before being read
+ */
+ @Test
+ public void testSelectedFilesReordering() throws Exception {
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ var context = cluster.getServerContext();
+
+ String pathPrefix =
"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/";
+ StoredTabletFile stf1 = new StoredTabletFile(pathPrefix + "F0000070.rf");
+ StoredTabletFile stf2 = new StoredTabletFile(pathPrefix + "F0000071.rf");
+ StoredTabletFile stf3 = new StoredTabletFile(pathPrefix + "F0000072.rf");
+
+ final Set<StoredTabletFile> storedTabletFiles = Set.of(stf1, stf2, stf3);
+ final boolean initiallySelectedAll = true;
+ final long fateTxId = 2L;
+ final SelectedFiles selectedFiles =
+ new SelectedFiles(storedTabletFiles, initiallySelectedAll, fateTxId);
+
+ ConditionalTabletsMutatorImpl ctmi = new
ConditionalTabletsMutatorImpl(context);
+
+ // write the SelectedFiles to the keyextent
+
ctmi.mutateTablet(e1).requireAbsentOperation().putSelectedFiles(selectedFiles)
+ .submit(tm -> false);
+
+ // verify we can read the selected files
+ Status mutationStatus = ctmi.process().get(e1).getStatus();
+ assertEquals(Status.ACCEPTED, mutationStatus, "Failed to put selected
files to tablet");
+ assertEquals(selectedFiles,
context.getAmple().readTablet(e1).getSelectedFiles(),
+ "Selected files should match those that were written");
+
+ final Text row = e1.toMetaRow();
+ final Text selectedColumnFamily = SELECTED_COLUMN.getColumnFamily();
+ final Text selectedColumnQualifier =
SELECTED_COLUMN.getColumnQualifier();
+
+ Supplier<String> selectedMetadataValue = () -> {
+ try (Scanner scanner = client.createScanner(MetadataTable.NAME)) {
+ scanner.fetchColumn(selectedColumnFamily, selectedColumnQualifier);
+ scanner.setRange(new Range(row));
+
+ return scanner.stream().map(Map.Entry::getValue).map(Value::get)
+ .map(entry -> new String(entry, UTF_8)).collect(onlyElement());
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ };
+
+ String actualMetadataValue = selectedMetadataValue.get();
+ final String expectedMetadata = selectedFiles.getMetadataValue();
+ assertEquals(expectedMetadata, actualMetadataValue,
+ "Value should be equal to metadata of SelectedFiles object that was
written");
+
+ // get the string value of the paths
+ List<String> filesPathList =
storedTabletFiles.stream().map(StoredTabletFile::toString)
+ .sorted().collect(Collectors.toList());
+
+ // verify we have the format of the json correct
+ String newJson = createSelectedFilesJson(fateTxId, initiallySelectedAll,
filesPathList);
+ assertEquals(actualMetadataValue, newJson,
+ "Test json should be identical to actual metadata at this point");
+
+ // reverse the order of the files and create a new json
+ Collections.reverse(filesPathList);
+ newJson = createSelectedFilesJson(fateTxId, initiallySelectedAll,
filesPathList);
+ assertNotEquals(actualMetadataValue, newJson,
+ "Test json should have reverse file order of actual metadata");
+
+ // write the json with reverse file order
+ try (BatchWriter bw = client.createBatchWriter(MetadataTable.NAME)) {
+ Mutation mutation = new Mutation(row);
+ mutation.put(selectedColumnFamily, selectedColumnQualifier,
+ new Value(newJson.getBytes(UTF_8)));
+ bw.addMutation(mutation);
+ }
+
+ // verify the metadata has been changed
+ actualMetadataValue = selectedMetadataValue.get();
+ assertEquals(newJson, actualMetadataValue,
Review Comment:
This verification that the misnormalized data is in the metadata table is
very thorough.
##########
core/src/main/java/org/apache/accumulo/core/iterators/SortedFilesIterator.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.iterators;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import java.io.IOException;
+import java.util.Collection;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.metadata.schema.SelectedFiles;
+
+/**
+ * This iterator encodes the value as a SelectedFiles object then decodes it
back into a String to
+ * ensure the resulting String value has a sorted array of file paths.
+ */
+public class SortedFilesIterator extends WrappingIterator {
+
+ private Value sortedValue = null;
+
+ @Override
+ public Value getTopValue() {
+ if (sortedValue == null) {
+ Value unsortedValue = super.getTopValue();
+ SelectedFiles selectedFiles = SelectedFiles.from(new
String(unsortedValue.get()));
+ sortedValue = new
Value(selectedFiles.getMetadataValue().getBytes(UTF_8));
+ }
+ return sortedValue;
Review Comment:
I think it would ok from a performance perspective to not cache the value
and it would simplify the code. Also the resort of the value could only be
done when the current top key has the expected column. Also need to specify
UTF_8 when creating the string.
```suggestion
if (SELECTED_COLUMN.hasColumns(getTopKey())) {
Value unsortedValue = super.getTopValue();
SelectedFiles selectedFiles = SelectedFiles.from(new
String(unsortedValue.get(), UTF_8));
return new Value(selectedFiles.getMetadataValue().getBytes(UTF_8));
}
return super.getTopValue();
```
##########
test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java:
##########
@@ -382,6 +397,125 @@ public void testSelectedFiles() throws Exception {
assertNull(context.getAmple().readTablet(e1).getSelectedFiles());
}
+ /**
+ * Verifies that if selected files have been manually changed in the
metadata, the files will be
+ * re-ordered before being read
+ */
+ @Test
+ public void testSelectedFilesReordering() throws Exception {
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ var context = cluster.getServerContext();
+
+ String pathPrefix =
"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/";
+ StoredTabletFile stf1 = new StoredTabletFile(pathPrefix + "F0000070.rf");
+ StoredTabletFile stf2 = new StoredTabletFile(pathPrefix + "F0000071.rf");
+ StoredTabletFile stf3 = new StoredTabletFile(pathPrefix + "F0000072.rf");
+
+ final Set<StoredTabletFile> storedTabletFiles = Set.of(stf1, stf2, stf3);
+ final boolean initiallySelectedAll = true;
+ final long fateTxId = 2L;
+ final SelectedFiles selectedFiles =
+ new SelectedFiles(storedTabletFiles, initiallySelectedAll, fateTxId);
+
+ ConditionalTabletsMutatorImpl ctmi = new
ConditionalTabletsMutatorImpl(context);
+
+ // write the SelectedFiles to the keyextent
+
ctmi.mutateTablet(e1).requireAbsentOperation().putSelectedFiles(selectedFiles)
+ .submit(tm -> false);
+
+ // verify we can read the selected files
+ Status mutationStatus = ctmi.process().get(e1).getStatus();
+ assertEquals(Status.ACCEPTED, mutationStatus, "Failed to put selected
files to tablet");
+ assertEquals(selectedFiles,
context.getAmple().readTablet(e1).getSelectedFiles(),
+ "Selected files should match those that were written");
+
+ final Text row = e1.toMetaRow();
+ final Text selectedColumnFamily = SELECTED_COLUMN.getColumnFamily();
+ final Text selectedColumnQualifier =
SELECTED_COLUMN.getColumnQualifier();
+
+ Supplier<String> selectedMetadataValue = () -> {
+ try (Scanner scanner = client.createScanner(MetadataTable.NAME)) {
+ scanner.fetchColumn(selectedColumnFamily, selectedColumnQualifier);
+ scanner.setRange(new Range(row));
+
+ return scanner.stream().map(Map.Entry::getValue).map(Value::get)
+ .map(entry -> new String(entry, UTF_8)).collect(onlyElement());
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ };
+
+ String actualMetadataValue = selectedMetadataValue.get();
+ final String expectedMetadata = selectedFiles.getMetadataValue();
+ assertEquals(expectedMetadata, actualMetadataValue,
+ "Value should be equal to metadata of SelectedFiles object that was
written");
+
+ // get the string value of the paths
+ List<String> filesPathList =
storedTabletFiles.stream().map(StoredTabletFile::toString)
+ .sorted().collect(Collectors.toList());
+
+ // verify we have the format of the json correct
+ String newJson = createSelectedFilesJson(fateTxId, initiallySelectedAll,
filesPathList);
+ assertEquals(actualMetadataValue, newJson,
+ "Test json should be identical to actual metadata at this point");
+
+ // reverse the order of the files and create a new json
+ Collections.reverse(filesPathList);
+ newJson = createSelectedFilesJson(fateTxId, initiallySelectedAll,
filesPathList);
+ assertNotEquals(actualMetadataValue, newJson,
+ "Test json should have reverse file order of actual metadata");
+
+ // write the json with reverse file order
+ try (BatchWriter bw = client.createBatchWriter(MetadataTable.NAME)) {
+ Mutation mutation = new Mutation(row);
+ mutation.put(selectedColumnFamily, selectedColumnQualifier,
+ new Value(newJson.getBytes(UTF_8)));
+ bw.addMutation(mutation);
+ }
+
+ // verify the metadata has been changed
+ actualMetadataValue = selectedMetadataValue.get();
+ assertEquals(newJson, actualMetadataValue,
+ "Value should be equal to the new json we manually wrote above");
+
+ // submit a mutation with the condition that the selected files match
what was originally
+ // written
+ TabletMetadata tm1 =
+
TabletMetadata.builder(e1).putSelectedFiles(selectedFiles).build(SELECTED);
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().requireSame(tm1,
SELECTED).deleteFile(stf1)
Review Comment:
Instead of doing `.deleteFile(stf1)` something could be added or set on the
tablet metadata. Then later after verifying the mutation was accepted that
could be checked to ensure the change is there.
--
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]
