-
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-28204 : Region Canary can take lot more time If any region (except the first region) starts with delete markers #5675
Conversation
… first region) starts with delete markers
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.
Left minor nits, +1 otherwise. Really nice improvement!
// regions in sequence until it can find first available row. | ||
// | ||
// This could result in multiple millions of scans based on the size of table and number of | ||
// empty regions in sequence. In test environment, A table no data and 1000 empty regions, |
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: A table with no data
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.
Done
} else { | ||
scan = new Scan(); | ||
// In case of first region of the HBase Table, we do not have start-key for the region. | ||
// For Region Canary, we only need scan a single row/cell in the region to make sure that |
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: we only need to scan
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.
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.
At least this change does not change any behavior of the Canary, other than avoiding unnecessary scans for empty tables/regions, and avoiding a Get of a very large deleted row, if it exists. Seems to be a strict improvement in that sense.
If anyone was relying on the RPCs being Gets, by using separate RPC handlers for Gets vs. Scans, this will change that behavior. But that seems like weird behavior to rely on.
+1
// do canary run for the table. | ||
// | ||
// Since First region of the table doesn't have any start key, We should set End Key as | ||
// stop row and set inclusive=false to limit scan to single region only. |
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 could even add a comment that we think this would be the better approach for all regions (not only the first region of the table), but who knows what performance people may expect from the canary, and whether they are counting on most Get requests (now Scans) not returning any data, and therefore performing extra fast (?)
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.
Added some more comments as well as TODO for future.
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@d-c-manning 's suggestion is valid. Should we include this as JIRA release note? So it's documented for every branch that has this JIRA? @virajjasani |
Sounds good, feel free to update release note on the Jira. |
🎊 +1 overall
This message was automatically generated. |
Updated Release note on the JIRA @virajjasani |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…ept the first region) starts with delete markers (#5675) Signed-off-by: David Manning <david.manning@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
…ept the first region) starts with delete markers (#5675) Signed-off-by: David Manning <david.manning@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
…ept the first region) starts with delete markers (#5675) Signed-off-by: David Manning <david.manning@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
…ept the first region) starts with delete markers (#5675) Signed-off-by: David Manning <david.manning@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
…ept the first region) starts with delete markers (#5675) Signed-off-by: David Manning <david.manning@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Jira: HBASE-28204