kevinrr888 commented on code in PR #5678:
URL: https://github.com/apache/accumulo/pull/5678#discussion_r2167292855
##########
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:
Instead of 3 of the same javadocs, might be clearer and more maintainable to
just have javadocs for one method and use the see tag for the others
##########
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:
```suggestion
* they represent a tablet over the range of -inf and/or inf respectively.
```
No need to include implementation details
##########
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.
Review Comment:
```suggestion
* Return a TabletId object for the provided TableId in range of endRow
and prevEndRow. If the parameters endRow or prevEndRow are null,
* they represent a tablet over the range of -inf and inf respectively.
```
No need to include implementation details. Also, `TabletIdImpl` is an
internal class and this is public API so we don't want that in the javadoc.
##########
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");
Review Comment:
The messages for these assertions isn't needed (fine to leave in though).
When the assertion fails it will show the expected and the actual in the
error message
##########
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()));
Review Comment:
Should use UTF-8 when converting strings to bytes or vice-versa
See https://github.com/apache/accumulo/issues/4765 for more info
--
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
