2
\$\begingroup\$

(following up from here)

I am working on a little project where I need to scan all the config files present in a folder on the disk and load it in memory. Below are the steps:

  • On the disk there is already a default Records folder which has all the default config files present. This is to fallback if loadDefaultFlag is enabled. We will never overwrite this config or delete at all.
  • There are also new config files present as a tar.gz file (max 100 MB size) in a remote url location which I need to download and store it on disk in secondary location only if loadDefaultFlag is disabled.

During Server startup

During server startup I need to either load local files from default Records folder or remote files by downloading it from remote server (and storing it in new secondary location) and then using it in memory.

{"loadDefaultFlag":"false", "remoteFileName":"abc-123.tgz", "reload":"false"}

For example: server started up and loaded abc-123.tgz config in memory.

After server startup

Case 1:

After server started up with some configs (abc-123.tgz) then someone from outside can tell us to download new configs from remote location again or go back and use default local configs from Records folder.

{"loadDefaultFlag":"true", "remoteFileName":"abc-123.tgz", "reload":"false"}

If loadDefaultFlag is true, then it means someone is telling from outside to load configs from default Records folder in memory so once this is changed, all machines will switch to use local configs in memory.

Case 2:

Second case can be someone telling to download new remote configs as we have new configs available that we should use now.

{"loadDefaultFlag":"false", "remoteFileName":"abc-124.tgz", "reload":"false"}

so now all machines will download abc-124.tgz onto the disk but they won't switch to these new configs yet in memory unless someone is instructing them from outside to start using new configs in memory. Save method actually switches config in memory from old to new. And that flag to switch to new config is reload - once that is true then all machines will switch to use new abc-124.tgz configs in memory.

Records folder which has default configs is just a backup and not meant to be used in regular cases.

Below is my code:

public class RecordManager
{
    private const string _remoteUrl = "remote-url-from-where-to-download-new-configs";
    private static string _remoteFileName; 

    private const string SecondaryLocation = "SecondaryConfigs";
    private readonly IConfiguration _configuration;
    private readonly string _localPath; 
    private IEnumerable<RecordHolder> _records; 
    
    public enum ConfigLocation { System, Local, Remote }
    
    public RecordManager(IConfiguration configuration, string localPath)
    {
        if(configuration == null) { throw new ArgumentNullException(nameof(configuration)); }
        
        if(localPath?.Length == 0) { throw new ArgumentNullException(nameof(localPath)); }
        
        _localPath = localPath;
        _configuration = configuration;
        ChangeToken.OnChange(configuration.GetReloadToken, _ => ConfigChanged(), new object());
    }
    
    public RecordManager(IConfiguration configuration) : this(configuration, "Records") { } 
    
    public RecordManager LoadConfigurationsFrom(ConfigLocation location)
    {
        switch(location)
        {
            case ConfigLocation.Remote:
                _records = GetConfigFromServer();
                break; 
            case ConfigLocation.Local:
                _records = GetConfigFromLocalFiles();
                break; 
            case ConfigLocation.System:
                _records = IsConfigFromServer() ?  GetConfigFromServer() : GetConfigFromLocalFiles();
                break;  
        }
        
        return this; 
    }
    
    public void Save()
    {
        // now load `_records` configs in memory here
        // only called once you are ready to switch
    }

    private bool IsConfigFromServer()
    {
        string configValue = configuration["configKey"];

        if (string.IsNullOrWhiteSpace(configValue)){ return false; }
        
        var dcc = JsonConvert.DeserializeObject<RecordPojo>(configValue);
        if(!bool.TryParse(dcc.loadDefaultFlag?.ToString(), out bool loadDefaultFlag)) { return false; }
        
        _remoteFileName = dcc.remoteFileName;
        
        return !loadDefaultFlag && !string.IsNullOrWhiteSpace(dcc.remoteFileName);
    }
    
    // download tar.gz file from remote server, store it on disk in secondary location
    // uncompress tar.gz file, read it and return RecordHolder list back.
    private IEnumerable<RecordHolder> GetConfigFromServer()
    {       
       var isDownloaded = _fileHelper.Download($"{_remoteUrl}{_remoteFileName}", _secondaryLocation);
        if(!isDownloaded) { yield return default; }
        
        var isExtracted = _fileHelper.ExtractTarGz(_remoteFileName, _directory);
        if(!isExtracted) { yield return default; }
        
        foreach(var configPath in _fileHelper.GetFiles(directory))
        {
            if(!File.Exists(configPath)) { continue; }
            
            var fileDate = File.GetLastWriteTimeUtc(configPath);
            var fileContent = File.ReadAllText(configPath);
            var pathPieces = configPath.Split(System.IO.Path.DirectorySeparatorChar, StringSplitOptions.RemoveEmptyEntries);
            var fileName = pathPieces[pathPieces.Length - 1];
            yield return new RecordHolder
            {
                Name = fileName,
                Date = fileDate,
                JDoc = fileContent
            };
        }
    }


