Re: [PR] bugfix: close metrics exporters with the server lifecycle [incubator-seata]

["slievrly (via GitHub)" <gi...@apache.org>]

slievrly commented on code in PR #8252:
URL: https://github.com/apache/incubator-seata/pull/8252#discussion_r4167163451


##########
server/src/main/java/org/apache/seata/server/metrics/MetricsManager.java:
##########
@@ -42,25 +49,55 @@ public static final MetricsManager get() {
     }
 
     private Registry registry;
+    private final List<Exporter> exporters = new ArrayList<>();
+    private MetricsSubscriber subscriber;
 
     public Registry getRegistry() {
         return registry;
     }
 
-    public void init() {
+    public synchronized void init() {
+        if (registry != null) {
+            return;
+        }
         boolean enabled = ConfigurationFactory.getInstance()
                 .getBoolean(
                         ConfigurationKeys.METRICS_PREFIX + ConfigurationKeys.METRICS_ENABLED, DEFAULT_METRICS_ENABLED);
         if (enabled) {
             registry = RegistryFactory.getInstance();
             if (registry != null) {
-                List<Exporter> exporters = ExporterFactory.getInstanceList();
+                exporters.addAll(ExporterFactory.getInstanceList());
                 // only at least one metrics exporter implement had imported in pom then need register MetricsSubscriber
                 if (exporters.size() != 0) {
                     exporters.forEach(exporter -> exporter.setRegistry(registry));
-                    EventBusManager.get().register(new MetricsSubscriber(registry));
+                    subscriber = new MetricsSubscriber(registry);
+                    EventBusManager.get().register(subscriber);
                 }
             }
         }
     }
+
+    @Override
+    public synchronized void destroy() {
+        if (subscriber != null) {
+            EventBusManager.get().unregister(subscriber);
+            subscriber = null;
+        }
+        for (Exporter exporter : exporters) {
+            try {
+                exporter.close();
+            } catch (Exception e) {
+                LOGGER.warn(
+                        "Failed to close metrics exporter {}",
+                        exporter.getClass().getName(),
+                        e);
+            }
+        }
+        if (!exporters.isEmpty()) {
+            // Closed SPI instances cannot be reused when the server starts again.
+            EnhancedServiceLoader.unload(Exporter.class);
+            exporters.clear();
+        }
+        registry = null;

Review Comment:
   Agreed. Commit 8d04257dc calls registry.clearUp() before dropping the registry reference. The lifecycle test verifies cleanup occurs once even with repeated destroy and an exporter close failure. MetricsManagerTest passes locally (2 tests, run by both configured Surefire executions).



-- 
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: notifications-unsubscribe@seata.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org