Skip to content

HBASE-24186: RegionMover ignores replicationId #1512

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 1 commit into from
Apr 16, 2020
Merged

HBASE-24186: RegionMover ignores replicationId #1512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionMover.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,8 @@ private ServerName getServerNameForRegion(RegionInfo region) throws IOException
return null;
}
HRegionLocation loc =
conn.getRegionLocator(region.getTable()).getRegionLocation(region.getStartKey(), true);
conn.getRegionLocator(region.getTable()).getRegionLocation(region.getStartKey(),
Copy link
Contributor

@HorizonNet HorizonNet Apr 14, 2020

Choose a reason for hiding this comment

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

Is there already a test, which covers the old as well as the new behavior?

Copy link
Contributor

@virajjasani virajjasani Apr 15, 2020

Choose a reason for hiding this comment

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

Existing test cases are good. But yes good to add additional test case specific for replicaId. @BukrosSzabolcs can you add a test case in TestRegionMover ?

Copy link
Contributor Author

@BukrosSzabolcs BukrosSzabolcs Apr 15, 2020

Choose a reason for hiding this comment

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

The issue was found by a test. IntegrationTestTimeBoundedRequestsWithRegionReplicas failed while running with chaos monkey using GracefulRollingRestartRsAction. So there is coverage just not in TestRegionMover. Would that be sufficient?

Copy link
Contributor

@virajjasani virajjasani Apr 15, 2020

Choose a reason for hiding this comment

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

It's great that IT test found this issue but they are not run as part of QA build. Hence, if we can have one unit test, that would be really great. QA will always run it. Sounds good @BukrosSzabolcs ?

Copy link
Contributor

@virajjasani virajjasani Apr 15, 2020

Choose a reason for hiding this comment

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

However, since we have an IT that covers this, ideally it should be fine and maybe you can take up unit test as separate child Jira of this Jira.
Should be good @HorizonNet ?

Copy link
Contributor Author

@BukrosSzabolcs BukrosSzabolcs Apr 15, 2020

Choose a reason for hiding this comment

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

I realized my original description of the issue is not correct. This does not prevent the moving of the region, the result of getServerNameForRegion() is only used in the validation of the move, so it only forces the move to try to repeat itself because it does not realize the move already happened. So it just slows down the process but not break it.

I tried to write a test for this, but the only thing I could come up with is a test that goes trough regions on a rs and checks if getServerNameForRegion founds the correct server for it. I can add it if you insist but I see very little added value.

Copy link
Contributor

@HorizonNet HorizonNet Apr 16, 2020

Choose a reason for hiding this comment

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

Then let it be.

region.getReplicaId(),true);
if (loc != null) {
return loc.getServerName();
} else {
Expand Down