    private IEnumerable<RecordHolder> GetConfigFromLocalFiles()
    {
        // read config files already present in default "Records" folder
        // and return RecordHolder list back.
    }

    // this can be improved a lot to achieve below cases in proper way
    private void ConfigChanged()
    {
        string configValue = _configuration["configKey"];
        if (string.IsNullOrWhiteSpace(configValue)) { return; }

        var dcc = JsonConvert.DeserializeObject<ConsulConfig>(configValue);
        bool.TryParse(dcc.loadDefaultFlag?.ToString(), out bool loadDefaultFlag);
        bool.TryParse(dcc.reloadConfig?.ToString(), out bool reloadConfig);
        _remoteFileName = dcc.remoteFileName;

        if (switchConfig) { Save(); }
        if (loadDefaultFlag) { _records = GetConfigFromLocalFiles(); }
        else { _records = GetConfigFromServer(); }
    }
}

This is how I use it as a fluent api and during server startup this will be called as it is:

new RecordManager(configuration)
    .LoadConfigurationsFrom(RecordManager.ConfigLocation.Remote)
    .Save();

Question:

Now as you can see I have a ChangeToken.OnChange notification enabled in my constructor where I need to do something whenever my configuration (configKey) is changed and it will invoke my ConfigChanged method. Basically after server startup is done and configs is loaded up in memory with above code then someone can tell us to download new configs again and then load it in memory and thats what I do in ConfigChanged method.

Opting for a code review here specifically for the case when I need to reload configs again and load it in memory. I am specifically interested in the way I have designed and implemented my code for ConfigChanged method. I am sure there must be a better way to rewrite the ConfigChanged method code in a better way that can handle all those above cases efficiently.

\$\endgroup\$
10
  • 1
    \$\begingroup\$ I'm a bit confused here, how come (in case 3) you need to download the files while it should be loaded from local config? if every case will require to download the file, then there is no need to have a special case for loading from local, and you'll always go back and check the server for newer version and downloaded if any, then update current stored config locally. \$\endgroup\$
    – iSR5
    Jul 21, 2020 at 17:14
  • \$\begingroup\$ yeah I mean for case 3 - sometimes people can say lets load local config (example like when we have issues downloading from remote server or we got wrong config files on remote server) as a fallback so for case 3 it should load from local configs again in memory. \$\endgroup\$
    – dragons
    Jul 21, 2020 at 17:16
  • \$\begingroup\$ then if loadDefaultFlag and reload are true, it should go and load local config, if any exceptions occurs, then you'll need to retry loading it again, if there another exception, then you can redownload the latest version and reloaded it. \$\endgroup\$
    – iSR5
    Jul 21, 2020 at 17:19
  • \$\begingroup\$ However, if reload is false (in both cases local and remote), it should flag the system to prepare to update the system with the selected configuration, in which will be loaded at certain pre-configured event (say restarting for instance). (the default event in your system). \$\endgroup\$
    – iSR5
    Jul 21, 2020 at 17:21
  • \$\begingroup\$ Idea is - Before server startup there is already a local default Records folder which has local configs and during server startup, it will try to load config either from local default folder or from remote server as you already know. We don't delete or override files in local default folder (Records) and once we download new files from remote server, we keep it at secondary folder always. Now server started up and working fine without any issues. \$\endgroup\$
    – dragons
    Jul 21, 2020 at 17:34

1 Answer 1

1
\$\begingroup\$

The Records is a backup configuration that would be used if there is any issues on the configuration.

What I think you need is the following workflow:

  1. On server startup, read the JSON configuration, download the file, and store JSON values statically.
  2. After server started up, if the JSON values have been changed, get the new values, compared them with the stored values, and execute the logic based on this comparison.

So, to prepare that we need to add a child private class which would store the json config values. Next, add static instance of this new private class which would store the current settings and On the ConfigChanged just compare between the new file name and the current one. Then, just load the settings from local or server or return the default values.

You need a separate method for loading the Default settings (which is the backup). So, at the end you'll have three methods for loading the configurations.

here is the changes you need (I have optout the rest of the code only included the changes).

public class RecordManager
{
    private static JsonConfiguation _jsonConfig; 

    private class JsonConfiguation
    {
        public string RemoteFileName { get; set; }

