sanjeet006py commented on code in PR #6868:
URL: https://github.com/apache/hbase/pull/6868#discussion_r2096306546
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/metrics/ScanMetrics.java:
##########
@@ -51,53 +51,118 @@ public class ScanMetrics extends ServerSideScanMetrics {
/**
* number of RPC calls
*/
- public final AtomicLong countOfRPCcalls =
createCounter(RPC_CALLS_METRIC_NAME);
+ @SuppressWarnings("checkstyle:VisibilityModifier")
Review Comment:
> So previously it would be "reasonable" for a consumer to grab the handle
to this Atomic counter from any thread and then have confidence that their
handle will be viable for the lifetime of the program. By making the attributes
non-final and re-initializing their pointers, you break a kind of concurrency
pattern that was present here.
Earlier someone getting hold of `AtomicInteger` metric instance and using it
throughout the program made sense as the metrics were collected as sum of
metrics per region scanned. And, this contract is still valid if scan metrics
by region are not enabled. If scan metrics by region are enabled, user can get
hold of `AtomicLong` instance though this instance will be different per region
but the instance itself is viable for the lifetime of the program. With this
feature enabled I don't see how getting hold of `AtomicLong` metric instances
can be helpful.
> In order to preserve semantic compatibility, I think that you need to
retain the final annotation on these fields.
I think this point is also related to above only.
> I'm really disappointed by the situation with these public fields.
> Since locking down these fields will be an API-breaking change, this class
is annotated as InterfaceAudience.Public, and you want to get this feature into
the existing release lines, I suggest that you approach the API improvement
from a separate PR.
Yeah, I also wonder why these fields have been directly exposed as Public.
Regarding improving the API I think this can only be done for `master` branch
as for rest of the release branches this will be API breaking change as you
mentioned. Please correct me if I am wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
