cipd: Implement local cache for ResolveVersion(...) call with tags.

cipd: Implement local cache for ResolveVersion(...) call with tags.

Tags are not detachable. If ResolveVersion(<tag>) resolved to something, it is
guaranteed to resolve to the same thing later or not to resolve at all (in case
same <tag> was attached to another instance).

Tags that are applied to multiple instances aren't supposed to be used for
version resolution anyway.

So it seems acceptable to cache <tag> -> <instance id> mapping locally to avoid
round trip to the server next time it is used.

In practice same tag reresolution happens a lot. Puppet runs "cipd ensure" each
N minutes, passing cipd "pkg1:tag1 pkg2:tag2 ..." list where tags are mostly
static (compared to the frequency of puppet runs). No need to go to the backend
all the time. Especially considering it might not be available due to temporary
networks hiccups, etc.

I expect this CL will reduce QPS to the backed to ~0 compared to the current
state.

R=nodir@chromium.org, tandrii@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/72d3a5a5d8173d0a353d26ce307b96270355c77f

[Vadim Sh., 2015-09-30 03:50:01]

ptal

tests are coming

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
File go/src/infra/tools/cipd/internal/messages/generate.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/messages/generate.go:13: var _ = proto.Marshal
this is needed to make "proto" package actually used, so that "go get" can
potentially fetch it, so that if go-generated code is not generated yet, proto
is still around.

It's mostly unnecessary in our pinned universe. But I copy-pasted the pattern
from elsewhere (I think dnj@ uses it), and it sort of cute.

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
File go/src/infra/tools/cipd/internal/tagcache.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:166: Package: &pin.PackageName,
pointers to strings annoy me, put that's how golang protobuf work :-/

[tandrii(chromium), 2015-09-30 18:11:15]

inline comments + 2 general question:
1) why protobuf? aka why not readable json for easier debuggability? reading 300
json items isn't that much slower.
2) Why least-recently-resolved cache and not LRU, and if LRU, re-using
https://github.com/luci/luci-go/blob/master/common/lru/lru.go seems like a good
thing (except for cross infra/luci-go references...)

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:301: // doRequest lazy-initializes http.Client
by calling giving callback and then
s/by calling giving callback/using provided factory

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:347: // Called under lock.
nit suggestion: s/.../Must be called under lock.

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:354: //It's tiny in size (and protobuf can't
serialize to io.Reader anyway). Then
nit: space between // and It
nit: s/Then/So,

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:454: // Use local cache when resolving tags to
avoid round trips to backed when
s/backed/backend

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
File go/src/infra/tools/cipd/internal/tagcache.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:23: // multiple instances, in which
case the tag is misused anyway). In any case,
Maybe warn the misuse info to the set-tag and register commands?

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:28: type TagCache interface {
frankly, i don't see a reason to have interface => this is internal package,
with no other implementation, so just a struct should do.
I agree with nodir@ that we do indeed overuse this pattern.

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:107: }
didn't you mean to add:
  c.cache.Entries = compacted
?
And if so, then maybe pre-allocating len(c.cache.Entries) isn't the best
approach. Then again, it's ~300 items...

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:113: // to keep it.
but you don't move "old hot" items to the end, so tail isn't always "hot". not
critical, as just-remove-old-hot will soon become new-hot, so I guess it's OK. 
tl;dr I personally think LRU fits better this, but LRR (last R for resolved) is
also OK :)

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:145: if bad {
since you have comment, why not avoid `bad` var entierly?

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:152: // Try to find an existing
entry in the cache. It will be moved to bottom
this is weird. As a safety check, OK. But current usage of this cache
essentially makes sure this never happens. This is because AddTag is only called
after ResolveTag() finds nothing.

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:155: // compacted in Save().
Hm, holes are compacted in what gets actually written to resulting buffer, but
actual entries stay with holes.

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/loc...
File go/src/infra/tools/cipd/local/fs.go (left):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:84: // Root returns absolute path to a
directory FileSystem operates in. All FS
why is this no longer the case?

[nodir, 2015-09-30 19:08:24]

main offenders: Save does not compact, ResolveTag iterates old entries first

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:337: client.tagCache = internal.NewTagCache()
remove this

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:340: client.tagCache = internal.NewTagCache()
remove this

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:342: }
add 

