elek commented on a change in pull request #481:
URL: https://github.com/apache/ratis/pull/481#discussion_r650698669
##########
File path:
ratis-metrics/src/main/java/org/apache/ratis/metrics/RatisMetrics.java
##########
@@ -31,9 +31,16 @@
protected RatisMetricRegistry registry;
protected static RatisMetricRegistry create(MetricRegistryInfo info) {
+ return create(info, true);
+ }
+
+ protected static RatisMetricRegistry create(MetricRegistryInfo info,
+ boolean shouldDebugLog) {
Optional<RatisMetricRegistry> metricRegistry =
MetricRegistries.global().get(info);
return metricRegistry.orElseGet(() -> {
- LOG.info("Creating Metrics Registry : {}", info.getName());
+ if (shouldDebugLog) {
Review comment:
I understand the motivation but for the sake of the simplicity I would
vote to always use LOG.debug level if `LOG.isDebugEnabled` (and drop the
specific parameter). I think the information here is not so important to have
an explicit flag for it. If somebody were interested, debug log level always
can be adjusted.
(But I am fine with either approach)
##########
File path:
ratis-metrics/src/main/java/org/apache/ratis/metrics/RatisMetrics.java
##########
@@ -31,9 +31,16 @@
protected RatisMetricRegistry registry;
protected static RatisMetricRegistry create(MetricRegistryInfo info) {
+ return create(info, true);
+ }
+
+ protected static RatisMetricRegistry create(MetricRegistryInfo info,
+ boolean shouldDebugLog) {
Optional<RatisMetricRegistry> metricRegistry =
MetricRegistries.global().get(info);
return metricRegistry.orElseGet(() -> {
- LOG.info("Creating Metrics Registry : {}", info.getName());
+ if (shouldDebugLog) {
+ LOG.debug("Creating Metrics Registry : {}", info.getName());
Review comment:
We have INFO log in the same file:
```
LOG.info("Unregistering Metrics Registry : {}", info.getName());
```
We can also modify the debug level here...
##########
File path:
ratis-metrics/src/main/java/org/apache/ratis/metrics/impl/MetricRegistriesImpl.java
##########
@@ -61,11 +61,6 @@ public MetricRegistriesImpl(MetricRegistryFactory factory) {
@Override
public RatisMetricRegistry create(MetricRegistryInfo info) {
return registries.put(info, () -> {
- if (reporterRegistrations.isEmpty()) {
- LOG.warn(
- "First MetricRegistry has been created without registering
reporters. You may need to call" +
- " MetricRegistries.global().addReporterRegistration(...)
before.");
- }
Review comment:
bq. Currently, if we run a load on the system, we keep getting these
warning messages from ratis clients. It does not serve the purpose to spam the
client logs with this warning message, as it needs to fixed on server side.
I am fine with removing the warning, but can you please give more
information how can the problem be reproduced? As far as I remember I also saw
it earlier, but today I just tested with `ozone sh` and `ozonen freon` and
couldn't reproduce.
Do we have any racecondition somewhere between adding reporters +
registering the first metric? Or is the metric registration missing from
somewhere?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
