Skip to content

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

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

Merged
virajjasani merged 3 commits into from
Apr 5, 2024
Merged

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

Changes from 2 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
19 changes: 17 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,23 @@ 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.

@Apache9 Apache9 Mar 25, 2024

Copy link
Copy Markdown
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...

@virajjasani virajjasani Mar 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

@virajjasani virajjasani Mar 25, 2024

Copy link
Copy Markdown
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.

@virajjasani virajjasani Mar 25, 2024

Copy link
Copy Markdown
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? 🙂

@Apache9 Apache9 Mar 25, 2024

Copy link
Copy Markdown
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.

@virajjasani virajjasani Mar 25, 2024

Copy link
Copy Markdown
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 ignored, could not record the server: " + sn
+ " . Consider yourself dead as server with higher startcode is already registered.";

@bbeaudreault bbeaudreault Mar 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't need to block merging, but given the comments above, doesn't the message here seem odd? The message seems to be addressing the regionserver. But you say in the comment that this error is not sent to the regionserver and is only meant to signal upper layers to skip processing the dead server.

I wonder if we shouldn't have a new exception type or at least clarify the message (which is not just thrush but also logged)

@virajjasani virajjasani Mar 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The exception is sent to regionserver but the purpose is bit different. So not only we don't want to process the report but also we want to let the regionserver know that it should have been dead.
Let me see how I can make this clear in the comments.

@Apache9 Apache9 Mar 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can not send this message to regionserver as it is already dead... I have repeated these words many times, on the jira issue and also here...

@virajjasani virajjasani Mar 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I meant to say that master sends it as if regionserver is still alive. But in reality it is dead by the time master receives the RPC call.

@bbeaudreault bbeaudreault Mar 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the comment is clear. To me the log message needs to change

@apurtell apurtell Mar 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about

final String errorMsg = "RegionServerReport received from " + sn
  + ", but another server with the same name and higher startcode is already registered, ignoring";

@bbeaudreault bbeaudreault Mar 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yea, that sounds great

@virajjasani virajjasani Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure let me make changes today.

LOG.warn(errorMsg);
throw new YouAreDeadException(errorMsg);
}
}
updateLastFlushedSequenceIds(sn, sl);
Expand Down