saintstack commented on a change in pull request #1305: HBASE-23984 [Flakey
Tests] TestMasterAbortAndRSGotKilled fails in tea…
URL: https://github.com/apache/hbase/pull/1305#discussion_r394707055
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -3185,71 +3195,61 @@ private void closeRegionIgnoreErrors(RegionInfo
region, final boolean abort) {
* If a close was in progress, this new request will be ignored, and an
exception thrown.
* </p>
*
- * @param encodedName Region to close
* @param abort True if we are aborting
+ * @param destination Where the Region is being moved too... maybe null if
unknown.
* @return True if closed a region.
* @throws NotServingRegionException if the region is not online
*/
- protected boolean closeRegion(String encodedName, final boolean abort, final
ServerName sn)
- throws NotServingRegionException {
- //Check for permissions to close.
- HRegion actualRegion = this.getRegion(encodedName);
- // Can be null if we're calling close on a region that's not online
- if ((actualRegion != null) && (actualRegion.getCoprocessorHost() != null))
{
- try {
- actualRegion.getCoprocessorHost().preClose(false);
- } catch (IOException exp) {
- LOG.warn("Unable to close region: the coprocessor launched an error ",
exp);
- return false;
- }
- }
-
- // previous can come back 'null' if not in map.
- final Boolean previous =
this.regionsInTransitionInRS.putIfAbsent(Bytes.toBytes(encodedName),
- Boolean.FALSE);
-
+ protected boolean closeRegion(String encodedRegionName, final boolean abort,
+ @Nullable final ServerName destination) throws NotServingRegionException
{
+ // TODO: Check for perms to close.
+ // Ideally we'd shove a bunch of the below logic into CloseRegionHandler
only we have to do
+ // some handling inline w/ the request such as throwing
NotServervingRegionException if we
+ // are not hosting so client gets the message directly; can't do this from
inside an
+ // executor service.
+ Region region = getRegion(encodedRegionName);
+ if (region == null) {
+ // Changed behavior. We'd proceed though Region not online.
+ // The master deletes the znode when it receives this exception.
+ throw new NotServingRegionException(encodedRegionName);
+ }
+ byte [] encodedRegionNameAsBytes = Bytes.toBytes(encodedRegionName);
+ // This getting from this.regionsInTransitionInRS and checking the return
is done in a few
+ // places, mostly over in Handlers; this.regionsInTransitionInRS is how we
ensure we don't
+ // clash opens/closes or duplicate closes when one is ongoing. FYI.
+ final Boolean previous = this.regionsInTransitionInRS.
+ putIfAbsent(encodedRegionNameAsBytes, Boolean.FALSE);
if (Boolean.TRUE.equals(previous)) {
- LOG.info("Received CLOSE for the region:" + encodedName + " , which we
are already " +
- "trying to OPEN. Cancelling OPENING.");
- if (!regionsInTransitionInRS.replace(Bytes.toBytes(encodedName),
previous, Boolean.FALSE)) {
+ LOG.info("Received CLOSE for {} which is OPENING. Cancelling OPENING.",
encodedRegionName);
+ if (!this.regionsInTransitionInRS.replace(encodedRegionNameAsBytes,
previous,
+ Boolean.FALSE)) {
// The replace failed. That should be an exceptional case, but
theoretically it can happen.
Review comment:
It seems to be a vestige of an old behavior. Work needs to be done to figure
if it can be safely removed. Let me make it more explicit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
