keith-turner commented on code in PR #5678: URL: https://github.com/apache/accumulo/pull/5678#discussion_r2167304216
########## core/src/test/java/org/apache/accumulo/core/data/TabletIdTest.java: ########## @@ -0,0 +1,125 @@ +/* + * 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.data; + +import static org.junit.jupiter.api.Assertions.*; + +import org.apache.accumulo.core.dataImpl.TabletIdImpl; +import org.apache.hadoop.io.Text; +import org.junit.jupiter.api.Test; + +class TabletIdTest { + /** + * Tests the {@link TabletId#of(TableId, String, String)} method to verify that a {@link TabletId} + * object is created + */ + @Test + void testOfGivenStringArgs() { + TabletId tabletId = TabletId.of(TableId.of("2"), "b", "a"); + assertEquals("2", tabletId.getTable().canonical(), "Expected the Table ID to be 2"); + assertEquals(new Text("b"), tabletId.getEndRow(), "Expected endRow to be b"); + assertEquals(new Text("a"), tabletId.getPrevEndRow(), "Expected prevEndRow to be a"); + } + + /** + * Tests the {@link TabletId#of(TableId, Text, Text)} method to verify that a {@link TabletId} + * object is created. + */ + @Test + void testOfGivenTextArgs() { + TabletId tabletId = TabletId.of(TableId.of("2"), new Text("b"), new Text("a")); + assertEquals("2", tabletId.getTable().canonical(), "Expected the Table ID to be 2"); + assertEquals(new Text("b"), tabletId.getEndRow(), "Expected endRow to be b"); + assertEquals(new Text("a"), tabletId.getPrevEndRow(), "Expected prevEndRow to be a"); + } + + /** + * Tests the {@link TabletId#of(TableId, byte[], byte[])} method to verify that a {@link TabletId} + * object is created. + */ + @Test + void testOfGivenByteArrayArgs() { + TabletId tabletId = TabletId.of(TableId.of("2"), ("b").getBytes(), ("a".getBytes())); + assertEquals("2", tabletId.getTable().canonical(), "Expected the Table ID to be 2"); + assertEquals(new Text("b"), tabletId.getEndRow(), "Expected endRow to be b"); + assertEquals(new Text("a"), tabletId.getPrevEndRow(), "Expected prevEndRow to be a"); + } + + /** + * Test when null is given for the endRow/prevEndRow, that it is treated as an infinite range, and + * any other TabletId objects with the same TableId are within range, no matter the values for + * endRow and prevEndRow. The values of endRow and prevEndRow must be null or cast to a type to + * avoid ambiguous references. + */ + @Test + void testInfiniteRanges() { + Text endRow = null; + Text prevEndRow = null; + TabletId tabletId = TabletId.of(TableId.of("2"), endRow, prevEndRow); + TabletId tabletId1 = TabletId.of(TableId.of("2"), "b", "a"); + TabletId tabletId2 = TabletId.of(TableId.of("2"), "z", "a"); + TabletId tabletId3 = TabletId.of(TableId.of("2"), new Text("z"), null); + + assertTrue((((TabletIdImpl) tabletId).toKeyExtent()) + .overlaps(((TabletIdImpl) tabletId1).toKeyExtent())); + assertTrue((((TabletIdImpl) tabletId).toKeyExtent()) + .overlaps(((TabletIdImpl) tabletId2).toKeyExtent())); + assertTrue((((TabletIdImpl) tabletId).toKeyExtent()) + .overlaps(((TabletIdImpl) tabletId3).toKeyExtent())); + } + + /** + * Tests that given a endRow equal to null, that {@link TabletId#getEndRow()} is equal to null. + */ + @Test + void testEndRowIsNull() { + Text endRow = null; + TabletId tabletId = TabletId.of(TableId.of("2"), endRow, new Text("a")); + + assertNull(tabletId.getEndRow(), "Expected endRow to be null"); Review Comment: Would be good to also check the table id and prev end row. We could have a bug where when the endRow is null we also set the prev end row to null. ########## core/src/test/java/org/apache/accumulo/core/data/TabletIdTest.java: ########## @@ -0,0 +1,125 @@ +/* + * 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.data; + +import static org.junit.jupiter.api.Assertions.*; + +import org.apache.accumulo.core.dataImpl.TabletIdImpl; +import org.apache.hadoop.io.Text; +import org.junit.jupiter.api.Test; + +class TabletIdTest { Review Comment: Would be nice to test compareTo. One way to do this is to create TabletIds w/ different table ids, different rows (null and not null), put them all in a list and sort the list and check the list is in the correct order. Would also be nice to have some test of equals() and hashCodes() methods of TabletId ########## core/src/main/java/org/apache/accumulo/core/data/TabletId.java: ########## @@ -26,6 +28,82 @@ * @since 1.7.0 */ public interface TabletId extends Comparable<TabletId> { + + /** + * Return a TabletId object for the provided TableId in range of endRow and prevEndRow. Generate + * ke using {@link KeyExtent#KeyExtent(TableId, Text, Text)}, to then return + * {@link TabletIdImpl#TabletIdImpl(KeyExtent)}. If the parameters endRow or prevEndRow are null, + * they represent a tablet over the range of -inf and inf respectively. + * + * @param tableId the ID for a table + * @param endRow the last row in this tablet, or null if this is the last tablet in this table + * @param prevEndRow he last row in the immediately preceding tablet for the table, or null if + * this represents the first tablet in this table + * @return Return the new {@link TabletId} object + */ + static TabletId of(TableId tableId, Text endRow, Text prevEndRow) { + KeyExtent ke = new KeyExtent(tableId, endRow, prevEndRow); + return new TabletIdImpl(ke); + } + + /** + * Return a TabletId object for the provided TableId in range of endRow and prevEndRow. Generate + * ke using {@link KeyExtent#KeyExtent(TableId, Text, Text)}, to then return + * {@link TabletIdImpl#TabletIdImpl(KeyExtent)}. If the parameters endRow or prevEndRow are null, + * they represent a tablet over the range of -inf and/or inf respectively. Does null checks to + * avoid getting an ambiguous Text reference error when null is passed through + * {@link Text#Text()}. Review Comment: Could omit this sentence as it seems like an internal impl detail. Stating how it behaves when nulls are passed seems sufficient to me. ```suggestion * they represent a tablet over the range of -inf and/or inf respectively. ``` ########## core/src/main/java/org/apache/accumulo/core/data/TabletId.java: ########## @@ -26,6 +28,82 @@ * @since 1.7.0 */ public interface TabletId extends Comparable<TabletId> { + + /** + * Return a TabletId object for the provided TableId in range of endRow and prevEndRow. Generate + * ke using {@link KeyExtent#KeyExtent(TableId, Text, Text)}, to then return + * {@link TabletIdImpl#TabletIdImpl(KeyExtent)}. If the parameters endRow or prevEndRow are null, + * they represent a tablet over the range of -inf and inf respectively. + * + * @param tableId the ID for a table + * @param endRow the last row in this tablet, or null if this is the last tablet in this table + * @param prevEndRow he last row in the immediately preceding tablet for the table, or null if + * this represents the first tablet in this table + * @return Return the new {@link TabletId} object + */ + static TabletId of(TableId tableId, Text endRow, Text prevEndRow) { + KeyExtent ke = new KeyExtent(tableId, endRow, prevEndRow); + return new TabletIdImpl(ke); + } + + /** + * Return a TabletId object for the provided TableId in range of endRow and prevEndRow. Generate + * ke using {@link KeyExtent#KeyExtent(TableId, Text, Text)}, to then return + * {@link TabletIdImpl#TabletIdImpl(KeyExtent)}. If the parameters endRow or prevEndRow are null, + * they represent a tablet over the range of -inf and/or inf respectively. Does null checks to + * avoid getting an ambiguous Text reference error when null is passed through + * {@link Text#Text()}. + * + * @param tableId the ID for a table + * @param endRow the last row in this tablet, or null if this is the last tablet in this table + * @param prevEndRow he last row in the immediately preceding tablet for the table, or null if + * this represents the first tablet in this table + * @return Return the new {@link TabletId} object + */ + static TabletId of(TableId tableId, String endRow, String prevEndRow) { + KeyExtent ke; + if (endRow == null && prevEndRow == null) { + ke = new KeyExtent(tableId, null, null); + } else if (endRow == null) { + ke = new KeyExtent(tableId, null, new Text(prevEndRow)); + } else if (prevEndRow == null) { + ke = new KeyExtent(tableId, new Text(endRow), null); + } else { + ke = new KeyExtent(tableId, new Text(endRow), new Text(prevEndRow)); + } + + return new TabletIdImpl(ke); + } + + /** + * Return a TabletId object for the provided TableId in range of endRow and prevEndRow. Generate + * ke using {@link KeyExtent#KeyExtent(TableId, Text, Text)}, to then return + * {@link TabletIdImpl#TabletIdImpl(KeyExtent)}. If the parameters endRow or prevEndRow are null, + * they represent a tablet over the range of -inf and/or inf respectively. Does null checks to + * avoid getting an ambiguous Text reference error when null is passed through + * {@link Text#Text()}. + * + * @param tableId the ID for a table + * @param endRow the last row in this tablet, or null if this is the last tablet in this table + * @param prevEndRow he last row in the immediately preceding tablet for the table, or null if + * this represents the first tablet in this table + * @return Return the new {@link TabletId} object + */ Review Comment: need `@since 2.1.4` tags on all of the javadoc of the new methods. -- 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: notifications-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
