Update APIs to use new Google cloud paths.

Update APIs to use new Google cloud paths.

Fixes #24.

Because the new cloud APIs also import the Google descriptor protobuf,
we now have a "protobuf" package conflict, with our Descriptor protobuf
conflicting with the one imported by the cloud framework. Since they don't
vendor their protobuf package, we cede our descriptor (re)definition and
rework "common/proto/google/descriptor" to be a set of utility functions
("descutil") that operate on the external protobufs. This involved a few
rewrites, but nothing major or breaking.

BUG=None
TEST=None

Committed: https://github.com/luci/luci-go/commit/fa4e50dfe3dfdbfeb21a2a5af53a8b39d7473e60

[dnj, 2016-08-05 16:36:59]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org, nodir@chromium.org, vadimsh@chromium.org

[dnj, 2016-08-05 16:37:00]

PTAL. This includes a regenerate and some re-plumbing of the descriptor /
discovery code since the new cloud package depends on its own (proto-identical)
version.

Requires:
https://codereview.chromium.org/2220533002

[nodir, 2016-08-05 17:15:39]

lgtm, except the rant about your approach to code complexity

https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
File common/proto/google/descutil/tags.go (right):

https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
common/proto/google/descutil/tags.go:52:
resolveTagsFor(&pb.FileDescriptorProto{}, func(resolve func(string) int) {
but why?

proto tags are actually constants (will never change) and don't require code
(can't have bugs, don't require review, easier to comprehend). with these
changes we have more code and it is more complex and no practical benefits. this
code scales with number of different fields, but strongly doubt we will add a
lot of them

i more concerned about your approach to code complexity in general, rather this
particular piece. it is fun only to write new code, not maintain it; and
maintenance is a much bigger contributor to cost than development. we should try
not to write code unless it is practically justified.

https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
common/proto/google/descutil/tags.go:76: t := reflect.TypeOf(msg).Elem()
the more correct of doing this is to use
https://godoc.org/github.com/golang/protobuf/proto#GetProperties

https://codereview.chromium.org/2219023003/diff/1/logdog/server/cmd/logdog_ar...
File logdog/server/cmd/logdog_archivist/task.go (right):

https://codereview.chromium.org/2219023003/diff/1/logdog/server/cmd/logdog_ar...
logdog/server/cmd/logdog_archivist/task.go:58: func (t *pubSubArchivistTask)
AssertLease(c context.Context) error { return nil }
i think you can delete this method now

[dnj (Google), 2016-08-05 18:21:56]

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

[dnj (Google), 2016-08-05 18:21:57]

https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
File common/proto/google/descutil/tags.go (right):

https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
common/proto/google/descutil/tags.go:52:
resolveTagsFor(&pb.FileDescriptorProto{}, func(resolve func(string) int) {
On 2016/08/05 17:15:38, nodir wrote:
> but why?
> 
> proto tags are actually constants (will never change) and don't require code
> (can't have bugs, don't require review, easier to comprehend). with these
> changes we have more code and it is more complex and no practical benefits.
this
> code scales with number of different fields, but strongly doubt we will add a
> lot of them
> 
> i more concerned about your approach to code complexity in general, rather
this
> particular piece. it is fun only to write new code, not maintain it; and
> maintenance is a much bigger contributor to cost than development. we should
try
> not to write code unless it is practically justified.

Since we no longer host the Protos, I think it is a bad idea to embed their
values. I'm not writing code to have fun. I'm minimizing assumptions between
code that is no longer versioned in tandem with this code.

[nodir, 2016-08-05 18:32:36]

On 2016/08/05 18:21:57, dnj (Google) wrote:
>
https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
> File common/proto/google/descutil/tags.go (right):
> 
>
https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
> common/proto/google/descutil/tags.go:52:
> resolveTagsFor(&pb.FileDescriptorProto{}, func(resolve func(string) int) {
> On 2016/08/05 17:15:38, nodir wrote:
> > but why?
> > 
> > proto tags are actually constants (will never change) and don't require code
> > (can't have bugs, don't require review, easier to comprehend). with these
> > changes we have more code and it is more complex and no practical benefits.
> this
> > code scales with number of different fields, but strongly doubt we will add
a
> > lot of them
> > 
> > i more concerned about your approach to code complexity in general, rather
> this
> > particular piece. it is fun only to write new code, not maintain it; and
> > maintenance is a much bigger contributor to cost than development. we should
> try
> > not to write code unless it is practically justified.
> 
> Since we no longer host the Protos, I think it is a bad idea to embed their
> values. 

I don't buy this. If it is a bad idea to copy tags to our code, why it is not a
bad idea to copy proto files into our repo? in both of cases the source of truth
is not in our repo, but some google repo, but copying immutable things is fine.

before .proto files were removed, we actually had tags in two places copied
(.proto and .go files). Was it bad? sure we can write a tool that reads .proto
files, perhaps compiles to descriptor file, retrieves field tags and generates
tags.go, but it is complex. We should always balance between software complexity
and duplication/non-generality, rather than maximize generality/minimize
duplication without thinking about complexity. There is an unbounded set of
software that is not worth to implement.

> I'm not writing code to have fun. I'm minimizing assumptions between
> code that is no longer versioned in tandem with this code.

[dnj (Google), 2016-08-05 18:37:50]

On 2016/08/05 18:32:36, nodir wrote:
> On 2016/08/05 18:21:57, dnj (Google) wrote:
> >
>
https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
> > File common/proto/google/descutil/tags.go (right):
> > 
> >
>
https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
> > common/proto/google/descutil/tags.go:52:
> > resolveTagsFor(&pb.FileDescriptorProto{}, func(resolve func(string) int) {
> > On 2016/08/05 17:15:38, nodir wrote:
> > > but why?
> > > 
> > > proto tags are actually constants (will never change) and don't require
code
> > > (can't have bugs, don't require review, easier to comprehend). with these
> > > changes we have more code and it is more complex and no practical
benefits.
> > this
> > > code scales with number of different fields, but strongly doubt we will
add
> a
> > > lot of them
> > > 
> > > i more concerned about your approach to code complexity in general, rather
> > this
> > > particular piece. it is fun only to write new code, not maintain it; and
> > > maintenance is a much bigger contributor to cost than development. we
should
> > try
> > > not to write code unless it is practically justified.
> > 
> > Since we no longer host the Protos, I think it is a bad idea to embed their
> > values. 
> 
> I don't buy this. If it is a bad idea to copy tags to our code, why it is not
a
> bad idea to copy proto files into our repo? in both of cases the source of
truth
> is not in our repo, but some google repo, but copying immutable things is
fine.

Because those are the protos that we actually use. If we were interoperating
with other components via those protos, we should rethink the strategy. That
said, timrstamp and duration are probably way more stable than protobuf
internals.

> before .proto files were removed, we actually had tags in two places copied
> (.proto and .go files). Was it bad? sure we can write a tool that reads .proto
> files, perhaps compiles to descriptor file, retrieves field tags and generates
> tags.go, but it is complex. We should always balance between software
complexity
> and duplication/non-generality, rather than maximize generality/minimize
> duplication without thinking about complexity. There is an unbounded set of
> software that is not worth to implement.

Proto and go are fine because they are alongside each other and versioned
together. If they weren't, it would be something to think about.

This code isn't complex, either. It's more complex than hardcoded numbers, but
it's also less magic.

> 
> > I'm not writing code to have fun. I'm minimizing assumptions between
> > code that is no longer versioned in tandem with this code.

[dnj (Google), 2016-08-05 18:51:45]

On 2016/08/05 18:37:50, dnj (Google) wrote:
> On 2016/08/05 18:32:36, nodir wrote:
> > On 2016/08/05 18:21:57, dnj (Google) wrote:
> > >
> >
>
https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
> > > File common/proto/google/descutil/tags.go (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
> > > common/proto/google/descutil/tags.go:52:
> > > resolveTagsFor(&pb.FileDescriptorProto{}, func(resolve func(string) int) {
> > > On 2016/08/05 17:15:38, nodir wrote:
> > > > but why?
> > > > 
> > > > proto tags are actually constants (will never change) and don't require
> code
> > > > (can't have bugs, don't require review, easier to comprehend). with
these
> > > > changes we have more code and it is more complex and no practical
> benefits.
> > > this
> > > > code scales with number of different fields, but strongly doubt we will
> add
> > a
> > > > lot of them
> > > > 
> > > > i more concerned about your approach to code complexity in general,
rather
> > > this
> > > > particular piece. it is fun only to write new code, not maintain it; and
> > > > maintenance is a much bigger contributor to cost than development. we
> should
> > > try
> > > > not to write code unless it is practically justified.
> > > 
> > > Since we no longer host the Protos, I think it is a bad idea to embed
their
> > > values. 
> > 
> > I don't buy this. If it is a bad idea to copy tags to our code, why it is
not
> a
> > bad idea to copy proto files into our repo? in both of cases the source of
> truth
> > is not in our repo, but some google repo, but copying immutable things is
> fine.
> 
> Because those are the protos that we actually use. If we were interoperating
> with other components via those protos, we should rethink the strategy. That
> said, timrstamp and duration are probably way more stable than protobuf
> internals.
> 
> > before .proto files were removed, we actually had tags in two places copied
> > (.proto and .go files). Was it bad? sure we can write a tool that reads
.proto
> > files, perhaps compiles to descriptor file, retrieves field tags and
generates
> > tags.go, but it is complex. We should always balance between software
> complexity
> > and duplication/non-generality, rather than maximize generality/minimize
> > duplication without thinking about complexity. There is an unbounded set of
> > software that is not worth to implement.
> 
> Proto and go are fine because they are alongside each other and versioned
> together. If they weren't, it would be something to think about.
> 
> This code isn't complex, either. It's more complex than hardcoded numbers, but
> it's also less magic.

If you want constants, I have no objection to them as long as we do everything
we can to bind them to reality. In this case, a test suite asserting value
correctness seems like a good way to go about that.

I still don't think complexity is the issue here though, either in code or
philosophy.

[nodir, 2016-08-05 19:10:37]

On 2016/08/05 18:37:50, dnj (Google) wrote:
> On 2016/08/05 18:32:36, nodir wrote:
> > On 2016/08/05 18:21:57, dnj (Google) wrote:
> > >
> >
>
https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
> > > File common/proto/google/descutil/tags.go (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2219023003/diff/1/common/proto/google/descuti...
> > > common/proto/google/descutil/tags.go:52:
> > > resolveTagsFor(&pb.FileDescriptorProto{}, func(resolve func(string) int) {
> > > On 2016/08/05 17:15:38, nodir wrote:
> > > > but why?
> > > > 
> > > > proto tags are actually constants (will never change) and don't require
> code
> > > > (can't have bugs, don't require review, easier to comprehend). with
these
> > > > changes we have more code and it is more complex and no practical
> benefits.
> > > this
> > > > code scales with number of different fields, but strongly doubt we will
> add
> > a
> > > > lot of them
> > > > 
> > > > i more concerned about your approach to code complexity in general,
rather
> > > this
> > > > particular piece. it is fun only to write new code, not maintain it; and
> > > > maintenance is a much bigger contributor to cost than development. we
> should
> > > try
> > > > not to write code unless it is practically justified.
> > > 
> > > Since we no longer host the Protos, I think it is a bad idea to embed
their
> > > values. 
> > 
> > I don't buy this. If it is a bad idea to copy tags to our code, why it is
not
> a
> > bad idea to copy proto files into our repo? in both of cases the source of
> truth
> > is not in our repo, but some google repo, but copying immutable things is
> fine.
> 
> Because those are the protos that we actually use. If we were interoperating
> with other components via those protos, we should rethink the strategy. That
> said, timrstamp and duration are probably way more stable than protobuf
> internals.

not internal. descriptor.proto is public and documented.
protoc has a flag to generate it
changing tags there would break programs that consume protoc output and all
descriptors ever created, including our discovery. Not going to happen

> 
> > before .proto files were removed, we actually had tags in two places copied
> > (.proto and .go files). Was it bad? sure we can write a tool that reads
.proto
> > files, perhaps compiles to descriptor file, retrieves field tags and
generates
> > tags.go, but it is complex. We should always balance between software
> complexity
> > and duplication/non-generality, rather than maximize generality/minimize
> > duplication without thinking about complexity. There is an unbounded set of
> > software that is not worth to implement.
> 
> Proto and go are fine because they are alongside each other and versioned
> together. If they weren't, it would be something to think about.

yes, unless they are constants which is the case.
 
> This code isn't complex, either. 

it is:
- it assumes "protobuf" go field tag. How that is more stable than constants?
- it does reflection, tag parsing and creates unnecessary objects for something
known at compile time. it is close to writing an init() function to compute the
value of math.Pi

> It's more complex than hardcoded numbers, but
> it's also less magic.

field comments explicitly say where these numbers came from, there is no magic
if it is not enough, we can point the reader at .proto file

> 
> > 
> > > I'm not writing code to have fun. I'm minimizing assumptions between
> > > code that is no longer versioned in tandem with this code.

[nodir, 2016-08-05 20:48:50]

landing this because luci-go is broken now and this will fix it

[nodir, 2016-08-05 20:48:56]

The CQ bit was checked by nodir@chromium.org

[commit-bot: I haz the power, 2016-08-05 20:49:03]

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, 2016-08-05 21:06:34]

Description was changed from

==========
Update APIs to use new Google cloud paths.

Fixes #24.

Because the new cloud APIs also import the Google descriptor protobuf,
we now have a "protobuf" package conflict, with our Descriptor protobuf
conflicting with the one imported by the cloud framework. Since they don't
vendor their protobuf package, we cede our descriptor (re)definition and
rework "common/proto/google/descriptor" to be a set of utility functions
("descutil") that operate on the external protobufs. This involved a few
rewrites, but nothing major or breaking.

BUG=None
TEST=None
==========

to

==========
Update APIs to use new Google cloud paths.

Fixes #24.

Because the new cloud APIs also import the Google descriptor protobuf,
we now have a "protobuf" package conflict, with our Descriptor protobuf
conflicting with the one imported by the cloud framework. Since they don't
vendor their protobuf package, we cede our descriptor (re)definition and
rework "common/proto/google/descriptor" to be a set of utility functions
("descutil") that operate on the external protobufs. This involved a few
rewrites, but nothing major or breaking.

BUG=None
TEST=None

Committed:
https://github.com/luci/luci-go/commit/fa4e50dfe3dfdbfeb21a2a5af53a8b39d7473e60
==========

[commit-bot: I haz the power, 2016-08-05 21:06:36]

Committed patchset #1 (id:1) as
https://github.com/luci/luci-go/commit/fa4e50dfe3dfdbfeb21a2a5af53a8b39d7473e60

[dnj, 2016-08-06 05:42:25]

> it is:
> - it assumes "protobuf" go field tag. How that is more stable than constants?

This assumption is a contract between the Go protobuf library and its protobuf
generator adapter which are, minimally, versioned alongside each other. This is
a far stronger contract than the details of any given proto file.

> - it does reflection, tag parsing and creates unnecessary objects for
something
> known at compile time. it is close to writing an init() function to compute
the
> value of math.Pi

All of this complexity is part of the standard library. We don't have to
maintain it, which is your argument against complexity. Unless you think we
shouldn't use maps for things because behind the scenes they use rebalancing
hash tables and race checks? Your problem with complexity is a function of the
maintainability of that complexity, and in this case the maintainability of
these specific functions is very simple.

> > It's more complex than hardcoded numbers, but
> > it's also less magic.
> 
> field comments explicitly say where these numbers came from, there is no magic
> if it is not enough, we can point the reader at .proto file

There's no comment saying, "we chose 4 for this field because...". They picked
mostly sequential numbers. They might change. They might not change. We don't
know what constraints the protobuf package authors are applying to this proto,
under what circumstances they'll break backwards compatibility, and what their
perception of the liberty they have to govern the producers and consumers of the
protobuf. What I know: we are not known to them, so our usage isn't part of
their plan, regardless. I don't think baking unasserted assumptions into the
code based on a presumption of their operating mentality is a good idea.