Uh oh!
There was an error while loading. Please reload this page.
HBASE-27203 Clean up error-prone findings in hbase-client [branch-2] - #4651
Conversation
apurtell
left a comment
There was a problem hiding this comment.
Notable findings exclusive to branch-2.
| handleException(); | ||
| if (this.closed) { | ||
| return null; | ||
| try { |
There was a problem hiding this comment.
We were missing an inner try here.
| @Override | ||
| public void close() { | ||
| lock.lock(); |
There was a problem hiding this comment.
Not a bug like above but still a nit.
| * Global nonceGenerator shared per client.Currently there's no reason to limit its scope. Once | ||
| * it's set under nonceGeneratorCreateLock, it is never unset or changed. | ||
| */ | ||
| // XXX: It is a bad pattern to assign a value to a static field from a constructor. However |
There was a problem hiding this comment.
Declined to fix the issue because we'd have to use a Configuration instance at static class init time, not the Configuration that gets passed into the Constructor. However, marked it as a code smell with XXX.
| public void mergeRegions(final byte[] nameOfRegionA, final byte[] nameOfRegionB, | ||
| final boolean forcible) throws IOException { | ||
| mergeRegionsAsync(nameOfRegionA, nameOfRegionB, forcible); | ||
| try { |
There was a problem hiding this comment.
mergeRegions will return immediately without waiting for or surfacing any exception from the operation. This seems wrong. We don't expect to wait for the master to complete the merge but we would expect to surface an IOException from the RPC. Yet, it is a semantic change over previously released versions. For discussion.
There was a problem hiding this comment.
IIRC this is a known issue. You can see the comments in the Admin interface.
/**
* Merge two regions. Asynchronous operation.
* @param nameOfRegionA encoded or full name of region a
* @param nameOfRegionB encoded or full name of region b
* @param forcible <code>true</code> if do a compulsory merge, otherwise we will only merge
* two adjacent regions
* @throws IOException if a remote or network exception occurs
* @deprecated Since 2.0. Will be removed in 3.0. Use
* {@link #mergeRegionsAsync(byte[], byte[], boolean)} instead.
*/
@Deprecated
void mergeRegions(byte[] nameOfRegionA, byte[] nameOfRegionB, boolean forcible)
throws IOException;
There was a problem hiding this comment.
I'm not sure an IOException will get thrown as expected because we don't get() the Future. On the other hand this has been the behavior for a long time. I will stick to warning cleanups and come back to this later.
| // call out to master to do split now | ||
| splitRegionAsync(r, splitPoint); | ||
| try { | ||
| splitRegionAsync(r, splitPoint).get(); |
There was a problem hiding this comment.
split will return immediately without waiting for or surfacing any exception from the operation. This seems wrong. We don't expect to wait for the split to complete but we would expect to surface an IOException from the RPC. Yet, it is a semantic change over previously released versions. For discussion.
There was a problem hiding this comment.
Should be the same with mergeRegions method.
There was a problem hiding this comment.
Same, will come back to this later.
| return this.maxLatency.getAndSet(0); | ||
| } | ||
| @SuppressWarnings("FutureReturnValueIgnored") |
| private long fastFailClearingTimeMilliSec; | ||
| private final ThreadLocal<MutableBoolean> threadRetryingInFastFailMode = new ThreadLocal<>(); | ||
| private static final ThreadLocal<MutableBoolean> threadRetryingInFastFailMode = |
There was a problem hiding this comment.
ThreadLocals should to be class fields not instance fields. Otherwise the coder may get the false impression interactions with the thread local is also scoped to the instance. They are not, they are scoped to the thread.
| /** | ||
| * Set the tracker that should be used for tracking statistics about the server | ||
| * @deprecated Does nothing. Since 2.5.0, will be removed in 4.0.0. |
There was a problem hiding this comment.
Remove this in 3.0.0 instead?
There was a problem hiding this comment.
This class is IA.Private so we are free to remove it at any time.
| throw new IOException("Imposible? Arrive at an unreachable line..."); | ||
| } | ||
| @SuppressWarnings("FutureReturnValueIgnored") |
There was a problem hiding this comment.
Suppressed here because scheduling the callable returns a future we don't care about.
| private boolean reloginInProgress; | ||
| @SuppressWarnings("FutureReturnValueIgnored") |
There was a problem hiding this comment.
Suppressed here because the scheduling returns a future we don't care about.
Apache-HBase
commented
Jul 27, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 27, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 27, 2022
🎊 +1 overall
This message was automatically generated. |
Rolled back the HBaseAdmin changes called out above, just cleanups now, proceeding to merge. I will file a follow up for that, with a more considered set of changes. |
…4651) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
No description provided.