[bind10-dev] proposal: separate ModuleCCSession and ConfigData

[JINMEI Tatuya / 神明達哉]

At Wed, 28 Mar 2012 21:47:17 -0700,
JINMEI Tatuya <jinmei at isc.org> wrote:

> I have a specific proposal.  In short, the idea is to separate the
> ModuleCCSession and ConfigData classes separately:
> 
> _ New ModuleCCSession will only be responsible for the protocol with
>   cfgmgr, synchronous/asynchronous communication, and subscriber
>   management.  "Module" will now just be opaque string for this class.
> - New ConfigData will be responsible for the data handling part of the
>   current ModuleCCSession, such as merging new and old config,
>   validation according to the spec syntax, etc.

[snip]

> I'll send detailed background and other design details in a separate
> message.

...and here are the details.  I'd like to get opinions and discuss if
it makes sense and (if so) when we do it.

I've been feeling it's getting difficult to figure out what happens
inside ModuleCCSession when I debug something related to it.  Maybe it
was not so originally, but today it does so many complicated things in
a single class: handling asynchronous communication, some part of
configuration data management, and even doing something special for
logging.  It also makes difficult to test things related to the
class.  For example, to check whether configuration updates are
correctly reflected we need to emulation communication over a faked
channel.

It's more so as we see the need for newer concepts such as logging
related configuration or modules that don't belong to a process (so
there's no spec file), like "tsig_key".  On the other hand,
communication part of ModuleCCSession is also pretty complicated, as
many asynchronous operations are naturally so.  This led to the
trouble we had before when we added tsig_key config to auth
(separating the session and data classes wouldn't automatically solve
this type of issue, but I believe it will be easier to figure out the
code and fix issues with simpler changes).  It also mixes the initial
configuration and subsequent updates, which is one background cause of
#1707.  Again, separating session and data itself wouldn't
automatically solve the issue, but I think moving such details outside
of the communication class will make it easier to manage.

By separating these two responsibilities, I believe we can test each
part more easily.  We don't have to use faked session to track how
config updates are managed, and we don't have to test session message
handling through resulting configuration changes.

This is a possible interface of the revised classes (incomplete and
tentative, mainly for explaining the concept with specific API
examples, rather than providing precise definition to be implemented):

class ModuleCCSession {
public:
    ModuleCCSession(AbstractSession& session);

    // send the module spec, synchronously.
    void sendSpec(const std::string& module_name, ConstElementPtr spec);

    // Adds the module name to the internal subscription list with the
    // callback, and if not yet, subscribe to new messages for the module.
    // When a non config command is received via the session, it extracts
    // the data from the message, and calls the callback with it.
    // callback is supposed to return a null ElementPtr if it's not expected
    // respond to the update (as in the case of subscribing to changes to
    // other modules).
    void setCommandCallback(const std::string& module_name,
                            CallbackType callback);

    // Similar for config updates.
    void setCofigCallback(const std::string& module_name,
                          CallbackType callback);

    // get the latest local configuration from cfgmgr, synchronously.
    ConstElementPtr getConfig(const std::string& module_name);

    // get the module spec from cfgmgr, synchronously.
    ConstElementPtr getSpec(const std::string& module_name);

    // Start asynchronous session
    void start();
};

// This class internally maintains (in readonly) the config spec and the latest
// full data (default from the spec + latest update).
class ConfigData {
public:
    // Constructor from the module name.  It will be used for things like
    // tsig_key.
    ConfigData(const string& module_name, ModuleCCSession& ccsession,
               ConfigCallbackType callback);

    // Constructor from the module spec.  Used when the module spec is
    // expected to be generated from a file and the caller knows where the
    // file is (maybe via another helper API).
    ConfigData(const ModuleSpec& module_spec, ModuleCCSession& ccsession,
               ConfigCallbackType callback);

    // Start managing configuration.  It first gets the latest config of
    // the module via the cc session synchronously.  If the it was constructed
    // from a module name, it also gets the spec file from the cc session
    // beforehand.  It then constructs the full data, and calls the callback.
    // If it succeeds, it subscribes to subsequent updates for the module
    // via the cc session.  After that, if the subscription callback is
    // called, it validates the given config data, updates the internal config
    // data and calls the callback with the difference.
    void start();
};

I think it's not so difficult to implement this separation from the
current implementation.  I guess we could extract ModuleCCSession as
an independent class (instead of being a derived class of ConfigData),
and move config related features from ModuleCCSession to ConfigData.
Of course, there are many small things to be adjusted, but it
shouldn't be so radical like the migration of the data source APIs we
did before.

---
JINMEI, Tatuya