server/config: Add text protobuf support.

server/config: Add text protobuf support.

Add a text protobuf configuration Resolver. This will load text
protobufs into their proto.Message targets. It will also expose a binary
version of the protobufs for caching, enabling the protobufs to be
loaded from cache as binaries.

Loading from binaries is particularly useful because binaries ignore
unknown tags, whereas text protobufs error on them. This reduces risk of
application rollbacks when loading cached configurations.

BUG=chromium:674378
TEST=unit

Review-Url: https://codereview.chromium.org/2578893002
Committed: https://github.com/luci/luci-go/commit/aae008db5bab0425635902546ca339bda0ec7f5c

[dnj, 2016-12-15 05:10:21]

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

[dnj, 2016-12-15 05:10:22]

PTAL. Full stack is:
https://codereview.chromium.org/2580713002 (base)
https://codereview.chromium.org/2578893002 <-- this
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 (gaeconfig)
https://codereview.chromium.org/2576923003 (datastore cache)

[iannucci, 2017-01-07 20:22:11]

lgtm

https://codereview.chromium.org/2578893002/diff/20001/server/config/textproto...
File server/config/textproto/resolver.go (right):

https://codereview.chromium.org/2578893002/diff/20001/server/config/textproto...
server/config/textproto/resolver.go:5: // Package textproto implements a
textproto config service Resolver.
I would mention that it actually stores the binary proto. Something like:

"This takes config files in textproto format, and caches them in binary proto
format."

https://codereview.chromium.org/2578893002/diff/20001/server/config/textproto...
server/config/textproto/resolver.go:32: var typeOfProtoMessage =
reflect.TypeOf((*proto.Message)(nil)).Elem()
really dumb that the proto package doesn't have this as an exported var.

https://codereview.chromium.org/2578893002/diff/20001/server/config/textproto...
server/config/textproto/resolver.go:34: // BinaryFormat is the resolver's binary
protobuf format string.
I would make sure that Format is documented so that it suggests this format. In
fact, if Format is a type, maybe it could be `struct { package, subformat string
}` ?

[dnj, 2017-01-10 03:26:37]

Updated, PTAL.

https://codereview.chromium.org/2578893002/diff/20001/server/config/textproto...
File server/config/textproto/resolver.go (right):

https://codereview.chromium.org/2578893002/diff/20001/server/config/textproto...
server/config/textproto/resolver.go:5: // Package textproto implements a
textproto config service Resolver.
On 2017/01/07 20:22:11, iannucci wrote:
> I would mention that it actually stores the binary proto. Something like:
> 
> "This takes config files in textproto format, and caches them in binary proto
> format."

Done.

https://codereview.chromium.org/2578893002/diff/20001/server/config/textproto...
server/config/textproto/resolver.go:32: var typeOfProtoMessage =
reflect.TypeOf((*proto.Message)(nil)).Elem()
On 2017/01/07 20:22:11, iannucci wrote:
> really dumb that the proto package doesn't have this as an exported var.

Acknowledged.

https://codereview.chromium.org/2578893002/diff/20001/server/config/textproto...
server/config/textproto/resolver.go:34: // BinaryFormat is the resolver's binary
protobuf format string.
On 2017/01/07 20:22:11, iannucci wrote:
> I would make sure that Format is documented so that it suggests this format.
In
> fact, if Format is a type, maybe it could be `struct { package, subformat
string
> }` ?

The "subformat" part is variable, dependent on the message type, so this can't
be constant. But otherwise yes.

[dnj, 2017-01-10 23:08:01]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-01-10 23:08:01]

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/2578893002/#ps40001
(title: "Rebase, comments.")

[commit-bot: I haz the power, 2017-01-10 23:08:09]

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-10 23:15:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484089681219140,
"parent_rev": "0e70e04e28394c34fad2ec3ba85ff402074f429e", "commit_rev":
"aae008db5bab0425635902546ca339bda0ec7f5c"}

[commit-bot: I haz the power, 2017-01-10 23:15:24]

Description was changed from

==========
server/config: Add text protobuf support.

Add a text protobuf configuration Resolver. This will load text
protobufs into their proto.Message targets. It will also expose a binary
version of the protobufs for caching, enabling the protobufs to be
loaded from cache as binaries.

Loading from binaries is particularly useful because binaries ignore
unknown tags, whereas text protobufs error on them. This reduces risk of
application rollbacks when loading cached configurations.

BUG=chromium:674378
TEST=unit
==========

to

==========
server/config: Add text protobuf support.

Add a text protobuf configuration Resolver. This will load text
protobufs into their proto.Message targets. It will also expose a binary
version of the protobufs for caching, enabling the protobufs to be
loaded from cache as binaries.

Loading from binaries is particularly useful because binaries ignore
unknown tags, whereas text protobufs error on them. This reduces risk of
application rollbacks when loading cached configurations.

BUG=chromium:674378
TEST=unit

Review-Url: https://codereview.chromium.org/2578893002
Committed:
https://github.com/luci/luci-go/commit/aae008db5bab0425635902546ca339bda0ec7f5c
==========

[commit-bot: I haz the power, 2017-01-10 23:15:25]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/aae008db5bab0425635902546ca339bda0ec7f5c