Re: [tarantool-patches] Re: [PATCH v2 02/11] vinyl: factor load regulator out of quota

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

On Tue, Oct 02, 2018 at 09:16:56PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/09/28 21:00]:
> > Turned out that throttling isn't going to be as simple as maintaining
> > the write rate below the estimated dump bandwidth, because we also need
> > to take into account whether compaction keeps up with dumps. Tracking
> > compaction progress isn't a trivial task and mixing it in a module
> > responsible for resource limiting, which vy_quota is, doesn't seem to be
> > a good idea. Let's factor out the related code into a separate module
> > and call it vy_regulator. Currently, the new module only keeps track of
> > the write rate and the dump bandwidth and sets the memory watermark
> > accordingly, but soon we will extend it to configure throttling as well.
> 
> OK, if we keep regulator in place there is a few comments about
> the contents of the patch (i.e. the renames).
> 
> > -	struct vy_quota *q = &env->quota;
> > +	struct vy_regulator *r = &env->regulator;
> >  
> > -	info_table_begin(h, "quota");
> > -	info_append_int(h, "used", q->used);
> > -	info_append_int(h, "limit", q->limit);
> > -	info_append_int(h, "watermark", q->watermark);
> > -	info_append_int(h, "use_rate", q->use_rate);
> > -	info_append_int(h, "dump_bandwidth", q->dump_bw);
> > -	info_table_end(h); /* quota */
> > +	info_table_begin(h, "regulator");
> > +	info_append_int(h, "watermark", r->watermark);
> > +	info_append_int(h, "write_rate", r->write_rate);
> > +	info_append_int(h, "dump_bandwidth", r->dump_bw);
> 
> I think "watermark" should be now "dump_watermark", sicne simply
> "watermark" is not specific enough. Is it regulator watermark? A
> regulator watermark doesn't make any sense to me.
> 
> The rename should take place across the entire code base imho.
> 
> >  	assert(mem_used_after >= mem_used_before);
> >  	vy_quota_adjust(&env->quota, tx->write_size,
> >  			mem_used_after - mem_used_before);
> > +	vy_regulator_check_watermark(&env->regulator);
> 
> Like check_dump_watermark.
> 
> >  vy_env_quota_exceeded_cb(struct vy_quota *quota)
> >  {
> >  	struct vy_env *env = container_of(quota, struct vy_env, quota);
> > +	vy_regulator_no_memory(&env->regulator);
> 
> IMHO vy_regulator_quota_exceeded is a better name.

Makes sense to me. I also renamed dump_bw to dump_bandwidth, because
we typically don't use abbreviations in member names, and used_last to
quota_used_last and pushed it to 1.10.