Vladsz83 commented on a change in pull request #7446: IGNITE-12464 : Service 
metrics
URL: https://github.com/apache/ignite/pull/7446#discussion_r401133474
 
 

 ##########
 File path: 
modules/core/src/main/java/org/apache/ignite/internal/processors/service/IgniteServiceProcessor.java
 ##########
 @@ -1801,4 +1842,178 @@ private boolean enterBusy() {
     private void leaveBusy() {
         opsLock.readLock().unlock();
     }
+
+    /**
+     * Registers metrics to measure durations of service methods.
+     *
+     * @param srvc Service for invocations measurement.
+     * @param srvcName Name of {@code srvc}.
+     */
+    private void registerMetrics(Service srvc, String srvcName) {
+        
getInterfaces(srvc.getClass()).stream().map(Class::getMethods).flatMap(Arrays::stream)
 
 Review comment:
   Same here. You are right about usage of separate method. But the 
synchronization you suggest seems already relies in the logic above. I don’t 
think `IgniteServiceProcessor.undeploy()` and ` 
IgniteServiceProcessor.redeploy()` can happen concurrently but we should 
consider it can, ok. Let’s see:
   -    Metrics are erased by `IgniteServiceProcessor.unregisterMetrics()` in 2 
places (): `IgniteServiceProcessor.redeploy()` and 
`IgniteServiceProcessor.undeploy()`. Both leads to removing whole map by 
service name ‘invocationHistograms.remove(srvcName);’
   -    Metrics are created by `IgniteServiceProcessor.registerMetrics()`  in 
one place: `IgniteServiceProcessor.redeploy()`.
   
   These calls are not any APIs, are private.
   
   All 3 calls covered with sync by service context collection given bt same 
serviced:
   ```
   Collection<ServiceContextImpl> ctxs …
   synchronized (ctxs) {
        unregisterMetrics(cfg.getName());
           }
   ```
   and
   ```
   Collection<ServiceContextImpl> ctxs …
               synchronized (ctxs) {
                   if (!ctxs.iterator().next().isCancelled())
                       registerMetrics(srvc, srvcCtx.name());
               } 
   ```
   
   I think I really missed the last part earlier. Thanks for the note. Isn’t it 
the synchronized block you suggest?

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


With regards,
Apache Git Services

Reply via email to