-
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-24967 The table.jsp cost long time to load if the table include… #2326
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -655,12 +656,14 @@ | |||
<% | |||
if (master.getAssignmentManager().isTableEnabled(table.getName())) { | |||
try { | |||
CompactionState compactionState = admin.getCompactionState(table.getName()).get(); | |||
CompactionState compactionState = admin.getCompactionState(table.getName()).get(1, TimeUnit.SECONDS); |
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 sec is too short? And another way is: get the compaction state from master's memory and avoid rpc call.
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 the suggestion, let me try.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Could you help to review it if you have time, thanks. @infraio |
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.
So the intention here is that, the admin.getCompactionState is slow so on web ui we use the method from HMaster directly?
.setLastMajorCompactionTs(r.getOldestHfileTs(true)); | ||
r.setCompleteSequenceId(regionLoadBldr); | ||
|
||
LOG.info(">>>>>>setCompactionState:{}",r.getCompactionState()); |
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.
Debug log?
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.
Will fix, thanks.
* @param tableName The table name | ||
* @return CompactionState Compaction state of the table | ||
*/ | ||
public org.apache.hadoop.hbase.client.CompactionState getCompactionState( |
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.
Why full class name here?
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.
Conflict with org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.GetRegionInfoResponse.CompactionState
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.
Then better not import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.GetRegionInfoResponse.CompactionState directly? Import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.GetRegionInfoResponse, and then use GetRegionInfoResponse.CompactionState to reference the protobuf message.
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.
Good idea, will fix, thanks.
public RegionMetricsBuilder setCompactionState(CompactionState compactionState) { | ||
this.compactionState = compactionState; | ||
return this; | ||
} |
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.
New line here?
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, will fix.
@@ -164,10 +166,10 @@ private void compaction(final String tableName, final int flushes, | |||
long curt = System.currentTimeMillis(); | |||
long waitTime = 5000; | |||
long endt = curt + waitTime; | |||
CompactionState state = admin.getCompactionState(table); | |||
CompactionState state = master.getCompactionState(table); |
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 this test is for testing the HBaseAdmin.getCompactionState so we should not change to use master?
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.
Make sense, will fix later, thanks.
💔 -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. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Ping @Apache9 |
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, so the solution is to add compaction state to RegionMetrics, so when getting the compaction state, we could use the in memory state instead of doing rpc.
+1
Yeah, thanks for your time. |
… closed regions