Add server/cache support to gaeconfig.

Add server/cache support to gaeconfig.

Rewrite "gaeconfig" and apps that use it to use "server/cache" instead.

Because this fully replaces usage of common/config/caching and
common/config/impl/erroring, delete those packages. Also, common/config
is no longer used in Contexts, so delete that as well.

BUG=chromium:674378
TEST=local,unit

Review-Url: https://codereview.chromium.org/2575383002
Committed: https://github.com/luci/luci-go/commit/45e9a86d32dabca5671d0c1773e124dbc2ec8b5c

[dnj, 2016-12-15 05:11:33]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2016-12-15 05:11:34]

PTAL. Full stack is:
https://codereview.chromium.org/2580713002 (base)
https://codereview.chromium.org/2578893002 (textproto)
https://codereview.chromium.org/2576993002 (access)
https://codereview.chromium.org/2576343002 (erroring)
https://codereview.chromium.org/2582433002 (test)
https://codereview.chromium.org/2573403002 (caching)
https://codereview.chromium.org/2575383002 <-- this
https://codereview.chromium.org/2576923003 (datastore cache)

[iannucci, 2017-01-07 21:05:26]

looks reasonable to me, but I think it would be safer to change each service in
individual CLs, followed immediately by deployment; this change would be
dangerous to have be a sleeper CL in some rollup on a per-service basis.

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/cache.go
File appengine/gaeconfig/cache.go (right):

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/cache.g...
appengine/gaeconfig/cache.go:5: package gaeconfig
this package could be split into luci-config/caching/gaememcache and
luci-config/caching/gaedatastore or something like that maybe?

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/cache.g...
appengine/gaeconfig/cache.go:62: return errors.Reason("entry exceeds memcache
size (%(size)d > %(max)d)").
should this be logged instead of an error?

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/default.go
File appengine/gaeconfig/default.go (right):

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/default...
appengine/gaeconfig/default.go:53: func Use(c context.Context) context.Context {
return useImpl(c, nil) }
if you do the package renaming, this should move out (maybe
`luci-config/setup/gaeluciconfig` with just the relevant usage bits, the intent
being that the setup/* packages would be full instantiations for various
environments or something.)

https://codereview.chromium.org/2575383002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/auth.go (right):

https://codereview.chromium.org/2575383002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/auth.go:75: // Artifically handle "all".
:( (for previous 'all is a valid group name' reasons)

[dnj, 2017-01-10 03:30:07]

PTAL, updated.

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/cache.go
File appengine/gaeconfig/cache.go (right):

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/cache.g...
appengine/gaeconfig/cache.go:5: package gaeconfig
On 2017/01/07 21:05:26, iannucci wrote:
> this package could be split into luci-config/caching/gaememcache and
> luci-config/caching/gaedatastore or something like that maybe?

Sure, done.

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/cache.g...
appengine/gaeconfig/cache.go:62: return errors.Reason("entry exceeds memcache
size (%(size)d > %(max)d)").
On 2017/01/07 21:05:26, iannucci wrote:
> should this be logged instead of an error? 

I don't think so. If config entries are really big, I don't think it's the cache
layer's duty to complain. If we want to add a constriant filter elsewhere we can
do that.

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/default.go
File appengine/gaeconfig/default.go (right):

https://codereview.chromium.org/2575383002/diff/1/appengine/gaeconfig/default...
appengine/gaeconfig/default.go:53: func Use(c context.Context) context.Context {
return useImpl(c, nil) }
On 2017/01/07 21:05:26, iannucci wrote:
> if you do the package renaming, this should move out (maybe
> `luci-config/setup/gaeluciconfig` with just the relevant usage bits, the
intent
> being that the setup/* packages would be full instantiations for various
> environments or something.)

I'll move to luci_config/appengine/gaeconfig.

https://codereview.chromium.org/2575383002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/auth.go (right):

https://codereview.chromium.org/2575383002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/auth.go:75: // Artifically handle "all".
On 2017/01/07 21:05:26, iannucci wrote:
> :( (for previous 'all is a valid group name' reasons)

Actually this can die now.

[iannucci, 2017-01-11 02:57:26]

lgtm

[dnj, 2017-01-11 22:56:07]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2017-01-11 22:56:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 22:58:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 22:58:21]

Try jobs failed on following builders:
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/33a890a10b4bfb10)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/33a890a108914210)
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/33a890a076311f10)

[dnj, 2017-01-11 23:04:58]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-01-11 23:04:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2575383002/#ps40001
(title: "Rebarse")

[commit-bot: I haz the power, 2017-01-11 23:05:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dnj, 2017-01-11 23:07:02]

The CQ bit was unchecked by dnj@chromium.org

[dnj, 2017-01-11 23:10:32]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-01-11 23:10:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2575383002/#ps60001
(title: "Un-collapse.")

[commit-bot: I haz the power, 2017-01-11 23:10:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 23:21:39]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484176232805480,
"parent_rev": "0e1e01b5fd687f16f2df2566e3cefc96097ce3ae", "commit_rev":
"45e9a86d32dabca5671d0c1773e124dbc2ec8b5c"}

[commit-bot: I haz the power, 2017-01-11 23:21:42]

Description was changed from

==========
Add server/cache support to gaeconfig.

Rewrite "gaeconfig" and apps that use it to use "server/cache" instead.

Because this fully replaces usage of common/config/caching and
common/config/impl/erroring, delete those packages. Also, common/config
is no longer used in Contexts, so delete that as well.

BUG=chromium:674378
TEST=local,unit
==========

to

==========
Add server/cache support to gaeconfig.

Rewrite "gaeconfig" and apps that use it to use "server/cache" instead.

Because this fully replaces usage of common/config/caching and
common/config/impl/erroring, delete those packages. Also, common/config
is no longer used in Contexts, so delete that as well.

BUG=chromium:674378
TEST=local,unit

Review-Url: https://codereview.chromium.org/2575383002
Committed:
https://github.com/luci/luci-go/commit/45e9a86d32dabca5671d0c1773e124dbc2ec8b5c
==========

[commit-bot: I haz the power, 2017-01-11 23:21:45]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-go/commit/45e9a86d32dabca5671d0c1773e124dbc2ec8b5c

[nodir, 2017-04-05 21:03:53]

FWIU this made scheduler cron job that updates configs use two caching layers
(process memory, memcache) which has has 10min expiration each

[dnj, 2017-04-05 21:14:05]

On 2017/04/05 21:03:53, nodir wrote:
> FWIU this made scheduler cron job that updates configs use two caching layers
> (process memory, memcache) which has has 10min expiration each

Both expirations should start at the same time.

[Vadim Sh., 2017-04-05 21:15:48]

On 2017/04/05 21:14:05, dnj wrote:
> On 2017/04/05 21:03:53, nodir wrote:
> > FWIU this made scheduler cron job that updates configs use two caching
layers
> > (process memory, memcache) which has has 10min expiration each
> 
> Both expirations should start at the same time.

I was under impression that this
https://screenshot.googleplex.com/tVqL3Ne8G8c.png disables all caches and the
new library is just a pass-through to luci-config API client? (That's what I
need in luci-scheduler crons: a way to fetch most recent configs from
luci-config, without any kind of caching).

[dnj, 2017-04-05 21:30:41]

On 2017/04/05 21:15:48, Vadim Sh. wrote:
> On 2017/04/05 21:14:05, dnj wrote:
> > On 2017/04/05 21:03:53, nodir wrote:
> > > FWIU this made scheduler cron job that updates configs use two caching
> layers
> > > (process memory, memcache) which has has 10min expiration each
> > 
> > Both expirations should start at the same time.
> 
> I was under impression that this
> https://screenshot.googleplex.com/tVqL3Ne8G8c.png disables all caches and the
> new library is just a pass-through to luci-config API client? (That's what I
> need in luci-scheduler crons: a way to fetch most recent configs from
> luci-config, without any kind of caching).

Um ... old bug discussion is a little odd, but w/e. Your screenshot does what
you said, I believe.

That said, it's really dangerous to do that, since it might cause your service
to slam luci-config. It'd be better if we just had an "INVALIDATE ALL TEH
CACHES!" button that we could hit, but that doesn't exist ATM. I also think that
when you re-enable caching in the UI, the old caches will kick back in.

[Vadim Sh., 2017-04-05 21:33:12]

On 2017/04/05 21:30:41, dnj wrote:
> On 2017/04/05 21:15:48, Vadim Sh. wrote:
> > On 2017/04/05 21:14:05, dnj wrote:
> > > On 2017/04/05 21:03:53, nodir wrote:
> > > > FWIU this made scheduler cron job that updates configs use two caching
> > layers
> > > > (process memory, memcache) which has has 10min expiration each
> > > 
> > > Both expirations should start at the same time.
> > 
> > I was under impression that this
> > https://screenshot.googleplex.com/tVqL3Ne8G8c.png disables all caches and
the
> > new library is just a pass-through to luci-config API client? (That's what I
> > need in luci-scheduler crons: a way to fetch most recent configs from
> > luci-config, without any kind of caching).
> 
> Um ... old bug discussion is a little odd, but w/e. Your screenshot does what
> you said, I believe.
> 
> That said, it's really dangerous to do that, since it might cause your service
> to slam luci-config. It'd be better if we just had an "INVALIDATE ALL TEH
> CACHES!" button that we could hit, but that doesn't exist ATM. I also think
that
> when you re-enable caching in the UI, the old caches will kick back in.

My service contacts luci-config from a single cron job, once a minute. It can't
slam luci-config. 
It uses cron job because it needs to react to config _changes_ (like, start a
new scheduler job, or disable old scheduler job).

[iannucci, 2017-04-05 21:35:48]

I suspect that this is all hacking around the lack of pubsub pushes from
luci-config.

On Wed, Apr 5, 2017 at 2:33 PM <vadimsh@chromium.org> wrote:

> On 2017/04/05 21:30:41, dnj wrote:
> > On 2017/04/05 21:15:48, Vadim Sh. wrote:
> > > On 2017/04/05 21:14:05, dnj wrote:
> > > > On 2017/04/05 21:03:53, nodir wrote:
> > > > > FWIU this made scheduler cron job that updates configs use two
> caching
> > > layers
> > > > > (process memory, memcache) which has has 10min expiration each
> > > >
> > > > Both expirations should start at the same time.
> > >
> > > I was under impression that this
> > > https://screenshot.googleplex.com/tVqL3Ne8G8c.png disables all caches
> and
> the
> > > new library is just a pass-through to luci-config API client? (That's
> what I
> > > need in luci-scheduler crons: a way to fetch most recent configs from
> > > luci-config, without any kind of caching).
> >
> > Um ... old bug discussion is a little odd, but w/e. Your screenshot does
> what
> > you said, I believe.
> >
> > That said, it's really dangerous to do that, since it might cause your
> service
> > to slam luci-config. It'd be better if we just had an "INVALIDATE ALL TEH
> > CACHES!" button that we could hit, but that doesn't exist ATM. I also
> think
> that
> > when you re-enable caching in the UI, the old caches will kick back in.
>
> My service contacts luci-config from a single cron job, once a minute. It
> can't
> slam luci-config.
> It uses cron job because it needs to react to config _changes_ (like,
> start a
> new scheduler job, or disable old scheduler job).
>
> https://codereview.chromium.org/2575383002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nodir, 2017-04-06 00:57:13]

On 2017/04/05 21:14:05, dnj wrote:
> On 2017/04/05 21:03:53, nodir wrote:
> > FWIU this made scheduler cron job that updates configs use two caching
layers
> > (process memory, memcache) which has has 10min expiration each
> 
> Both expirations should start at the same time.

If memcache expires the moment process memory cache expires, what's the point of
memcaching?..

[dnj, 2017-04-06 01:03:44]

On 2017/04/06 00:57:13, nodir wrote:
> On 2017/04/05 21:14:05, dnj wrote:
> > On 2017/04/05 21:03:53, nodir wrote:
> > > FWIU this made scheduler cron job that updates configs use two caching
> layers
> > > (process memory, memcache) which has has 10min expiration each
> > 
> > Both expirations should start at the same time.
> 
> If memcache expires the moment process memory cache expires, what's the point
of
> memcaching?..

New processes spin up and shut down all the time.