[GitHub] [dubbo-go] flycash commented on a change in pull request #1030: Improve config center

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

flycash commented on a change in pull request #1030:
URL: https://github.com/apache/dubbo-go/pull/1030#discussion_r567779148



##########
File path: common/config/environment.go
##########
@@ -72,22 +72,34 @@ func (env *Environment) UpdateExternalConfigMap(externalMap map[string]string) {
 	for k, v := range externalMap {
 		env.externalConfigMap.Store(k, v)
 	}
+	env.externalConfigMap.Range(func(key, value interface{}) bool {
+		if _, ok := externalMap[key.(string)]; !ok {
+			env.externalConfigMap.Delete(key)

Review comment:
       In which case this code will be excuted? 
   
   It looks like that the unit tests do not cover this special case.

##########
File path: common/config/environment.go
##########
@@ -72,22 +72,34 @@ func (env *Environment) UpdateExternalConfigMap(externalMap map[string]string) {
 	for k, v := range externalMap {
 		env.externalConfigMap.Store(k, v)
 	}
+	env.externalConfigMap.Range(func(key, value interface{}) bool {
+		if _, ok := externalMap[key.(string)]; !ok {
+			env.externalConfigMap.Delete(key)
+		}
+		return true
+	})
 }
 
 // UpdateAppExternalConfigMap updates env appExternalConfigMap field
 func (env *Environment) UpdateAppExternalConfigMap(externalMap map[string]string) {
 	for k, v := range externalMap {
 		env.appExternalConfigMap.Store(k, v)
 	}
+	env.appExternalConfigMap.Range(func(key, value interface{}) bool {
+		if _, ok := externalMap[key.(string)]; !ok {
+			env.appExternalConfigMap.Delete(key)
+		}
+		return true
+	})

Review comment:
       I think those lines could be refactor and extract them to be a new function

##########
File path: config/config_loader.go
##########
@@ -194,6 +207,17 @@ func loadProviderConfig() {
 	}
 	checkRegistries(providerConfig.Registries, providerConfig.Registry)
 
+	// Write the current configuration to cache file.
+	if providerConfig.CacheFile != "" {
+		if data, err := yaml.MarshalYML(providerConfig); err != nil {
+			logger.Errorf("Marshal provider config err: %s", err.Error())
+		} else {
+			if err := ioutil.WriteFile(providerConfig.CacheFile, data, 0666); err != nil {
+				logger.Errorf("Write provider config cache file err: %s", err.Error())
+			}
+		}
+	}

Review comment:
       See my above comment

##########
File path: config/config_loader.go
##########
@@ -138,6 +140,17 @@ func loadConsumerConfig() {
 		ref.Implement(rpcService)
 	}
 
+	// Write current configuration to cache file.
+	if consumerConfig.CacheFile != "" {
+		if data, err := yaml.MarshalYML(consumerConfig); err != nil {
+			logger.Errorf("Marshal consumer config err: %s", err.Error())
+		} else {
+			if err := ioutil.WriteFile(consumerConfig.CacheFile, data, 0666); err != nil {
+				logger.Errorf("Write consumer config cache file err: %s", err.Error())
+			}
+		}
+	}

Review comment:
       I am not sure whether we should do this like:
   ```go
   (c *consumerConfig) SaveToCacheFile() {
       // ...
   }
   
   // in this funtion
   
   consumerConfig.SaveToFile()
   ```




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



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