ndimiduk commented on code in PR #5692:
URL: https://github.com/apache/hbase/pull/5692#discussion_r1765175228
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java:
##########
@@ -1071,14 +1096,29 @@ CompletableFuture<FlushRegionResponse>
flushRegionInternal(byte[] regionName, by
.completeExceptionally(new
NoServerForRegionException(Bytes.toStringBinary(regionName)));
return;
}
- addListener(flush(serverName, location.getRegion(), columnFamily,
writeFlushWALMarker),
- (ret, err2) -> {
- if (err2 != null) {
- future.completeExceptionally(err2);
- } else {
- future.complete(ret);
- }
- });
+ TableName tableName = location.getRegion().getTable();
+ addListener(getDescriptor(tableName), (tDesc, error2) -> {
Review Comment:
See above about introducing the additional RPC.
IMHO, the client should be dumber, not smarter. I'd rather see the
`NoSuchColumnFamilyException` come from the RS than the client.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java:
##########
@@ -975,29 +978,51 @@ public CompletableFuture<Void> flush(TableName tableName,
List<byte[]> columnFam
// If the server version is lower than the client version, it's possible
that the
// flushTable method is not present in the server side, if so, we need to
fall back
// to the old implementation.
+ Preconditions.checkNotNull(columnFamilyList,
+ "columnFamily is null, If you don't specify a columnFamily, use
flush(TableName) instead.");
List<byte[]> columnFamilies = columnFamilyList.stream()
.filter(cf -> cf != null && cf.length >
0).distinct().collect(Collectors.toList());
- FlushTableRequest request =
RequestConverter.buildFlushTableRequest(tableName, columnFamilies,
- ng.getNonceGroup(), ng.newNonce());
- CompletableFuture<Void> procFuture = this.<FlushTableRequest,
FlushTableResponse> procedureCall(
- tableName, request, (s, c, req, done) -> s.flushTable(c, req, done),
- (resp) -> resp.getProcId(), new
FlushTableProcedureBiConsumer(tableName));
CompletableFuture<Void> future = new CompletableFuture<>();
- addListener(procFuture, (ret, error) -> {
+ addListener(getDescriptor(tableName), (tDesc, error) -> {
Review Comment:
Introducing `getDescriptors` introduces an additional RPC. I think we
shouldn't do this, only to do validation on client-side. Instead please post
the original message and let the server validate it. If we had some client-side
cache of the descriptor, we could do a pre-validation on that, but even then,
caches are lossy and probably shouldn't be relied upon for something that the
server is already the authority.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -2522,6 +2522,20 @@ public FlushResultImpl flushcache(List<byte[]> families,
boolean writeFlushReque
LOG.debug(msg);
return new FlushResultImpl(FlushResult.Result.CANNOT_FLUSH, msg, false);
}
+ // cannot flush non-existing column families, and fail-fast
+ if (families != null) {
+ List<String> noSuchFamilies =
+ families.stream().filter(cf ->
!getTableDescriptor().hasColumnFamily(cf))
+ .map(cf -> Bytes.toString(cf)).collect(Collectors.toList());
+ if (noSuchFamilies.size() > 0) {
+ String noSuchFamiliesMsg = String.format(
+ "There are non-existing families %s, we cannot flush the region %s,
in table %s.",
+ noSuchFamilies, getRegionInfo().getRegionNameAsString(),
+ getTableDescriptor().getTableName().getNameAsString());
+ LOG.warn(noSuchFamiliesMsg);
Review Comment:
I don't think this should log at all. It's essentially an HTTP 400 kind of
situation. Maybe a TRACE log, if at all. By all means, send the message back to
the client, but i don't think the RS needs to emit this message.
--
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]
