-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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-24491 Remove HRegionInfo #1830
Conversation
Could pass compile. Let's see the UTs. And I guess we also need to deal with hbase-shell. Will take a look soon. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skimmed. So far, it looks really good.
@@ -2345,14 +2345,6 @@ public static TableName toTableName(HBaseProtos.TableName tableNamePB) { | |||
.setQualifier(UnsafeByteOperations.unsafeWrap(tableName.getQualifier())).build(); | |||
} | |||
|
|||
public static HBaseProtos.RegionInfo toProtoRegionInfo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used anymore? This seems not releated to HRegionInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have two toRegionInfo methods for converting from RegionInfo to proto and vice versa. This method is not used and the implementation is wrong, so just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Great cleanup.
Seems the ut was not trigger in previous HBase QA? |
There are failurs for small tests so the medium and large tests are skipped. Let me take a look. |
The method too long checkstyle warning is not introduced by the PR here. Can open a follow on issue to remove HBaseTestCase and rewrite TestHStoreFile. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
The failed UT is caused by a serious problem of our RegionInfo related API. Let me open a new issue for fixing it. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. Left two minor comments.
@@ -350,31 +353,20 @@ public void testSplitParentFirstComparator() { | |||
SplitParentFirstComparator comp = new SplitParentFirstComparator(); | |||
TableDescriptor td = createTableDescriptorForCurrentMethod(); | |||
|
|||
/* Region splits: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep the visual representation.
@@ -766,15 +766,14 @@ private int getScannedCount(RegionScanner scanner) throws IOException { | |||
|
|||
/** | |||
* Create an HRegion with the result of a WAL split and test we only see the | |||
* good edits | |||
* @throws Exception | |||
* good edits= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be "." instead of a "=".
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No description provided.