[GitHub] [sling-org-apache-sling-distribution-journal] akrainiouk commented on a change in pull request #18: SLING-8934 Create new metric for the number of subscribers of distrib…

[GitBox <gi...@apache.org>]

akrainiouk commented on a change in pull request #18: SLING-8934 Create new metric for the number of subscribers of distrib…
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal/pull/18#discussion_r360510776
 
 

 ##########
 File path: src/main/java/org/apache/sling/distribution/journal/impl/publisher/DistributionPublisher.java
 ##########
 @@ -167,6 +173,13 @@ public void activate(PublisherConfiguration config, BundleContext context) {
         
         String msg = String.format("Started Publisher agent %s with packageBuilder %s, queuedTimeout %s",
                 pubAgentName, pkgType, queuedTimeout);
+        if (metricsService != null) {
 
 Review comment:
   This **is** and activator code, not sure were this concern about constructor is coming from.
   metricsService field is indeed properly injected in real OSGI environment. The problem was in the unit test where it was remaining null. I think I found the root cause of it though, which is a name mismatch between the mock defined in the test and actual field name. I also missed the fact that there is already an existing reference to DistributionMetricsService available in DistributionPublisher under proper name. So I am committing the fix which will clean this duplication and at the same time make above null check unnecessary.
   I wish I could also add unit test for the metric as well, but this requires much more extensive mocking so I am deferring it for later.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services