        public bool LoadDefault { get; set; }

        public bool Reload { get; set; }

        public bool HasNewerFile(JsonConfiguation jsonConfiguation)
        {
            return !RemoteFileName.Equals(jsonConfiguation.RemoteFileName, StringComparison.InvariantCultureIgnoreCase);
        }

        public bool IsConfigFromServer => !LoadDefault && !string.IsNullOrWhiteSpace(RemoteFileName);
    }


    
    public RecordManager(IConfiguration configuration, string localPath)
    {
        if(configuration == null) { throw new ArgumentNullException(nameof(configuration)); }
        
        if(localPath?.Length == 0) { throw new ArgumentNullException(nameof(localPath)); }
        
        _localPath = localPath;
        
        _configuration = configuration;
        
        if(_jsonConfig == null)
            _jsonConfig = GetConfigValuesFromJson();

        ChangeToken.OnChange(configuration.GetReloadToken, _ => ConfigChanged(), new object());
    }   

    private JsonConfiguation GetConfigValuesFromJson()
    {
        string configValue = _configuration["configKey"];
        
        if (string.IsNullOrWhiteSpace(configValue)) { throw new ArgumentNullException(nameof(configValue)); }

        var dcc = JsonConvert.DeserializeObject<ConsulConfig>(configValue);
        
        return new JsonConfiguation
        {
            RemoteFileName = dcc.remoteFileName, 
            LoadDefault = bool.TryParse(dcc.loadDefaultFlag?.ToString(), out bool loadDefaultFlag) ? loadDefaultFlag : false, 
            Reload = bool.TryParse(dcc.reloadConfig?.ToString(), out bool reloadConfig) ? reloadConfig : false
        };
    }

    
    private void ConfigChanged()
    {
        var configNew = GetConfigValuesFromJson();

        // fallback in case if something happened unexpectedly. 
        if (_jsonConfig == null)
        {
            _jsonConfig = configNew;
        }
       
        if(configNew.IsConfigFromServer)
        {
            // if both (the current downloaded and on the remote) are different, 
            // Redownload the file before going to the next step.
            // else just load the local config 

            _records = _jsonConfig.HasNewerFile(configNew) ? GetConfigFromServer() : GetConfigFromLocalFiles();
            _jsonConfig = configNew;
        }
        else
        {
            // here it will cover if the loadDefaultFlag is true or any other issue with the configuration (like missing values)
            // it will reload the default configuration (as a reset switch). 
            _records = GetDefaultConfiguration();
            _jsonConfig = configNew;
        }


        // if it requires to reload the configuration immediately
        // if not, it'll now reload the configuration, and it would be stored in this instance.
        if (configNew.Reload)
        {
            Save();
        }

    }

    private IEnumerable<RecordHolder> GetDefaultConfiguration()
    {
        // get the default config files already present in default "Records" folder
        // and return RecordHolder list back.
    }   

    private IEnumerable<RecordHolder> GetConfigFromServer()
    {
        // get the config files from the server 
        // and return RecordHolder list back.       
    }
    
    
    private IEnumerable<RecordHolder> GetConfigFromLocalFiles()
    {
        // get the config files from the secondary location 
        // and return RecordHolder list back.
    }               
}
\$\endgroup\$
9
  • \$\begingroup\$ I see one minor error on this line - _jsonConfig.RemoteFileName.Equals as cannot be accessed with an instance reference; qualify it with a type name instead. \$\endgroup\$
    – dragons
    Jul 22, 2020 at 2:26
  • \$\begingroup\$ Also I see you are using return like this return new JsonConfiguation but in the previous question you mentioned to use yield return new .... Just wanted to see why in this case we are using differently than other way? \$\endgroup\$
    – dragons
    Jul 22, 2020 at 2:27
  • \$\begingroup\$ @dragons code updated. About your return question, because it's returning JsonConfiguation and not IEnumerable<JsonConfiguation>. yield works only with IEnumerable<T>. I've mentioned that in my previous answer \$\endgroup\$
    – iSR5
    Jul 22, 2020 at 7:31
  • \$\begingroup\$ yeah I realized it after I commented and went back to your previous answer. It makes sense. Also that instance reference error is still there btw. \$\endgroup\$
    – dragons
    Jul 22, 2020 at 13:54
  • 1
    \$\begingroup\$ @dragons I've just found out that my code was duplicated, shame on me ;(. I've updated code, and also made a small changes to the json class. and configchanged method (i've also moved IsConfigFromServer method inside the json class. \$\endgroup\$
    – iSR5
    Jul 22, 2020 at 14:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.