if client.tagCache == nil {
  client.TagCache = internal.NewTagCache()
}

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
File go/src/infra/tools/cipd/internal/checksum.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/checksum.go:33: envelope :=
messages.BlobWithSHA1{}
I prefer 

  var envelope messages.BlobWithSHA1

but it doesn't matter

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
File go/src/infra/tools/cipd/internal/messages/generate.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/messages/generate.go:13: var _ = proto.Marshal
On 2015/09/30 03:50:00, Vadim Sh. wrote:
> this is needed to make "proto" package actually used, so that "go get" can
> potentially fetch it, so that if go-generated code is not generated yet, proto
> is still around.
> 
> It's mostly unnecessary in our pinned universe. But I copy-pasted the pattern
> from elsewhere (I think dnj@ uses it), and it sort of cute.

Acknowledged.

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
File go/src/infra/tools/cipd/internal/tagcache.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:23: // multiple instances, in which
case the tag is misused anyway). In any case,
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> Maybe warn the misuse info to the set-tag and register commands?

We should prevent this from happening on the server

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:28: type TagCache interface {
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> frankly, i don't see a reason to have interface => this is internal package,
> with no other implementation, so just a struct should do.
> I agree with nodir@ that we do indeed overuse this pattern.

I agree (with tandrii agreeing with me)

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:75: if err :=
UnmarshalWithSHA1(buf, &cache); err != nil {
Why do you have to do it? A message probably won't be decoded if it is corrupted

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:108: c.dirty = false
set it to false if MarshalWithSHA1 didn't error out

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:129: for _, e := range
c.cache.Entries {
shouldn't you iterate from the tail? given freshest entries are there

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:145: if bad {
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> since you have comment, why not avoid `bad` var entierly?

the if statement will probably become scary

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:152: // Try to find an existing
entry in the cache. It will be moved to bottom
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> this is weird. As a safety check, OK. But current usage of this cache
> essentially makes sure this never happens. This is because AddTag is only
called
> after ResolveTag() finds nothing.

it may happen because of a race condition. State may change between ResolveTag
and AddTag

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:155: // compacted in Save().
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> Hm, holes are compacted in what gets actually written to resulting buffer, but
> actual entries stay with holes.

+1

[Vadim Sh., 2015-09-30 22:27:02]

> 1) why protobuf? aka why not readable json for easier debuggability? reading
300 json items isn't that much slower.
This code will start creating files on all our machines (thousands). At some 
point I expect there'll be format changes, and there'll inevitably be bots 
with ancient cipd version that suddenly run newer version and vice versa.

I'd like not to think too much about backward and forward compatibility.
Protobufs makes this explicit 
and controllable.

> 2) Why least-recently-resolved cache and not LRU...

I decided I don't want to write to disk if cache hasn't actually changed. 

Also I think a process of converting an array of stored cache entries into
lru.Cache
and the using it will be much slower compared to just enumerating the cache 
as-stored linearly (considering it's size is < 300 entries and wast majority of
time it 
is used to resolve one or two items).

Also lru.Cache has no public API that allows its content to be stored.

[tandrii_google, 2015-09-30 22:34:22]

On 2015/09/30 22:27:02, Vadim Sh. wrote:
> > 1) why protobuf? aka why not readable json for easier debuggability? reading
> 300 json items isn't that much slower.
> This code will start creating files on all our machines (thousands). At some 
> point I expect there'll be format changes, and there'll inevitably be bots 
> with ancient cipd version that suddenly run newer version and vice versa.
> 
> I'd like not to think too much about backward and forward compatibility.
> Protobufs makes this explicit 
> and controllable.
I see. Well, I'm OK with that,
although my retort would be that any failures can be simple ignored. Hence,
compatability isn't really an issue.
 
> > 2) Why least-recently-resolved cache and not LRU...
> 
> I decided I don't want to write to disk if cache hasn't actually changed. 
Ah, that's reasonable.

> 
> Also I think a process of converting an array of stored cache entries into
> lru.Cache
> and the using it will be much slower compared to just enumerating the cache 
> as-stored linearly (considering it's size is < 300 entries and wast majority
of
> time it 
> is used to resolve one or two items).
True. Particularly given that a single invocation of cipd is not likely to have
thousands of tags to lookup.

> 
> Also lru.Cache has no public API that allows its content to be stored.
Well, that's changeable, but irrelevant give the above :)

