You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by GitBox <gi...@apache.org> on 2019/12/19 03:14:15 UTC
[GitHub] [sling-org-apache-sling-distribution-journal] akrainiouk opened a new pull request #18: SLING-8934 Create new metric for the number of subscribers of distrib…
akrainiouk opened a new 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
…ution publisher
----------------------------------------------------------------
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
[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…
Posted by 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
[GitHub] [sling-org-apache-sling-distribution-journal] cschneider commented on a change in pull request #18: SLING-8934 Create new metric for the number of subscribers of distrib…
Posted by GitBox <gi...@apache.org>.
cschneider 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_r360309936
##########
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:
You can not use the metricsService in the Constructor. Instead this must happen in activator. As the @Reference is mandatory you also do not need to check for null. Instead you could use requireNonNull(..) to make sure we do not forget to set it during tests.
----------------------------------------------------------------
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
[GitHub] [sling-org-apache-sling-distribution-journal] tmaret commented on issue #18: SLING-8934 Create new metric for the number of subscribers of distrib…
Posted by GitBox <gi...@apache.org>.
tmaret commented on issue #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#issuecomment-567963623
Approach LGTM, +1 with @cschneider findings.
----------------------------------------------------------------
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
[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…
Posted by 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 have committed a fix which cleans this duplication as well makes 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
[GitHub] [sling-org-apache-sling-distribution-journal] cschneider merged pull request #18: SLING-8934 Create new metric for the number of subscribers of distrib…
Posted by GitBox <gi...@apache.org>.
cschneider merged 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
----------------------------------------------------------------
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
[GitHub] [sling-org-apache-sling-distribution-journal] cschneider commented on a change in pull request #18: SLING-8934 Create new metric for the number of subscribers of distrib…
Posted by GitBox <gi...@apache.org>.
cschneider 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_r360638516
##########
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:
Alexei .. you are right.. it looked different in the diff view but when I look at the full file it is correct.
----------------------------------------------------------------
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