Apache9 commented on code in PR #4644:
URL: https://github.com/apache/hbase/pull/4644#discussion_r928114525
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java:
##########
@@ -187,7 +188,7 @@ default TableName[] listTableNames(Pattern pattern) throws
IOException {
* Get a table descriptor.
* @param tableName as a {@link TableName}
* @return the tableDescriptor
- * @throws org.apache.hadoop.hbase.TableNotFoundException
+ * @throws org.apache.hadoop.hbase.TableNotFoundException if the table was
not found
Review Comment:
Since we have already imported TableNotFoundException, let's just declare it
as TableNotFoundException so the text will be shorter.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/security/SaslUtil.java:
##########
@@ -101,10 +104,9 @@ public static Map<String, String>
initSaslProperties(String rpcProtection) {
if (rpcProtection.isEmpty()) {
saslQop = QualityOfProtection.AUTHENTICATION.getSaslQop();
} else {
- String[] qops = rpcProtection.split(",");
StringBuilder saslQopBuilder = new StringBuilder();
- for (int i = 0; i < qops.length; ++i) {
- QualityOfProtection qop = getQop(qops[i]);
+ for (String s : Splitter.on(',').splitToList(rpcProtection)) {
Review Comment:
Just call split is enough? We do not need a List here.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/CompactType.java:
##########
@@ -26,9 +26,7 @@
@InterfaceAudience.Public
public enum CompactType {
- NORMAL(0),
- MOB(1);
+ NORMAL,
Review Comment:
This is IA.Public so we should be careful. For me I haven't seen any
problems as the constructor is package private and the 'type' is not exposed,
so we are safe to remove it. Others could also think about whether there are
problems.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/coprocessor/ColumnInterpreter.java:
##########
@@ -40,11 +40,6 @@
* {@link ColumnInterpreter#castToReturnType(Object)} which takes a <T>
type and returns a
* <S> type. The AggregateIm>lementation uses PB messages to
initialize the user's
* ColumnInterpreter implementation, and for sending the responses back to
AggregationClient.
- * @param T Cell value data type
Review Comment:
I think these lines are still useful, let's keep them without using the
`@param` tag?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java:
##########
@@ -1183,19 +1191,21 @@ default CompletableFuture<Integer> getMasterInfoPort() {
CompletableFuture<Void> rollWALWriter(ServerName serverName);
/**
- * Clear compacting queues on a region server. n * @param queues the set of
queue name
+ * Clear compacting queues on a region server.
+ * @param serverName The servername of the region server.
+ * @param queues the set of queue name
*/
CompletableFuture<Void> clearCompactionQueues(ServerName serverName,
Set<String> queues);
/**
- * Get a list of {@link RegionMetrics} of all regions hosted on a region
seerver. n * @return a
- * list of {@link RegionMetrics} wrapped by {@link CompletableFuture}
+ * Get a list of {@link RegionMetrics} of all regions hosted on a region
server.
Review Comment:
Good.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Append.java:
##########
@@ -49,7 +49,7 @@
@InterfaceAudience.Public
public class Append extends Mutation {
private static final Logger LOG = LoggerFactory.getLogger(Append.class);
- private static final long HEAP_OVERHEAD = ClassSize.REFERENCE +
ClassSize.TIMERANGE;
+ private static final long HEAP_OVERHEAD = (long) (ClassSize.REFERENCE +
ClassSize.TIMERANGE);
Review Comment:
Will this fix the warning? I suppose we should cast one of the two elements
to long first.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java:
##########
@@ -1338,7 +1345,8 @@ Future<Boolean> abortProcedureAsync(long procId, boolean
mayInterruptIfRunning)
* @param serverName The servername of the regionserver.
* @throws IOException
if a remote or network
*
exception occurs
- * @throws org.apache.hadoop.hbase.regionserver.wal.FailedLogCloseException
+ * @throws org.apache.hadoop.hbase.regionserver.wal.FailedLogCloseException
if we failed to close
Review Comment:
Ditto.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/IsolationLevel.java:
##########
@@ -29,11 +29,8 @@
@InterfaceAudience.Public
public enum IsolationLevel {
- READ_COMMITTED(1),
- READ_UNCOMMITTED(2);
-
- IsolationLevel(int value) {
- }
+ READ_COMMITTED,
Review Comment:
Ah, we have a bunch of these defines...
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/PrefixFilter.java:
##########
@@ -110,9 +110,10 @@ public byte[] toByteArray() {
}
/**
+ * Parse a serialized representation of {@link PrefixFilter}
* @param pbBytes A pb serialized {@link PrefixFilter} instance
* @return An instance of {@link PrefixFilter} made from <code>bytes</code>
- * @throws org.apache.hadoop.hbase.exceptions.DeserializationException
+ * @throws org.apache.hadoop.hbase.exceptions.DeserializationException if an
error occurred
Review Comment:
Just use the simple class name is enough.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java:
##########
@@ -1635,42 +1641,36 @@ default CompletableFuture<List<Boolean>>
hasUserPermissions(List<Permission> per
/**
* Creates a new RegionServer group with the given name
* @param groupName the name of the group
- * @throws IOException if a remote or network exception occurs
Review Comment:
IIRC, the intention here is to let the users know the exception type
returned by CompletableFuture. Anyway, not a big deal, since at compile time
user will always get a Throwable from a CompletableFuture.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/QualifierFilter.java:
##########
@@ -76,9 +76,10 @@ public byte[] toByteArray() {
}
/**
+ * Parse a serialized representation of {@link QualifierFilter}
* @param pbBytes A pb serialized {@link QualifierFilter} instance
* @return An instance of {@link QualifierFilter} made from
<code>bytes</code>
- * @throws org.apache.hadoop.hbase.exceptions.DeserializationException
+ * @throws org.apache.hadoop.hbase.exceptions.DeserializationException if an
error occurred
Review Comment:
Ditto.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ParseFilter.java:
##########
@@ -109,6 +109,7 @@ public Filter parseFilterString(String filterString) throws
CharacterCodingExcep
* @param filterStringAsByteArray filter string given by the user
* @return filter object we constructed
*/
+ @SuppressWarnings("JdkObsolete")
Review Comment:
Removing the usage of Stack needs extra works as the API is different, but I
think we should change it to something like ArrayDeque. So let's not add this
SuppressWarnings annotation, just leave it as is for now and open another issue
for fixing this, something like 'Remove the usage of java.util.Stack in our
code base'
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/security/provider/AbstractSaslClientAuthenticationProvider.java:
##########
@@ -48,6 +48,7 @@ public final int hashCode() {
}
@Override
+ @SuppressWarnings("EqualsUsingHashCode")
Review Comment:
I suggest we do not hide this warning, the implementation seems awkward,
maybe it just means comparing the AuthMethod?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java:
##########
@@ -1559,11 +1554,13 @@ public CompletableFuture<Void> assign(byte[]
regionName) {
future.completeExceptionally(err);
return;
}
- addListener(this.<Void> newMasterCaller().priority(regionInfo.getTable())
- .action(((controller, stub) -> this.<AssignRegionRequest,
AssignRegionResponse, Void> call(
- controller, stub,
RequestConverter.buildAssignRegionRequest(regionInfo.getRegionName()),
- (s, c, req, done) -> s.assignRegion(c, req, done), resp -> null)))
- .call(), (ret, err2) -> {
+ addListener(
+ this.<Void> newMasterCaller().priority(regionInfo.getTable())
+ .action((controller, stub) -> this.<AssignRegionRequest,
AssignRegionResponse, Void> call(
Review Comment:
OK, useless parentheses
##########
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestSimpleRequestController.java:
##########
@@ -277,6 +279,7 @@ public void testSubmittedSizeChecker() {
}
@Test
+ @SuppressWarnings("ArrayAsKeyOfSetOrMap")
Review Comment:
Ditto.
##########
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestSimpleRequestController.java:
##########
@@ -358,10 +361,8 @@ public void testWaitForMaximumCurrentTasks() throws
Exception {
try {
barrier.await();
controller.waitForMaximumCurrentTasks(max.get(), 123, 1, null);
- } catch (InterruptedIOException e) {
+ } catch (InterruptedIOException | InterruptedException |
BrokenBarrierException e) {
Review Comment:
Just throw the exception out? The fail message will contain the message of
the exception.
##########
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java:
##########
@@ -192,8 +187,7 @@ public void testSetStartRowAndSetStopRow() {
scan.withStartRow(new byte[HConstants.MAX_ROW_LENGTH + 1]);
fail("should've thrown exception");
} catch (IllegalArgumentException iae) {
- } catch (Exception e) {
- fail("expected IllegalArgumentException to be thrown");
+ // Expected
Review Comment:
I think we could file another issue to change these tests to use
assertThrows. Anyway, the current modifications make the code better, can open
a follow on issue for changing to assertThrows.
##########
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestSimpleRequestController.java:
##########
@@ -115,6 +116,7 @@ public long heapSize() {
}
@Test
+ @SuppressWarnings("ArrayAsKeyOfSetOrMap")
Review Comment:
Since this is a test, let's just use TreeMap with Bytes.BYTES_COMPARATOR
instead of HashMap to remove the warning?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/WhileMatchFilter.java:
##########
@@ -103,9 +103,10 @@ public byte[] toByteArray() throws IOException {
}
/**
+ * Parse a serialized representation of {@link WhileMatchFilter}
* @param pbBytes A pb serialized {@link WhileMatchFilter} instance
* @return An instance of {@link WhileMatchFilter} made from
<code>bytes</code>
- * @throws org.apache.hadoop.hbase.exceptions.DeserializationException
+ * @throws org.apache.hadoop.hbase.exceptions.DeserializationException if an
error occurred
Review Comment:
Ditto.
--
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]
