keith-turner commented on code in PR #5284: URL: https://github.com/apache/accumulo/pull/5284#discussion_r1929591111
########## core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeabilityInfo.java: ########## @@ -0,0 +1,112 @@ +/* + * 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.client.admin; + +import java.time.Duration; +import java.util.Objects; +import java.util.Optional; +import java.util.function.Supplier; + +import org.apache.accumulo.core.clientImpl.TabletMergeabilityUtil; + +import com.google.common.base.Preconditions; + +/** + * @since 4.0.0 + */ +public class TabletMergeabilityInfo { + + private final TabletMergeability tabletMergeability; + private final Optional<Duration> insertionTime; + private final Supplier<Duration> currentTime; + + public TabletMergeabilityInfo(TabletMergeability tabletMergeability, + Optional<Duration> insertionTime, Supplier<Duration> currentTime) { + this.tabletMergeability = Objects.requireNonNull(tabletMergeability); + this.insertionTime = Objects.requireNonNull(insertionTime); + this.currentTime = Objects.requireNonNull(currentTime); + // This makes sure that insertionTime is set if TabletMergeability has a delay, and is empty + // if TabletMergeability is NEVER + Preconditions.checkArgument(tabletMergeability.isNever() == insertionTime.isEmpty(), + "insertionTime must not be empty if and only if TabletMergeability delay is >= 0"); + } + + /** + * @return the TabletMergeability + */ + public TabletMergeability getTabletMergeability() { + return tabletMergeability; + } + + /** + * Returns the time of the Manager when the TabletMergeability was set on the tablet. This will be + * empty if TabletMergeability is set to never + * + * @return the insertion time is set or else empty + */ + public Optional<Duration> getInsertionTime() { + return insertionTime; + } + + /** + * Returns the current time of the Manager + * + * @return the current time + */ + public Duration getCurrentTime() { + return currentTime.get(); + } Review Comment: Understanding what getInsertionTime() and getCurrentTime() mean would require a lot of knowledge of Accumulo internals. Tried to come up with something that requires less understanding and came up with the following, but not sure about the name of the method. ```suggestion /** * If the tabletTabletMergeability is configured with a delay returns an estimate of the elapsed * time since the delay was initially set. Returns optional.empty() if the tablet was configured * with a TabletMergability of never. */ public Optional<Duration> elapsed() { // its possible a the "current time" is read from the manager and then a tablets insertion time // is read much later and this could cause a negative read, in this case set the elapsed time to zero. // Avoiding this would require an RPC per a call to this method to get the latest manager time, so // since this is an estimate return zero for this case. return insertionTime.map(it -> currentTime.get().minus(it)) .map(elapsed -> elapsed.isNegative() ? Duration.ZERO : elapsed); } ``` ########## test/src/main/java/org/apache/accumulo/test/functional/AddSplitIT.java: ########## @@ -124,22 +132,165 @@ public void addSplitWithMergeabilityTest() throws Exception { assertEquals(splits.keySet(), new TreeSet<>(c.tableOperations().listSplits(tableName))); TableId id = TableId.of(c.tableOperations().tableIdMap().get(tableName)); - try (TabletsMetadata tm = getServerContext().getAmple().readTablets().forTable(id).build()) { - tm.stream().forEach(t -> { - // default tablet should be set to never - if (t.getEndRow() == null) { - assertEquals(TabletMergeability.never(), - t.getTabletMergeability().getTabletMergeability()); - } else { - // New splits should match the original setting in the map - assertEquals(splits.get(t.getEndRow()), - t.getTabletMergeability().getTabletMergeability()); - } - }); - } + verifySplits(id, splits); + verifySplitsWithApi(c, tableName, splits); + } } + @Test + public void updateSplitWithMergeabilityTest() throws Exception { + String tableName = getUniqueNames(1)[0]; + try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) { + c.tableOperations().create(tableName); + + NavigableMap<Text,TabletMergeability> splits = new TreeMap<>(); + splits.put(new Text(String.format("%09d", 333)), TabletMergeability.never()); + splits.put(new Text(String.format("%09d", 666)), TabletMergeability.never()); + splits.put(new Text(String.format("%09d", 999)), + TabletMergeability.after(Duration.ofDays(10))); + + c.tableOperations().putSplits(tableName, splits); + Thread.sleep(100); + assertEquals(splits.keySet(), new TreeSet<>(c.tableOperations().listSplits(tableName))); + + TableId id = TableId.of(c.tableOperations().tableIdMap().get(tableName)); + verifySplits(id, splits); + + // Set all splits to system default (always) and update + var splitUpdates = TabletMergeabilityUtil.systemDefaultSplits(splits.navigableKeySet()); + c.tableOperations().putSplits(tableName, splitUpdates); + verifySplits(id, splitUpdates); + verifySplitsWithApi(c, tableName, splitUpdates); + + // Update two existing and add two new splits + splits.put(new Text(String.format("%09d", 666)), + TabletMergeability.after(Duration.ofHours(10))); + splits.put(new Text(String.format("%09d", 999)), + TabletMergeability.after(Duration.ofMinutes(7))); + splits.put(new Text(String.format("%09d", 444)), TabletMergeability.always()); + splits.put(new Text(String.format("%09d", 777)), + TabletMergeability.after(Duration.ofMinutes(5))); + + c.tableOperations().putSplits(tableName, splits); + verifySplits(id, splits); + verifySplitsWithApi(c, tableName, splits); + + // Update with same and make sure insertion time updated + splits.clear(); + var split = new Text(String.format("%09d", 777)); + splits.put(split, TabletMergeability.after(Duration.ofMinutes(5))); + + var originalTmi = c.tableOperations().getTabletInformation(tableName, new Range(split)) + .findFirst().orElseThrow().getTabletMergeabilityInfo(); + // getCurrentTime() uses a supplier and delays until invoking so trigger here + assertTrue(originalTmi.getCurrentTime().getNano() > 0); + + // Update existing with same delay of 5 minutes + c.tableOperations().putSplits(tableName, splits); + var updatedTmi = c.tableOperations().getTabletInformation(tableName, new Range(split)) + .findFirst().orElseThrow().getTabletMergeabilityInfo(); + + // TabletMergeability setting should be the same but the insertion and current time + // should be different + assertEquals(originalTmi.getTabletMergeability(), updatedTmi.getTabletMergeability()); + assertTrue(updatedTmi.getInsertionTime().orElseThrow() + .compareTo(originalTmi.getInsertionTime().orElseThrow()) > 0); + // we previously called getCurrentTime() on the originalTmi object so the updated one + // should be newer + assertTrue(updatedTmi.getCurrentTime().compareTo(originalTmi.getCurrentTime()) > 0); + } + } + + @Test + public void addSplitIsMergeableTest() throws Exception { + String tableName = getUniqueNames(1)[0]; + try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) { + c.tableOperations().create(tableName); + + var split1 = new Text(String.format("%09d", 333)); + var split2 = new Text(String.format("%09d", 666)); + var split3 = new Text(String.format("%09d", 888)); + var split4 = new Text(String.format("%09d", 999)); + + SortedMap<Text,TabletMergeability> splits = new TreeMap<>(); + splits.put(split1, TabletMergeability.always()); + splits.put(split2, TabletMergeability.never()); + splits.put(split3, TabletMergeability.after(Duration.ofMillis(1))); + splits.put(split4, TabletMergeability.after(Duration.ofDays(1))); + + c.tableOperations().putSplits(tableName, splits); + Thread.sleep(100); + assertEquals(splits.keySet(), new TreeSet<>(c.tableOperations().listSplits(tableName))); + + var tableInfo = c.tableOperations().getTabletInformation(tableName, new Range()) + .collect(Collectors.toMap(ti -> ti.getTabletId().getEndRow(), Function.identity())); + + // Set to always + var split1Tmi = tableInfo.get(split1).getTabletMergeabilityInfo(); + assertTrue(split1Tmi.isMergeable()); + assertTrue(split1Tmi.getInsertionTime().isPresent()); + assertTrue( + split1Tmi.getCurrentTime().compareTo(split1Tmi.getInsertionTime().orElseThrow()) > 0); + + // Set to never + var split2Tmi = tableInfo.get(split2).getTabletMergeabilityInfo(); + assertFalse(split2Tmi.isMergeable()); + assertFalse(split2Tmi.getInsertionTime().isPresent()); + + // Set to a delay of 1 ms and current time should have elapsed long enough + var split3Tmi = tableInfo.get(split3).getTabletMergeabilityInfo(); + assertTrue(split3Tmi.isMergeable()); + assertTrue(split3Tmi.getInsertionTime().isPresent()); + assertTrue( + split3Tmi.getCurrentTime().compareTo(split3Tmi.getInsertionTime().orElseThrow()) > 0); + + // Set to a delay of 1 day and current time has NOT elapsed long enough + var split4Tmi = tableInfo.get(split4).getTabletMergeabilityInfo(); + assertFalse(split4Tmi.isMergeable()); + assertTrue(split4Tmi.getInsertionTime().isPresent()); + assertTrue( + split4Tmi.getCurrentTime().compareTo(split4Tmi.getInsertionTime().orElseThrow()) > 0); + + } + } + + // Checks that TabletMergeability in metadata matches split settings in the map + private void verifySplits(TableId id, SortedMap<Text,TabletMergeability> splits) { + try (TabletsMetadata tm = getServerContext().getAmple().readTablets().forTable(id).build()) { + tm.stream().forEach(t -> { + // default tablet should be set to never + if (t.getEndRow() == null) { + assertEquals(TabletMergeability.never(), + t.getTabletMergeability().getTabletMergeability()); + } else { + // New splits should match the original setting in the map + assertEquals(splits.get(t.getEndRow()), + t.getTabletMergeability().getTabletMergeability()); + } + }); + } + } + + private void verifySplitsWithApi(AccumuloClient c, String tableName, Review Comment: This function could also check that everything in the splits map is seen. ########## core/src/main/thrift/manager.thrift: ########## @@ -445,4 +452,21 @@ service ManagerClientService { 1:client.ThriftSecurityException sec 2:client.ThriftTableOperationException toe ) + + list<data.TKeyExtent> updateTabletMergeability( + 1:client.TInfo tinfo + 2:security.TCredentials credentials + 3:string tableName + 4:map<data.TKeyExtent,TTabletMergeability> splits + ) throws ( + 1:client.ThriftSecurityException sec + 2:client.ThriftTableOperationException toe + ) + + i64 getManagerTime( Review Comment: Could add the unit to the method name. ```suggestion i64 getManagerTimeNanos( ``` ########## test/src/main/java/org/apache/accumulo/test/functional/AddSplitIT.java: ########## @@ -124,22 +132,165 @@ public void addSplitWithMergeabilityTest() throws Exception { assertEquals(splits.keySet(), new TreeSet<>(c.tableOperations().listSplits(tableName))); TableId id = TableId.of(c.tableOperations().tableIdMap().get(tableName)); - try (TabletsMetadata tm = getServerContext().getAmple().readTablets().forTable(id).build()) { - tm.stream().forEach(t -> { - // default tablet should be set to never - if (t.getEndRow() == null) { - assertEquals(TabletMergeability.never(), - t.getTabletMergeability().getTabletMergeability()); - } else { - // New splits should match the original setting in the map - assertEquals(splits.get(t.getEndRow()), - t.getTabletMergeability().getTabletMergeability()); - } - }); - } + verifySplits(id, splits); + verifySplitsWithApi(c, tableName, splits); + } } + @Test + public void updateSplitWithMergeabilityTest() throws Exception { + String tableName = getUniqueNames(1)[0]; + try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) { + c.tableOperations().create(tableName); + + NavigableMap<Text,TabletMergeability> splits = new TreeMap<>(); + splits.put(new Text(String.format("%09d", 333)), TabletMergeability.never()); + splits.put(new Text(String.format("%09d", 666)), TabletMergeability.never()); + splits.put(new Text(String.format("%09d", 999)), + TabletMergeability.after(Duration.ofDays(10))); + + c.tableOperations().putSplits(tableName, splits); + Thread.sleep(100); + assertEquals(splits.keySet(), new TreeSet<>(c.tableOperations().listSplits(tableName))); + + TableId id = TableId.of(c.tableOperations().tableIdMap().get(tableName)); + verifySplits(id, splits); + + // Set all splits to system default (always) and update + var splitUpdates = TabletMergeabilityUtil.systemDefaultSplits(splits.navigableKeySet()); + c.tableOperations().putSplits(tableName, splitUpdates); + verifySplits(id, splitUpdates); + verifySplitsWithApi(c, tableName, splitUpdates); + + // Update two existing and add two new splits + splits.put(new Text(String.format("%09d", 666)), + TabletMergeability.after(Duration.ofHours(10))); + splits.put(new Text(String.format("%09d", 999)), + TabletMergeability.after(Duration.ofMinutes(7))); + splits.put(new Text(String.format("%09d", 444)), TabletMergeability.always()); + splits.put(new Text(String.format("%09d", 777)), + TabletMergeability.after(Duration.ofMinutes(5))); Review Comment: The follownig may cover more cases. ```suggestion splits.put(new Text(String.format("%09d", 333)), TabletMergeability.after(Duration.ofHours(11))); splits.put(new Text(String.format("%09d", 666)), TabletMergeability.after(Duration.ofHours(10))); splits.put(new Text(String.format("%09d", 444)), TabletMergeability.always()); splits.put(new Text(String.format("%09d", 777)), TabletMergeability.after(Duration.ofMinutes(5))); ``` Trying to cover the following cases, think the above get 3 of 4. * For an existing tablet only update its end row. * For an existing tablet update its end row and add a split in its range. * For an existing tablet update only add a split in it range, do not update its end row. * For an existing tablet make no updates in its range or to its end row. ########## test/src/main/java/org/apache/accumulo/test/functional/AddSplitIT.java: ########## @@ -124,22 +132,165 @@ public void addSplitWithMergeabilityTest() throws Exception { assertEquals(splits.keySet(), new TreeSet<>(c.tableOperations().listSplits(tableName))); TableId id = TableId.of(c.tableOperations().tableIdMap().get(tableName)); - try (TabletsMetadata tm = getServerContext().getAmple().readTablets().forTable(id).build()) { - tm.stream().forEach(t -> { - // default tablet should be set to never - if (t.getEndRow() == null) { - assertEquals(TabletMergeability.never(), - t.getTabletMergeability().getTabletMergeability()); - } else { - // New splits should match the original setting in the map - assertEquals(splits.get(t.getEndRow()), - t.getTabletMergeability().getTabletMergeability()); - } - }); - } + verifySplits(id, splits); + verifySplitsWithApi(c, tableName, splits); + } } + @Test + public void updateSplitWithMergeabilityTest() throws Exception { + String tableName = getUniqueNames(1)[0]; + try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) { + c.tableOperations().create(tableName); + + NavigableMap<Text,TabletMergeability> splits = new TreeMap<>(); + splits.put(new Text(String.format("%09d", 333)), TabletMergeability.never()); + splits.put(new Text(String.format("%09d", 666)), TabletMergeability.never()); + splits.put(new Text(String.format("%09d", 999)), + TabletMergeability.after(Duration.ofDays(10))); + + c.tableOperations().putSplits(tableName, splits); + Thread.sleep(100); + assertEquals(splits.keySet(), new TreeSet<>(c.tableOperations().listSplits(tableName))); + + TableId id = TableId.of(c.tableOperations().tableIdMap().get(tableName)); + verifySplits(id, splits); + + // Set all splits to system default (always) and update + var splitUpdates = TabletMergeabilityUtil.systemDefaultSplits(splits.navigableKeySet()); + c.tableOperations().putSplits(tableName, splitUpdates); + verifySplits(id, splitUpdates); + verifySplitsWithApi(c, tableName, splitUpdates); + + // Update two existing and add two new splits + splits.put(new Text(String.format("%09d", 666)), + TabletMergeability.after(Duration.ofHours(10))); + splits.put(new Text(String.format("%09d", 999)), + TabletMergeability.after(Duration.ofMinutes(7))); + splits.put(new Text(String.format("%09d", 444)), TabletMergeability.always()); + splits.put(new Text(String.format("%09d", 777)), + TabletMergeability.after(Duration.ofMinutes(5))); + + c.tableOperations().putSplits(tableName, splits); + verifySplits(id, splits); + verifySplitsWithApi(c, tableName, splits); + + // Update with same and make sure insertion time updated + splits.clear(); + var split = new Text(String.format("%09d", 777)); + splits.put(split, TabletMergeability.after(Duration.ofMinutes(5))); + + var originalTmi = c.tableOperations().getTabletInformation(tableName, new Range(split)) + .findFirst().orElseThrow().getTabletMergeabilityInfo(); + // getCurrentTime() uses a supplier and delays until invoking so trigger here + assertTrue(originalTmi.getCurrentTime().getNano() > 0); + + // Update existing with same delay of 5 minutes + c.tableOperations().putSplits(tableName, splits); + var updatedTmi = c.tableOperations().getTabletInformation(tableName, new Range(split)) + .findFirst().orElseThrow().getTabletMergeabilityInfo(); + + // TabletMergeability setting should be the same but the insertion and current time + // should be different + assertEquals(originalTmi.getTabletMergeability(), updatedTmi.getTabletMergeability()); + assertTrue(updatedTmi.getInsertionTime().orElseThrow() + .compareTo(originalTmi.getInsertionTime().orElseThrow()) > 0); + // we previously called getCurrentTime() on the originalTmi object so the updated one + // should be newer + assertTrue(updatedTmi.getCurrentTime().compareTo(originalTmi.getCurrentTime()) > 0); + } + } + + @Test + public void addSplitIsMergeableTest() throws Exception { + String tableName = getUniqueNames(1)[0]; + try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) { + c.tableOperations().create(tableName); + + var split1 = new Text(String.format("%09d", 333)); + var split2 = new Text(String.format("%09d", 666)); + var split3 = new Text(String.format("%09d", 888)); + var split4 = new Text(String.format("%09d", 999)); + + SortedMap<Text,TabletMergeability> splits = new TreeMap<>(); + splits.put(split1, TabletMergeability.always()); + splits.put(split2, TabletMergeability.never()); + splits.put(split3, TabletMergeability.after(Duration.ofMillis(1))); + splits.put(split4, TabletMergeability.after(Duration.ofDays(1))); + + c.tableOperations().putSplits(tableName, splits); + Thread.sleep(100); + assertEquals(splits.keySet(), new TreeSet<>(c.tableOperations().listSplits(tableName))); + + var tableInfo = c.tableOperations().getTabletInformation(tableName, new Range()) + .collect(Collectors.toMap(ti -> ti.getTabletId().getEndRow(), Function.identity())); + + // Set to always + var split1Tmi = tableInfo.get(split1).getTabletMergeabilityInfo(); + assertTrue(split1Tmi.isMergeable()); + assertTrue(split1Tmi.getInsertionTime().isPresent()); + assertTrue( + split1Tmi.getCurrentTime().compareTo(split1Tmi.getInsertionTime().orElseThrow()) > 0); + + // Set to never + var split2Tmi = tableInfo.get(split2).getTabletMergeabilityInfo(); + assertFalse(split2Tmi.isMergeable()); + assertFalse(split2Tmi.getInsertionTime().isPresent()); + + // Set to a delay of 1 ms and current time should have elapsed long enough + var split3Tmi = tableInfo.get(split3).getTabletMergeabilityInfo(); + assertTrue(split3Tmi.isMergeable()); + assertTrue(split3Tmi.getInsertionTime().isPresent()); + assertTrue( + split3Tmi.getCurrentTime().compareTo(split3Tmi.getInsertionTime().orElseThrow()) > 0); + + // Set to a delay of 1 day and current time has NOT elapsed long enough + var split4Tmi = tableInfo.get(split4).getTabletMergeabilityInfo(); + assertFalse(split4Tmi.isMergeable()); + assertTrue(split4Tmi.getInsertionTime().isPresent()); + assertTrue( + split4Tmi.getCurrentTime().compareTo(split4Tmi.getInsertionTime().orElseThrow()) > 0); + + } + } + + // Checks that TabletMergeability in metadata matches split settings in the map + private void verifySplits(TableId id, SortedMap<Text,TabletMergeability> splits) { Review Comment: This verify function checks that splits it sees in the table match what is in the map. It could also check that all split in the map actually exist in the table. -- 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]
