[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5136: Separate config initialization in the plugin out of core level Config.

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

kezhenxu94 commented on a change in pull request #5136:
URL: https://github.com/apache/skywalking/pull/5136#discussion_r457363911



##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/boot/PluginConfigInitializer.java
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.boot;
+
+import java.util.ServiceLoader;
+import org.apache.skywalking.apm.agent.core.conf.SnifferConfigInitializer;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+
+/**
+ * PluginConfigInitializer loads Config(s) in all existing plugins, and initialize them through current agent settings.
+ */
+public class PluginConfigInitializer {
+    public void initConfigurationsOfAllPlugins() {
+        final ServiceLoader<ConfigInitializationService> configServiceLoader = ServiceLoader.load(
+            ConfigInitializationService.class, AgentClassLoader.getDefault());
+        configServiceLoader.forEach(configInitializationService -> {
+            SnifferConfigInitializer.initializeConfig(configInitializationService.config());

Review comment:
       Here is what I mean
   
   ```suggestion
               SnifferConfigInitializer.initializeConfig(configInitializationService.getClass());
   ```

##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/boot/ConfigInitializationService.java
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.boot;
+
+/**
+ * ConfigInitializationService provides the config class which should host all parameters originally from agent setup.
+ * {@link org.apache.skywalking.apm.agent.core.conf.Config} provides the core level config, all plugins could implement
+ * this interface to have the same capability about initializing config from agent.config, system properties and system
+ * environment variables.
+ */
+public interface ConfigInitializationService {
+    /**
+     * @return Config to host parameters, all static fields set based on the config variable name.
+     */
+    Class config();

Review comment:
       I didn't see any particular usage of this method, all implementations of this interface return the class itself, just to get the implemented class, I think this method is no needed actually 




----------------------------------------------------------------
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