[Vadim Sh., 2015-09-30 22:50:59]

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
File go/src/infra/tools/cipd/client.go (right):

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/client.go:301: // doRequest lazy-initializes http.Client
by calling giving callback and then
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> s/by calling giving callback/using provided factory

Done.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/client.go:337: client.tagCache = internal.NewTagCache()
On 2015/09/30 19:08:23, nodir wrote:
> remove this

Done.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/client.go:340: client.tagCache = internal.NewTagCache()
On 2015/09/30 19:08:23, nodir wrote:
> remove this

Done.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/client.go:342: }
On 2015/09/30 19:08:23, nodir wrote:
> add 
> 
> if client.tagCache == nil {
>   client.TagCache = internal.NewTagCache()
> }

Done.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/client.go:347: // Called under lock.
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> nit suggestion: s/.../Must be called under lock.

Done. Whatever.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/client.go:354: //It's tiny in size (and protobuf can't
serialize to io.Reader anyway). Then
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> nit: space between // and It
> nit: s/Then/So,

Done.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/client.go:454: // Use local cache when resolving tags to
avoid round trips to backed when
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> s/backed/backend

Done.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
File go/src/infra/tools/cipd/internal/checksum.go (right):

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/checksum.go:33: envelope :=
messages.BlobWithSHA1{}
On 2015/09/30 19:08:23, nodir wrote:
> I prefer 
> 
>   var envelope messages.BlobWithSHA1
> 
> but it doesn't matter

And I prefer var := type{} :)

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
File go/src/infra/tools/cipd/internal/tagcache.go (right):

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:23: // multiple instances, in which
case the tag is misused anyway). In any case,
On 2015/09/30 19:08:23, nodir wrote:
> On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > Maybe warn the misuse info to the set-tag and register commands?
> 
> We should prevent this from happening on the server

Applying same tag to multiple packages is not forbidden operation. It's
perfectly reasonable in some workflows. Such tags just should not be used to
name versions. (They still can be used as, well.. tags). For example imagine you
have some release pipeline and you want to mark packages that passed (or failed)
canary stage. You can use 'canary_passed:true' or 'canary_passed:false' tags on
them. Then all packages that passed canary can be fetched via search for
'canary_passed:true' tag.

