[GitHub] [sling-org-apache-sling-event] stefan-egli commented on a diff in pull request #30: Removing infinite loop

["stefan-egli (via GitHub)" <gi...@apache.org>]

stefan-egli commented on code in PR #30:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/30#discussion_r1245096454


##########
src/main/java/org/apache/sling/event/impl/jobs/stats/GaugeSupport.java:
##########
@@ -150,9 +154,10 @@ private void registerWithSuffix(String suffix, int count, Gauge<Long> value) {
             metricRegistry.register(metricName, value);
             gaugeMetricNames.add(metricName);
         } catch (IllegalArgumentException e) {
-            if (queueName != null) {
-                registerWithSuffix(suffix, count + 1, value);
-            }
+            logger.error("Failed to register mbean with suffix " + suffix, e);
+            // if (queueName != null) {
+            //     registerWithSuffix(suffix, count + 1, value);
+            // }

Review Comment:
   +1 to what @klcodanr recommends.
   
   This should do a log.debug for the first `n` times, then when it hits the limit `n` only do a log.error. The way it is done now is removing support for metric name collisions due to name sanitization (which is one legitimate reason for this retry)
   
   > I wouldn't think it would have to be large say 5(?)
   
   agree, I thought about `10` initially, but could be somewhere in that range, yes.



-- 
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: dev-unsubscribe@sling.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org