Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-28366 Mis-order of SCP and regionServerReport results into region inconsistencies #5774

Merged
merged 3 commits into from
Apr 5, 2024
Merged

HBASE-28366 Mis-order of SCP and regionServerReport results into region inconsistencies #5774

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae7d9ab
HBASE-28366 Mis-order of SCP and regionServerReport results into regi…
virajjasani Mar 24, 2024
fb2eaab
update comments
virajjasani Mar 25, 2024
1ff6f5b
msg update
virajjasani Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,24 @@ public void regionServerReport(ServerName sn, ServerMetrics sl) throws YouAreDea
// the ServerName to use. Here we presume a master has already done
// that so we'll press on with whatever it gave us for ServerName.
if (!checkAndRecordNewServer(sn, sl)) {
LOG.info("RegionServerReport ignored, could not record the server: " + sn);
return; // Not recorded, so no need to move on
// Master already registered server with same (host + port) and higher startcode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still do not think this is necessary, because if the new server with the same host and port has already registered to master, how can we return this YouAreDeadException to the old server? Even if there is a race condition, when sending we will receive a connection reset because the old server is already dead...

Copy link
Contributor Author

@virajjasani virajjasani Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it happened (as per logs mentioned on the jira), master processed the report and that generated inconsistencies.

We have seen this happen many times in the past when regionserver is not really aborted but looses connection with Zookeeper, triggering SCP by master. And regionserver with new startcode is not only alive but has also reported regionservers to master. After that, somehow master still receives regionserver report from old startcode regionserver, master processes it and that results into inconsistencies. I know this is rare case but it definitely happened more than once in more than one prod clusters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of safety check, it will prevent inconsistencies. I agree that anyone looking at this would think, why do we need such extra safety, it's valid point but I can guarantee that not having such strict validation has caused inconsistencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other way to think about this is: why should we even receive any report from old server and not throw YouAreDeadException while we already know that new server is alive and is already registered? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the code, now I understand why we need to throw an exception here. Your comment totally missed the most important part...

At least your comment should include these two points:

  1. The exception thrown here is not meant to tell the region server it is dead because if there is a new server on the same host port, the old server should have already been dead.
  2. The exception thrown here is to skip the later steps of the whole regionServerReport request processing. Usually, after recording it in ServerManager, we will call the related methods in AssignmentManager to record region states. If the region server is already dead, we should not do these steps any more, so here we throw an exception to let the upper layer know that they should not continue processing any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, will add more comments to make it clear. Thanks Duo!

// This can happen if regionserver report comes late from old server
// (possible race condition), by that time master has already processed SCP for that
// server and started accepting regionserver report from new server i.e. server with
// same (host + port) and higher startcode.
// The exception thrown here is not meant to tell the region server it is dead because if
// there is a new server on the same host port, the old server should have already been
// dead in ideal situation.
// The exception thrown here is to skip the later steps of the whole regionServerReport
// request processing. Usually, after recording it in ServerManager, we will call the
// related methods in AssignmentManager to record region states. If the region server
// is already dead, we should not do these steps anymore, so here we throw an exception
// to let the upper layer know that they should not continue processing anymore.
final String errorMsg = "RegionServerReport received from " + sn
+ ", but another server with the same name and higher startcode is already registered,"
+ " ignoring";
LOG.warn(errorMsg);
throw new YouAreDeadException(errorMsg);
}
}
updateLastFlushedSequenceIds(sn, sl);
Expand Down