ctubbsii commented on code in PR #5197: URL: https://github.com/apache/accumulo/pull/5197#discussion_r1956765989
########## core/src/test/java/org/apache/accumulo/core/clientImpl/bulk/LoadMappingIteratorTest.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.clientImpl.bulk; + +import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.SortedMap; +import java.util.TreeMap; + +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.hadoop.io.Text; +import org.junit.jupiter.api.Test; + +public class LoadMappingIteratorTest { + private LoadMappingIterator createLoadMappingIter(Map<KeyExtent,String> loadRanges) + throws IOException { + SortedMap<KeyExtent,Bulk.Files> mapping = new TreeMap<>(); + + loadRanges.forEach((extent, files) -> { + Bulk.Files testFiles = new Bulk.Files(); + long c = 0L; + for (String f : files.split(" ")) { + c++; + testFiles.add(new Bulk.FileInfo(f, c, c)); + } + + mapping.put(extent, testFiles); + }); + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + BulkSerialize.writeLoadMapping(mapping, "/some/dir", p -> baos); + ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray()); + LoadMappingIterator lmi = + BulkSerialize.readLoadMapping("/some/dir", TableId.of("1"), p -> bais); + return lmi; + } + + KeyExtent nke(String prev, String end) { + Text per = prev == null ? null : new Text(prev); + Text er = end == null ? null : new Text(end); + + return new KeyExtent(TableId.of("1"), er, per); + } + + @Test + void testValidOrderedInput() throws IOException { + Map<KeyExtent,String> loadRanges = new LinkedHashMap<>(); + loadRanges.put(nke(null, "c"), "f1 f2"); + loadRanges.put(nke("c", "g"), "f2 f3"); + loadRanges.put(nke("g", "r"), "f2 f4"); + loadRanges.put(nke("r", "w"), "f2 f5"); + loadRanges.put(nke("w", null), "f2 f6"); + + try (LoadMappingIterator iterator = createLoadMappingIter(loadRanges)) { + List<KeyExtent> result = new ArrayList<>(); + while (iterator.hasNext()) { + result.add(iterator.next().getKey()); + } + assertEquals(new ArrayList<>(loadRanges.keySet()), result); + } + } + + @Test + void testInvalidOutOfOrderInput() throws IOException { + Map<KeyExtent,String> loadRanges = new LinkedHashMap<>(); + loadRanges.put(nke("c", "g"), "f2 f3"); + loadRanges.put(nke(null, "c"), "f1 f2"); // Out of order! + loadRanges.put(nke("g", "r"), "f2 f4"); + loadRanges.put(nke("r", "w"), "f2 f5"); + loadRanges.put(nke("w", null), "f2 f6"); + + try (LoadMappingIterator iterator = createLoadMappingIter(loadRanges)) { + KeyExtent previous = null; + while (iterator.hasNext()) { + KeyExtent current = iterator.next().getKey(); + System.out.println("Comparing " + current + " with previous: " + previous); + assertFalse(previous != null && current.compareTo(previous) < 0, "Input is out of order!"); + + previous = current; + } + } catch (IllegalStateException e) { + assertThrows(IllegalStateException.class, () -> { + throw e; + }, "Expected an IllegalStateException due to out-of-order KeyExtents"); + } Review Comment: This is not the correct way to use assertThrows. Please see the docs for assertThrows and examine other uses for examples. It's supposed to replace the try-catch block, not live inside the catch block. ########## core/src/test/java/org/apache/accumulo/core/clientImpl/bulk/LoadMappingIteratorTest.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.clientImpl.bulk; + +import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.SortedMap; +import java.util.TreeMap; + +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.hadoop.io.Text; +import org.junit.jupiter.api.Test; + +public class LoadMappingIteratorTest { + private LoadMappingIterator createLoadMappingIter(Map<KeyExtent,String> loadRanges) + throws IOException { + SortedMap<KeyExtent,Bulk.Files> mapping = new TreeMap<>(); + + loadRanges.forEach((extent, files) -> { + Bulk.Files testFiles = new Bulk.Files(); + long c = 0L; + for (String f : files.split(" ")) { + c++; + testFiles.add(new Bulk.FileInfo(f, c, c)); + } + + mapping.put(extent, testFiles); + }); + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + BulkSerialize.writeLoadMapping(mapping, "/some/dir", p -> baos); + ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray()); + LoadMappingIterator lmi = + BulkSerialize.readLoadMapping("/some/dir", TableId.of("1"), p -> bais); + return lmi; + } + + KeyExtent nke(String prev, String end) { + Text per = prev == null ? null : new Text(prev); + Text er = end == null ? null : new Text(end); + + return new KeyExtent(TableId.of("1"), er, per); + } + + @Test + void testValidOrderedInput() throws IOException { + Map<KeyExtent,String> loadRanges = new LinkedHashMap<>(); + loadRanges.put(nke(null, "c"), "f1 f2"); + loadRanges.put(nke("c", "g"), "f2 f3"); + loadRanges.put(nke("g", "r"), "f2 f4"); + loadRanges.put(nke("r", "w"), "f2 f5"); + loadRanges.put(nke("w", null), "f2 f6"); + + try (LoadMappingIterator iterator = createLoadMappingIter(loadRanges)) { + List<KeyExtent> result = new ArrayList<>(); + while (iterator.hasNext()) { + result.add(iterator.next().getKey()); + } + assertEquals(new ArrayList<>(loadRanges.keySet()), result); Review Comment: instead of populating a second data structure, you could just iterate through the mapping and the loadRanges at the same time, and assertEquals them one by one inside the while loop. Then, after the while loop, do a `assertFalse(loadRangesIter.hasNext());` to make sure there's nothing left that didn't make it in to the load mapping iterator. ########## core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/LoadMappingIterator.java: ########## @@ -72,7 +74,23 @@ public boolean hasNext() { @Override public Map.Entry<KeyExtent,Bulk.Files> next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } Review Comment: I think this is generally the user's responsibility to call. It doesn't actually protect much, because next() is not thread safe, and another thread could call next() successfully, and grab the last element, while this thread successfully makes it past this check and then immediately fails, just like it would have before this change. A better fix would be to surround the gson.fromJson with a try-catch and handle the exception it throws when there's no more elements in the reader, and convert that exception to a NoSuchElementException in the catch block for that. ########## core/src/test/java/org/apache/accumulo/core/clientImpl/bulk/LoadMappingIteratorTest.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.clientImpl.bulk; + +import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.SortedMap; +import java.util.TreeMap; + +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.hadoop.io.Text; +import org.junit.jupiter.api.Test; + +public class LoadMappingIteratorTest { + private LoadMappingIterator createLoadMappingIter(Map<KeyExtent,String> loadRanges) + throws IOException { + SortedMap<KeyExtent,Bulk.Files> mapping = new TreeMap<>(); + + loadRanges.forEach((extent, files) -> { + Bulk.Files testFiles = new Bulk.Files(); + long c = 0L; + for (String f : files.split(" ")) { + c++; + testFiles.add(new Bulk.FileInfo(f, c, c)); + } + + mapping.put(extent, testFiles); + }); + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + BulkSerialize.writeLoadMapping(mapping, "/some/dir", p -> baos); + ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray()); + LoadMappingIterator lmi = + BulkSerialize.readLoadMapping("/some/dir", TableId.of("1"), p -> bais); + return lmi; + } + + KeyExtent nke(String prev, String end) { + Text per = prev == null ? null : new Text(prev); + Text er = end == null ? null : new Text(end); + + return new KeyExtent(TableId.of("1"), er, per); + } + + @Test + void testValidOrderedInput() throws IOException { + Map<KeyExtent,String> loadRanges = new LinkedHashMap<>(); + loadRanges.put(nke(null, "c"), "f1 f2"); + loadRanges.put(nke("c", "g"), "f2 f3"); + loadRanges.put(nke("g", "r"), "f2 f4"); + loadRanges.put(nke("r", "w"), "f2 f5"); + loadRanges.put(nke("w", null), "f2 f6"); + + try (LoadMappingIterator iterator = createLoadMappingIter(loadRanges)) { + List<KeyExtent> result = new ArrayList<>(); + while (iterator.hasNext()) { + result.add(iterator.next().getKey()); + } + assertEquals(new ArrayList<>(loadRanges.keySet()), result); + } + } + + @Test + void testInvalidOutOfOrderInput() throws IOException { + Map<KeyExtent,String> loadRanges = new LinkedHashMap<>(); + loadRanges.put(nke("c", "g"), "f2 f3"); + loadRanges.put(nke(null, "c"), "f1 f2"); // Out of order! + loadRanges.put(nke("g", "r"), "f2 f4"); + loadRanges.put(nke("r", "w"), "f2 f5"); + loadRanges.put(nke("w", null), "f2 f6"); + + try (LoadMappingIterator iterator = createLoadMappingIter(loadRanges)) { + KeyExtent previous = null; + while (iterator.hasNext()) { + KeyExtent current = iterator.next().getKey(); + System.out.println("Comparing " + current + " with previous: " + previous); + assertFalse(previous != null && current.compareTo(previous) < 0, "Input is out of order!"); Review Comment: This basically implements the check in the implementation. That doesn't make for a good test. Instead, step through the iterator one by one and verify that it is the entry you expect, until you get to the one that is out of order and going to cause a problem, then use assertThrows on that to verify that it actually throws when it hits the out of order one. -- 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]
