code review 6454064: misc/dashboard/codereview: add LastUpdate field to CL

misc/dashboard/codereview: add LastUpdate field to CL

To be filled in by a later CL. I deployed a test version to
App Engine to work on setting the values, so there are
a few records that have this field set already.

That field is breaking the live version, so I have pushed
a new copy with this 1-line change to the live version

I assumed that appengine/datastore was like every other
marshaling and unmarshaling package we have in Go
(for example, encoding/gob, encoding/json, encoding/xml,
and protobuf) and that if it loaded an unknown field it would
just ignore it. Apparently not. Sorry.

[rsc, 2012-07-30 14:13:49]

Hello dsymonds, dsymonds@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2012-07-30 14:14:01]

*** Submitted as http://code.google.com/p/go/source/detail?r=441d5f62e9ff ***

misc/dashboard/codereview: add LastUpdate field to CL

To be filled in by a later CL. I deployed a test version to
App Engine to work on setting the values, so there are
a few records that have this field set already.

That field is breaking the live version, so I have pushed
a new copy with this 1-line change to the live version

I assumed that appengine/datastore was like every other
marshaling and unmarshaling package we have in Go
(for example, encoding/gob, encoding/json, encoding/xml,
and protobuf) and that if it loaded an unknown field it would
just ignore it. Apparently not. Sorry.

R=dsymonds
TBR=dsymonds
CC=golang-dev
http://codereview.appspot.com/6454064

[dsymonds, 2012-07-30 22:51:25]

LGTM

I wish I had seen this before you deployed, as "LastUpdate" sounds
like a timestamp, not the LastReviewMessage or something that it is
actually used as. It's not worth changing now, but please add a
clarifying comment to it. (I'll mention this on the other CL.)

C'est la vie.

[rsc, 2012-07-30 22:54:27]

> I wish I had seen this before you deployed, as "LastUpdate" sounds
> like a timestamp, not the LastReviewMessage or something that it is
> actually used as. It's not worth changing now, but please add a
> clarifying comment to it. (I'll mention this on the other CL.)

If appengine/data behaved like all the other Go serialization
packages, we could just rename the field and ignore the few records
with a LastUpdate. Why doesn't it behave like that?

Russ

[dsymonds, 2012-07-30 22:59:19]

On Tue, Jul 31, 2012 at 8:54 AM, Russ Cox <rsc@golang.org> wrote:

> If appengine/data behaved like all the other Go serialization
> packages, we could just rename the field and ignore the few records
> with a LastUpdate. Why doesn't it behave like that?

That was a design decision made ages ago, but I still think it's the
right one. The main difference is that datastore entities are commonly
read, modified and rewritten. If you got an unknown field and just
accepted it by default, you would risk losing data. Protocol buffers
have XXX_unrecognized for this reason. json/xml/whatever don't, which
consequently makes them unsafe for read-modify-write, but that's not
how they are commonly used.


Dave.

[rsc, 2012-07-30 23:01:54]

> That was a design decision made ages ago, but I still think it's the
> right one. The main difference is that datastore entities are commonly
> read, modified and rewritten. If you got an unknown field and just
> accepted it by default, you would risk losing data. Protocol buffers
> have XXX_unrecognized for this reason. json/xml/whatever don't, which
> consequently makes them unsafe for read-modify-write, but that's not
> how they are commonly used.

But when you rewrite it, you wouldn't rewrite that field (since you
don't know about it) so what's the harm?

Russ

[dsymonds, 2012-07-30 23:10:39]

On Tue, Jul 31, 2012 at 9:01 AM, Russ Cox <rsc@golang.org> wrote:

> But when you rewrite it, you wouldn't rewrite that field (since you
> don't know about it) so what's the harm?

The unit of datastore storage is the entity, not the field, so that
data would be lost.