[
https://issues.apache.org/jira/browse/HBASE-22804?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16905424#comment-16905424
]
Caroline commented on HBASE-22804:
----------------------------------
Thanks Andrew, stack, and Mingliang for reviewing.
@[~apurtell]:
_>_ Therefore I propose that there be a follow up patch which makes Canary
Public/Evolving.
I can make this change after this one has been committed.
@[~stack]:
> ... see how avoid direct reference to guava and instead use a shaded internal
>version at org.apache.hbase.thirdparty..... That is probably what the above
>shadedjars complaints are about.
Thanks for the suggestion. As Mingliang pointed out, I can replace that line
with private Map<String, RegionTaskResult> regionMap = new
ConcurrentHashMap<>().
> Should this be an Interface so we can add more methods to it w/o breaking it
>(using 'default' implementations?) given it looks like you have to embed the
>Canary to get access to these new APIs?
Only region mode would use this API.. so I'm not sure there's a need for an
interface.
> Oh, maybe say a bit on how you intend to use this new API? Would help. Thanks.
We hope to address two concerns here: 1) was the region reachable (did
read/write succeed)? This will allow us to derive a more accurate HBase
availability metric by calculating a percentage of regions which are
responsive; and 2) did the request complete in time to meet the use case SLO?
(To determine this, we need read/write latencies for each region.)
@[~liuml07]:
I generally agree with your suggestions.
> {{TableName tableName = TableName.valueOf("testTable");}} Test table name can
>be a variable and be referenced later. The value can be the test method name
>to avoid potential conflict.
While this is a good suggestion, I will not change the other test cases already
present in order to conform to this strategy, because it seems irrelevant to
this patch. I will use a different table name for the new unit test.
> {{private Map<String, RegionTaskResult> regionMap =
>Maps.newConcurrentMap();}} can be replaced with {{private Map<String,
>RegionTaskResult> regionMap = new ConcurrentHashMap<>();}} to not use guava?
>Actually I'm thinking do we need it to be concurrent Map? We populate all
>regions before the sniff and then all other places simply get the individual
>item to update. Thoughts?
I think it's safer to use a Concurrent Map because there are 16 threads to
contact the regions, and they each need to access the map whenever they
publishReadTiming() or publishWriteTiming(). Although I highly doubt they will
try to access the same key, I think it's better to use a thread-safe
implementation when there are multiple threads. It should not have a
performance issue since the locking is at hashmap bucket level, not object
level.
> Provide an API to get list of successful regions and total expected regions
> in Canary
> -------------------------------------------------------------------------------------
>
> Key: HBASE-22804
> URL: https://issues.apache.org/jira/browse/HBASE-22804
> Project: HBase
> Issue Type: Improvement
> Components: canary
> Affects Versions: 3.0.0, 1.3.0, 1.4.0, 1.5.0, 2.0.0, 2.1.5, 2.2.1
> Reporter: Caroline
> Assignee: Caroline
> Priority: Minor
> Labels: Canary
> Attachments: HBASE-22804.branch-1.001.patch,
> HBASE-22804.branch-2.001.patch, HBASE-22804.master.001.patch
>
>
> At present HBase Canary tool only prints the successes as part of logs.
> Providing an API to get the list of successes, as well as total number of
> expected regions, will make it easier to get a more accurate availability
> estimate.
>
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