I'm considering making stronger distinction between two kinds of tags (see
https://chromium.googlesource.com/infra/infra/+/master/appengine/chrome_infra...),
but it's not of a very high priority and I prefer to add features only if
absolutely necessary.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:28: type TagCache interface {
On 2015/09/30 19:08:24, nodir wrote:
> On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > frankly, i don't see a reason to have interface => this is internal package,
> > with no other implementation, so just a struct should do.
> > I agree with nodir@ that we do indeed overuse this pattern.
> 
> I agree (with tandrii agreeing with me)

Converted to struct, whatever. I like interfaces because docs are in single
place, and not scattered around the file.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:75: if err :=
UnmarshalWithSHA1(buf, &cache); err != nil {
On 2015/09/30 19:08:23, nodir wrote:
> Why do you have to do it? A message probably won't be decoded if it is
corrupted

This code will be used on thousands of bots. Some of them under stress, with bad
disks, etc. I don't like to rely on "probably" and then debug random shit.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:107: }
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> didn't you mean to add:
>   c.cache.Entries = compacted
> ?
Yes, forgot.

> And if so, then maybe pre-allocating len(c.cache.Entries) isn't the best
> approach. Then again, it's ~300 items...

It should be reasonably small. cipd.Client is short lived currently, it is use
for a small number of operations and the closed.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:108: c.dirty = false
On 2015/09/30 19:08:23, nodir wrote:
> set it to false if MarshalWithSHA1 didn't error out

Done.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:113: // to keep it.
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> but you don't move "old hot" items to the end, so tail isn't always "hot". not
> critical, as just-remove-old-hot will soon become new-hot, so I guess it's OK.

> tl;dr I personally think LRU fits better this, but LRR (last R for resolved)
is
> also OK :)

I decided I don't want to write to disk after each cached ResolveVersion
response. That's why reads from cache do not change it (only AddTag does). It's
conscious decision ;)

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:129: for _, e := range
c.cache.Entries {
On 2015/09/30 19:08:23, nodir wrote:
> shouldn't you iterate from the tail? given freshest entries are there

Done. Whatever, it is <300 items.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:145: if bad {
On 2015/09/30 19:08:23, nodir wrote:
> On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > since you have comment, why not avoid `bad` var entierly?
> 
> the if statement will probably become scary

Yes, 'if' becomes uglier.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:152: // Try to find an existing
entry in the cache. It will be moved to bottom
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> this is weird. As a safety check, OK. But current usage of this cache
> essentially makes sure this never happens. This is because AddTag is only
called
> after ResolveTag() finds nothing.

TagCache just implements some interface, regardless of how this interface is
being used. I don't want to discover some hidden "wtf" few months later when
TagCache is used in some other way.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:155: // compacted in Save().
On 2015/09/30 19:08:23, nodir wrote:
> On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > Hm, holes are compacted in what gets actually written to resulting buffer,
but
> > actual entries stay with holes.
> 
> +1

Done.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
File go/src/infra/tools/cipd/local/fs.go (left):

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/local/fs.go:84: // Root returns absolute path to a
directory FileSystem operates in. All FS
On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> why is this no longer the case?

It is still the case. It was mistakenly copy-pasted chunk of interface
documentation. See line 24.

[tandrii_google, 2015-09-30 23:10:59]

tandrii@google.com changed reviewers:
+ tandrii@google.com

[tandrii_google, 2015-09-30 23:11:00]

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
File go/src/infra/tools/cipd/internal/tagcache.go (right):

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:23: // multiple instances, in which
case the tag is misused anyway). In any case,
On 2015/09/30 22:50:59, Vadim Sh. wrote:
> On 2015/09/30 19:08:23, nodir wrote:
> > On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > > Maybe warn the misuse info to the set-tag and register commands?
> > 
> > We should prevent this from happening on the server
> 
> Applying same tag to multiple packages is not forbidden operation. It's
> perfectly reasonable in some workflows. Such tags just should not be used to
> name versions. (They still can be used as, well.. tags). For example imagine
you
> have some release pipeline and you want to mark packages that passed (or
failed)
> canary stage. You can use 'canary_passed:true' or 'canary_passed:false' tags
on
> them. Then all packages that passed canary can be fetched via search for
> 'canary_passed:true' tag.

FTR, that's exactly what I intended to do with CQ package, actually.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:28: type TagCache interface {
On 2015/09/30 22:50:59, Vadim Sh. wrote:
> On 2015/09/30 19:08:24, nodir wrote:
> > On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > > frankly, i don't see a reason to have interface => this is internal
package,
> > > with no other implementation, so just a struct should do.
> > > I agree with nodir@ that we do indeed overuse this pattern.
> > 
> > I agree (with tandrii agreeing with me)
> 
> Converted to struct, whatever. I like interfaces because docs are in single
> place, and not scattered around the file.
True, but godoc does look worse. So, it's a trade-off.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:113: // to keep it.
On 2015/09/30 22:50:59, Vadim Sh. wrote:
> On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > but you don't move "old hot" items to the end, so tail isn't always "hot".
not
> > critical, as just-remove-old-hot will soon become new-hot, so I guess it's
OK.
> 
> > tl;dr I personally think LRU fits better this, but LRR (last R for resolved)
> is
> > also OK :)
> 
> I decided I don't want to write to disk after each cached ResolveVersion
> response. That's why reads from cache do not change it (only AddTag does).
It's
> conscious decision ;)

Acknowledged.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/internal/tagcache.go:152: // Try to find an existing
entry in the cache. It will be moved to bottom
On 2015/09/30 22:50:59, Vadim Sh. wrote:
> On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > this is weird. As a safety check, OK. But current usage of this cache
> > essentially makes sure this never happens. This is because AddTag is only
> called
> > after ResolveTag() finds nothing.
> 
> TagCache just implements some interface, regardless of how this interface is
> being used. I don't want to discover some hidden "wtf" few months later when
> TagCache is used in some other way.

I meant it more in connection with compaction not actually happening in Save().
Since that was a bug, this is irrelevant.

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
File go/src/infra/tools/cipd/local/fs.go (left):

https://chromiumcodereview.appspot.com/1381583007/diff/1/go/src/infra/tools/c...
go/src/infra/tools/cipd/local/fs.go:84: // Root returns absolute path to a
directory FileSystem operates in. All FS
On 2015/09/30 22:50:59, Vadim Sh. wrote:
> On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > why is this no longer the case?
> 
> It is still the case. It was mistakenly copy-pasted chunk of interface
> documentation. See line 24.

Acknowledged.

[nodir, 2015-10-01 23:24:04]

