keith-turner commented on code in PR #5355: URL: https://github.com/apache/accumulo/pull/5355#discussion_r1970339187
########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + String fmtTid = FateTxId.formatTid(1234L); + List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, + new KeyExtent(tid, new Text("c"), null), tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(0), tablets.get(0)); + assertEquals(tm.get(1), tablets.get(1)); + assertEquals(tm.get(2), tablets.get(2)); Review Comment: Could use subList to shorten this check ```suggestion assertEquals(tm.subList(0,3), tablets); ``` ########## server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFiles.java: ########## @@ -328,25 +365,19 @@ private long loadFiles(TableId tableId, Path bulkDir, LoadMappingIterator loadMa String fmtTid = FateTxId.formatTid(tid); log.trace("{}: Starting bulk load at row: {}", fmtTid, startRow); - Iterator<TabletMetadata> tabletIter = - TabletsMetadata.builder(manager.getContext()).forTable(tableId).overlapping(startRow, null) - .checkConsistency().fetch(PREV_ROW, LOCATION, LOADED).build().iterator(); + loader.start(bulkDir, manager, tid, bulkInfo.setTime); + long t1 = System.currentTimeMillis(); - Loader loader; - if (bulkInfo.tableState == TableState.ONLINE) { - loader = new OnlineLoader(); - } else { - loader = new OfflineLoader(); - } + try (TabletsMetadata tabletsMetadata = factory.newTabletsMetadata(startRow)) { Review Comment: This will call close on `tabletsMetadata` eventually. Seems like the close method on TabletsMetadataFactory can be removed as it does not seem to be used. ########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + String fmtTid = FateTxId.formatTid(1234L); + List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, + new KeyExtent(tid, new Text("c"), null), tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(0), tablets.get(0)); + assertEquals(tm.get(1), tablets.get(1)); + assertEquals(tm.get(2), tablets.get(2)); + + tablets = LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, null, new Text("x")), + tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(tm.size() - 3), tablets.get(tablets.size() - 3)); + assertEquals(tm.get(tm.size() - 2), tablets.get(tablets.size() - 2)); + assertEquals(tm.get(tm.size() - 1), tablets.get(tablets.size() - 1)); Review Comment: Could also use subList here ```suggestion assertEquals(tm.subList(tm.size()-3, tm.size()), tablets); ``` ########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + String fmtTid = FateTxId.formatTid(1234L); + List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, + new KeyExtent(tid, new Text("c"), null), tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(0), tablets.get(0)); + assertEquals(tm.get(1), tablets.get(1)); + assertEquals(tm.get(2), tablets.get(2)); + Review Comment: could add another test to find some tablets in the middle. The existing test find tablets at the beginning and end. ########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + String fmtTid = FateTxId.formatTid(1234L); + List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, + new KeyExtent(tid, new Text("c"), null), tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(0), tablets.get(0)); + assertEquals(tm.get(1), tablets.get(1)); + assertEquals(tm.get(2), tablets.get(2)); + + tablets = LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, null, new Text("x")), + tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(tm.size() - 3), tablets.get(tablets.size() - 3)); + assertEquals(tm.get(tm.size() - 2), tablets.get(tablets.size() - 2)); + assertEquals(tm.get(tm.size() - 1), tablets.get(tablets.size() - 1)); + + tablets = + LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, null, null), tm.iterator()); + assertEquals(tm, tablets); + + } + + @Test + public void testLoadFiles() throws Exception { + + TableId tid = TableId.of("1"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + TabletsMetadata tabletMeta = new TestTabletsMetadata(null, tm); + + Map<KeyExtent,String> loadRanges = new HashMap<>(); + loadRanges.put(nke(null, "c"), "f1 f2"); + loadRanges.put(nke("c", "g"), "f2 f3"); + loadRanges.put(nke("g", "l"), "f2 f4"); + loadRanges.put(nke("l", "n"), "f1 f2 f4 f5"); + loadRanges.put(nke("n", "r"), "f2 f4"); + loadRanges.put(nke("r", "w"), "f2 f5"); + loadRanges.put(nke("w", null), "f2 f6"); + LoadMappingIterator lmi = PrepBulkImportTest.createLoadMappingIter(loadRanges); + + CaptureLoader cl = new CaptureLoader(); + BulkInfo info = new BulkInfo(); + TabletsMetadataFactory tmf = new TabletsMetadataFactory() { + + @Override + public TabletsMetadata newTabletsMetadata(Text startRow) { + return tabletMeta; + } + + @Override + public void close() { + tabletMeta.close(); + } + + }; + long txid = 1234L; + + Manager manager = EasyMock.createMock(Manager.class); + Path bulkDir = EasyMock.createMock(Path.class); + EasyMock.replay(manager, bulkDir); + + LoadFiles.loadFiles(cl, info, bulkDir, lmi, tmf, manager, txid); + EasyMock.verify(manager, bulkDir); + + List<CaptureLoader.LoadResult> results = cl.getLoadResults(); + assertEquals(loadRanges.size(), results.size()); + + Map<String,HashSet<KeyExtent>> loadFileToExtentMap = new HashMap<>(); + for (CaptureLoader.LoadResult result : results) { + for (FileInfo file : result.getFiles()) { + HashSet<KeyExtent> extents = + loadFileToExtentMap.computeIfAbsent(file.getFileName(), fileName -> new HashSet<>()); + result.getTablets().forEach(m -> extents.add(m.getExtent())); + } + } + HashSet<KeyExtent> extents = loadFileToExtentMap.get("f1"); + assertNotNull(extents); + assertEquals(5, extents.size()); + assertTrue(extents.contains(nke(null, "a"))); + assertTrue(extents.contains(nke("a", "b"))); + assertTrue(extents.contains(nke("b", "c"))); + assertTrue(extents.contains(nke("l", "m"))); + assertTrue(extents.contains(nke("m", "n"))); Review Comment: Could shorten using Set.of ```suggestion assertEquals(Set.of(nke(null, "a"), nke("a", "b"), nke("b", "c"),nke("l", "m"),nke("m", "n")), extents); ``` ########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + String fmtTid = FateTxId.formatTid(1234L); + List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, + new KeyExtent(tid, new Text("c"), null), tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(0), tablets.get(0)); + assertEquals(tm.get(1), tablets.get(1)); + assertEquals(tm.get(2), tablets.get(2)); + + tablets = LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, null, new Text("x")), + tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(tm.size() - 3), tablets.get(tablets.size() - 3)); + assertEquals(tm.get(tm.size() - 2), tablets.get(tablets.size() - 2)); + assertEquals(tm.get(tm.size() - 1), tablets.get(tablets.size() - 1)); + + tablets = + LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, null, null), tm.iterator()); + assertEquals(tm, tablets); + + } + + @Test + public void testLoadFiles() throws Exception { + + TableId tid = TableId.of("1"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + TabletsMetadata tabletMeta = new TestTabletsMetadata(null, tm); + + Map<KeyExtent,String> loadRanges = new HashMap<>(); + loadRanges.put(nke(null, "c"), "f1 f2"); + loadRanges.put(nke("c", "g"), "f2 f3"); + loadRanges.put(nke("g", "l"), "f2 f4"); + loadRanges.put(nke("l", "n"), "f1 f2 f4 f5"); + loadRanges.put(nke("n", "r"), "f2 f4"); + loadRanges.put(nke("r", "w"), "f2 f5"); + loadRanges.put(nke("w", null), "f2 f6"); Review Comment: This load mapping covers all data in the table w/ no gaps. Need to test that case. Additionally it would be nice to test other load mappings w/ gaps and not starting ending at -inf or +inf. ########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + String fmtTid = FateTxId.formatTid(1234L); + List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, + new KeyExtent(tid, new Text("c"), null), tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(0), tablets.get(0)); + assertEquals(tm.get(1), tablets.get(1)); + assertEquals(tm.get(2), tablets.get(2)); + + tablets = LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, null, new Text("x")), + tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(tm.size() - 3), tablets.get(tablets.size() - 3)); + assertEquals(tm.get(tm.size() - 2), tablets.get(tablets.size() - 2)); + assertEquals(tm.get(tm.size() - 1), tablets.get(tablets.size() - 1)); + + tablets = + LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, null, null), tm.iterator()); + assertEquals(tm, tablets); + + } + + @Test + public void testLoadFiles() throws Exception { + + TableId tid = TableId.of("1"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { Review Comment: ```suggestion for (int i = 'a'; i < 'z'; i++) { ``` ########## server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFiles.java: ########## @@ -80,6 +79,14 @@ */ class LoadFiles extends ManagerRepo { + // visible for testing + interface TabletsMetadataFactory { + + TabletsMetadata newTabletsMetadata(Text startRow); + + void close(); Review Comment: May be able to remove this, made a comment elsewhere about why. ########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + String fmtTid = FateTxId.formatTid(1234L); + List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, + new KeyExtent(tid, new Text("c"), null), tm.iterator()); Review Comment: Would be good to check the iterator is in the expected place after calling findOverlappingTablets ```suggestion var iter = tm.iterator(); List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, new Text("c"), null), iter); assertEquals(tm.get(3), iter.next()); ``` ########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { Review Comment: ```suggestion for (int i = 'a'; i < 'z'; i++) { ``` ########## server/manager/src/test/java/org/apache/accumulo/manager/tableOps/bulkVer2/LoadFilesTest.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.manager.tableOps.bulkVer2; + +import static org.apache.accumulo.manager.tableOps.bulkVer2.PrepBulkImportTest.nke; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + +import org.apache.accumulo.core.clientImpl.bulk.Bulk.FileInfo; +import org.apache.accumulo.core.clientImpl.bulk.Bulk.Files; +import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator; +import org.apache.accumulo.core.data.TableId; +import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.fate.FateTxId; +import org.apache.accumulo.core.metadata.schema.TabletMetadata; +import org.apache.accumulo.core.metadata.schema.TabletsMetadata; +import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.manager.tableOps.bulkVer2.LoadFiles.TabletsMetadataFactory; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +public class LoadFilesTest { + + public static class TestTabletsMetadata extends TabletsMetadata { + + public TestTabletsMetadata(AutoCloseable closeable, Iterable<TabletMetadata> tmi) { + super(closeable, tmi); + } + + } + + private static class CaptureLoader extends LoadFiles.Loader { + + private static class LoadResult { + private final List<TabletMetadata> tablets; + private final Files files; + + public LoadResult(List<TabletMetadata> tablets, Files files) { + super(); + this.tablets = tablets; + this.files = files; + } + + public List<TabletMetadata> getTablets() { + return tablets; + } + + public Files getFiles() { + return files; + } + + } + + private final List<LoadResult> results = new ArrayList<>(); + + @Override + void load(List<TabletMetadata> tablets, Files files) { + results.add(new LoadResult(tablets, files)); + } + + public List<LoadResult> getLoadResults() { + return results; + } + + @Override + long finish() { + return 0; + } + + } + + @Test + public void testFindOverlappingFiles() { + + TableId tid = TableId.of("1a"); + List<TabletMetadata> tm = new ArrayList<>(); + tm.add(TabletMetadata.create(tid.canonical(), null, "a")); + for (int i = 97; i < 122; i++) { + tm.add(TabletMetadata.create(tid.canonical(), "" + (char) (i), "" + (char) (i + 1))); + } + tm.add(TabletMetadata.create(tid.canonical(), "z", null)); + + String fmtTid = FateTxId.formatTid(1234L); + List<TabletMetadata> tablets = LoadFiles.findOverlappingTablets(fmtTid, + new KeyExtent(tid, new Text("c"), null), tm.iterator()); + assertEquals(3, tablets.size()); + assertEquals(tm.get(0), tablets.get(0)); + assertEquals(tm.get(1), tablets.get(1)); + assertEquals(tm.get(2), tablets.get(2)); + + tablets = LoadFiles.findOverlappingTablets(fmtTid, new KeyExtent(tid, null, new Text("x")), + tm.iterator()); Review Comment: Could also check the iterator here, should not have a next after this call. -- 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]
