-
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-23682 Fix NPE when disable DeadServerMetricRegionChore #1026
Conversation
🎊 +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. Can you also add test cases to ensure we don't run into this one again (at least for the AssignmentManager
)?
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.
Stumbled upon this change. It looks like Nullable annotations are not a standard but curious whats the community's take on it.
hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Outdated
Show resolved
Hide resolved
Done. |
🎊 +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.
Thanks for adding the annotations. Few comments in the added test. Mind cleaning it up?
...src/test/java/org/apache/hadoop/hbase/master/assignment/TestDeadServerMetricRegionChore.java
Show resolved
Hide resolved
/** | ||
* Base class for AM test. | ||
*/ | ||
@Category({ MasterTests.class, LargeTests.class }) |
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.
This would be MediumTest, no?
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.
OK
protected HBaseTestingUtility util; | ||
|
||
protected void setupConfiguration(Configuration conf) throws Exception { | ||
FSUtils.setRootDir(conf, util.getDataTestDir()); |
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.
Again looks like a remnant of copy+paste from TestAssignment...
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.
fix it
this.util.createTable(tableName, "f"); | ||
this.util.waitTableAvailable(tableName); | ||
} finally { | ||
this.util.killMiniHBaseCluster(); |
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.
Move to tearDown()?
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 have a shutdown, instead of kill the cluster, in tearDown
. Also seems that an assertion in this test is missing.
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.
The test is failed when startMiniCluster without this patch.
[INFO] Running org.apache.hadoop.hbase.master.assignment.TestDeadServerMetricRegionChore
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 210.246 s <<< FAILURE! - in org.apache.hadoop.hbase.master.assignment.TestDeadServerMetricRegionChore
[ERROR] org.apache.hadoop.hbase.master.assignment.TestDeadServerMetricRegionChore.testDeadServerMetricRegionChore Time elapsed: 210.054 s <<< ERROR!
java.io.IOException: Shutting down
at org.apache.hadoop.hbase.master.assignment.TestDeadServerMetricRegionChore.testDeadServerMetricRegionChore(TestDeadServerMetricRegionChore.java:60)
Caused by: java.lang.RuntimeException: Master not initialized after 200000ms
at org.apache.hadoop.hbase.master.assignment.TestDeadServerMetricRegionChore.testDeadServerMetricRegionChore(TestDeadServerMetricRegionChore.java:60)
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.
Is the shutdown failing or because it is part of the tearDown
?
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.
The shutdown has no problem, i already change killMiniHBaseCluster
to shutdownMiniCluster.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/apache/hadoop/hbase/master/assignment/TestDeadServerMetricRegionChore.java
Show resolved
Hide resolved
@@ -0,0 +1,68 @@ | |||
/** |
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.
nit: /*
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.
You mean change "/**" to "/*" ? I see other tests all use "/**" no one use "/*"
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.
/** is interpreted as a javadoc in IDEs. If you notice others' changes, people are switching to /* incrementally in their patches (files they are touching). Now that you are adding a new class, it is better to start with /*.
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.
OK, thanks for you explanation.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Rerunning build. @bharathv , you approving this change sir? |
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.
@saintstack There were a couple of comments that weren't addressed since the last review. +1 once they are addressed.
@binlijin FYI.
+1
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/apache/hadoop/hbase/master/assignment/TestDeadServerMetricRegionChore.java
Show resolved
Hide resolved
🎊 +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.
@bharathv The latest patch should have addressed all you comment.
...src/test/java/org/apache/hadoop/hbase/master/assignment/TestDeadServerMetricRegionChore.java
Show resolved
Hide resolved
🎊 +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.
@HorizonNet You were looking at this too, any more comments or is this good to go?
…1026) Signed-off-by: Bharath Vissapragada <[email protected]> (cherry picked from commit 24823ec)
…1151) Signed-off-by: Bharath Vissapragada <[email protected]> (cherry picked from commit 24823ec)
…1026) Signed-off-by: Bharath Vissapragada <[email protected]>
…1026) Signed-off-by: Bharath Vissapragada <[email protected]>
…1026) (apache#1151) Signed-off-by: Bharath Vissapragada <[email protected]> (cherry picked from commit 24823ec)
No description provided.