HBASE-28187 NPE when flushing a non-existing column family #5692
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| "There are non-existing families %s, we cannot flush the region %s, in table %s.", | ||
| noSuchFamilies, getRegionInfo().getRegionNameAsString(), | ||
| getTableDescriptor().getTableName().getNameAsString()); | ||
| LOG.warn(noSuchFamiliesMsg); |
There was a problem hiding this 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.
There was a problem hiding this comment.
I also think the log here is unnecessary too. If you want to log this to trace bad requests, RegionServer will log some key information for FlushRegionRequest and ExecuteProceduresRequest. Is it possible to merge the information here into that log?
| } | ||
| }); | ||
| TableName tableName = location.getRegion().getTable(); | ||
| addListener(getDescriptor(tableName), (tDesc, error2) -> { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Agree. I think an NoSuchColumnFamilyException here would be better too.
| CompletableFuture<Void> future = new CompletableFuture<>(); | ||
| addListener(procFuture, (ret, error) -> { | ||
| addListener(getDescriptor(tableName), (tDesc, error) -> { |
There was a problem hiding this 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.
There was a problem hiding this comment.
I will refactor this part of the code later based on your good suggestions
Thank you both for your review
guluo2016
force-pushed
the
hbase_HBASE-28187
branch
2 times, most recently
from
58b7960
to
77e0934
Compare
guluo2016
force-pushed
the
hbase_HBASE-28187
branch
from
77e0934
to
8909436
Compare
This comment has been minimized.
This comment has been minimized.
|
I hava already refactor these code, please review, thanks. @ndimiduk @frostruan @Apache9 Summary: flush 'table', 'non_existing_family'Flush Procedure is enabled.we would check if the column families exist when executing FlushTableProcedure, and throwing NoSuchColumnFamilyException if it doesn't exist. And we will not retry to flush again at client side. Flush Procedure is disabled.we would check if the column families exist on MasterFlushTableProcedureManager.execProcedure(), and throwing NoSuchColumnFamilyException if it doesn't exist. flush 'region_name', 'non_existing_family'we would check if the column families exist on RSRpcServices.flushRegion(), and throwing NoSuchColumnFamilyException if it doesn't exist. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
LGTM. +1 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Hui Ruan <huiruan@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Hui Ruan <huiruan@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Hui Ruan <huiruan@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Hui Ruan <huiruan@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Hui Ruan <huiruan@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Hui Ruan <huiruan@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Hui Ruan <huiruan@apache.org> Co-authored-by: Peng Lu <lupeng_nwpu@qq.com>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Hui Ruan <huiruan@apache.org> Co-authored-by: Peng Lu <lupeng_nwpu@qq.com>
Details see: HBASE-28187
And even worse, although this flush can be terminated by exception or manually，we are still unable to operate this region because of RegionTooBusyException.