Re: [tarantool-patches] [PATCH 2/2] On ctl event trigger

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Thu, Aug 30, 2018 at 03:38:07PM +0300, Vladimir Davydov wrote:
> On Tue, Aug 28, 2018 at 07:19:13PM +0300, Georgy Kirichenko wrote:
> > Introduce a ctl event trigger fired in cases of a bootstrap/recovery status
> > changes, a space create/alter/drop action, an applier state change and
> > shutdown. Trigger could be set with box.ctl_event even before the first
> > box.cfg invocation to control recovery and bootstrap behavior.
> > 
> > Event constants accessible via box.ctl_event.const()
> > There are events:
> >   - RECOVERY
> >   - SPACE
> >   - SHUTDOWN
> >   - APPLIER
> > 
> > A recovery event might have a status:
> >    * RECOVERY_SNAPSHOT_START
> >    * RECOVERY_SNAPSHOT_DONE
> >    * RECOVERY_HOT_STANDBY_START
> >    * RECOVERY_HOT_STANDBY_DONE
> >    * RECOVERY_XLOGS_DONE
> >    * RECOVERY_BOOTSTRAP_START
> >    * RECOVERY_BOOTSTRAP_DONE
> >    * RECOVERY_INITIAL_JOIN_START
> >    * RECOVERY_INITIAL_JOIN_DONE
> >    * RECOVERY_FINAL_JOIN_DONE
> > 
> > A space event consists of space identifier and action:
> >    * SPACE_CREATE
> >    * SPACE_ALTER
> >    * SPACE_DELETE
> > 
> > An applier event contains peer uuid and state:
> >    * APPLIER_OFF
> >    * APPLIER_CONNECT
> >    * APPLIER_CONNECTED
> >    * APPLIER_AUTH
> >    * APPLIER_READY
> >    * APPLIER_INITIAL_JOIN
> >    * APPLIER_FINAL_JOIN
> >    * APPLIER_JOINED
> >    * APPLIER_SYNC
> >    * APPLIER_FOLLOW
> >    * APPLIER_STOPPED
> >    * APPLIER_DISCONNECTED
> >    * APPLIER_LOADING
> 
> I can't review this patch properly, because ctl_event.[hc] are missing,
> but I've a few notes about the API.
> 
> First, I don't understand why you need two kinds of constants. I mean
> SPACE and SPACE_CREATE, or RECOVERY and RECOVERY_SNAPSHOT_START. IMO
> there should be just one set of constants.
> 
> Second, IMO the event trigger should be installed with box.ctl.on_event
> (not box.ctl_event.on_ctl_event), and constants should be defined in
> box.ctl.event (not box.ctl_event.const()).
> 
> Third, let's start with fewer events, only ones that we need to right
> now (e.g. for replication conflict resolution with before_replace).
> We can add other events later, when we need to.

Another thought. I think that mixing space, applier, and global events
in the same trigger isn't a good idea. I'd prefer box.ctl.on_event (or
whatever it's going to be called) to be called only on global events,
such as recovery completion or shutdown. Probably, it shouldn't be
passed any context apart from the event type, because I can't think why
it would ever need it (if it needs anything it can always get it from
box.info).

For applier events, I'd prefer to have a separate trigger, which would
only be called whenever an applier state changes. It should be passed
applier id in the arguments - everything else it can retrieve from
box.info.

Space events look rather superfluous to me, because we already can track
creation of system objects with box space.on_replace. True, we can't
install it before recovery is complete, but we could introduce a global
event called on completion of recovery of all system spaces instead.
We could use it to install before_replace trigger for conflict
resolution.

Anyway, this topic cries for a discussion/brainstorming IMO.