lgtm

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
File go/src/infra/tools/cipd/internal/tagcache.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:23: // multiple instances, in which
case the tag is misused anyway). In any case,
On 2015/09/30 23:11:00, tandrii_google wrote:
> On 2015/09/30 22:50:59, Vadim Sh. wrote:
> > On 2015/09/30 19:08:23, nodir wrote:
> > > On 2015/09/30 18:11:15, tandrii(chromium) wrote:
> > > > Maybe warn the misuse info to the set-tag and register commands?
> > > 
> > > We should prevent this from happening on the server
> > 
> > Applying same tag to multiple packages is not forbidden operation. It's
> > perfectly reasonable in some workflows. Such tags just should not be used to
> > name versions. (They still can be used as, well.. tags). For example imagine
> you
> > have some release pipeline and you want to mark packages that passed (or
> failed)
> > canary stage. You can use 'canary_passed:true' or 'canary_passed:false' tags
> on
> > them. Then all packages that passed canary can be fetched via search for
> > 'canary_passed:true' tag.
> 
> FTR, that's exactly what I intended to do with CQ package, actually.

Looked up the server code: if there is >1 instance with a tag, server will
respond with AMBIGUOUS_VERSION status [1]. Client handles that. Someone may
abuse non-unique tags for versions, but that won't be subtle: they will quickly
discover that nothing is updated (as long as there is monitoring for cipd
deployment (?))

[1]:
https://chromium.googlesource.com/infra/infra/+/master/appengine/chrome_infra...

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:75: if err :=
UnmarshalWithSHA1(buf, &cache); err != nil {
On 2015/09/30 22:50:59, Vadim Sh. wrote:
> On 2015/09/30 19:08:23, nodir wrote:
> > Why do you have to do it? A message probably won't be decoded if it is
> corrupted
> 
> This code will be used on thousands of bots. Some of them under stress, with
bad
> disks, etc. I don't like to rely on "probably" and then debug random shit.

the word "probably" meant that I don't know for sure and suggested you to check
that (apparently, not a good idea). I am not opposed to sha1 check

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:129: for _, e := range
c.cache.Entries {
On 2015/09/30 22:50:59, Vadim Sh. wrote:
> On 2015/09/30 19:08:23, nodir wrote:
> > shouldn't you iterate from the tail? given freshest entries are there
> 
> Done. Whatever, it is <300 items.

I thought that was the point of putting fresh items in the end.

[Vadim Sh., 2015-10-01 23:34:07]

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
File go/src/infra/tools/cipd/internal/tagcache.go (right):

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:75: if err :=
UnmarshalWithSHA1(buf, &cache); err != nil {
On 2015/10/01 23:24:03, nodir wrote:
> On 2015/09/30 22:50:59, Vadim Sh. wrote:
> > On 2015/09/30 19:08:23, nodir wrote:
> > > Why do you have to do it? A message probably won't be decoded if it is
> > corrupted
> > 
> > This code will be used on thousands of bots. Some of them under stress, with
> bad
> > disks, etc. I don't like to rely on "probably" and then debug random shit.
> 
> the word "probably" meant that I don't know for sure and suggested you to
check
> that (apparently, not a good idea). I am not opposed to sha1 check

I'm sorry if my reply sounded harsh...

Protobufs do not have strict validation, it's very easy to "correctly" decode
corrupted proto message.

https://codereview.chromium.org/1381583007/diff/1/go/src/infra/tools/cipd/int...
go/src/infra/tools/cipd/internal/tagcache.go:129: for _, e := range
c.cache.Entries {
On 2015/10/01 23:24:03, nodir wrote:
> On 2015/09/30 22:50:59, Vadim Sh. wrote:
> > On 2015/09/30 19:08:23, nodir wrote:
> > > shouldn't you iterate from the tail? given freshest entries are there
> > 
> > Done. Whatever, it is <300 items.
> 
> I thought that was the point of putting fresh items in the end.

They are in the end because that way no operation needs to "shift" array when
inserting stuff. Though it is probably an overkill too, considering it's just
300 items...

[Vadim Sh., 2015-10-05 19:45:29]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2015-10-05 19:45:30]

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

[commit-bot: I haz the power, 2015-10-05 19:45:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381583007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381583007/60001

[commit-bot: I haz the power, 2015-10-05 19:48:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/72d3a5a5d8173d0a353d26ce307b9...