Issue 1929423002: filter/txnBuf: Use fully-qualified App ID.

Issue 1929423002: filter/txnBuf: Use fully-qualified App ID. (Closed)

Created:
4 years, 7 months ago by dnj

Modified:
4 years, 7 months ago

Reviewers:
iannucci

CC:
chromium-reviews, infra-reviews+luci-gae_chromium.org

Base URL:
https://chromium.googlesource.com/external/github.com/luci/gae@master

Target Ref:
refs/heads/master

Project:
luci-gae

Visibility:
Public.

More Reviews

Description

filter/txnBuf: Use fully-qualified App ID. Use the fully-qualified app ID when creating the in-memory datastore for transaction buffer. Previously, transaction buffer would use the standard App ID, while production datastore would use the fully-qualified App ID, causing key incompatibility errors in production. Cleanup impl/memory's NewDatastore function to not install superfluous services into the Context. This draws a cleaner line between impl/memory's dual purposes as both a rich testing enviornment and a production-grade in-memory datastore. In addition, have impl/memory's info service differentiate between the two. This causes tests for filter/txnBuf to properly fail lacking this fix, enabling regression protection. To support this, impl/memory's Info service will now parse the supplied App ID. If it contains a "~" character, it is split into fully-qualified and regular AppID. Otherwise, the two will be the same. BUG= Committed: https://github.com/luci/gae/commit/5e312adb9e0e0ce0abaa10d4d94f5bd5e11671e3

Patch Set 1 #

Patch Set 2 : Go back to "dev~", too many things care. #

Total comments: 2

Patch Set 3 : Use public methods for defining Info service in tests. #

Created: 4 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+37 lines, -35 lines)			Patch
M	filter/txnBuf/state.go	View		2 chunks	+4 lines, -9 lines	0 comments	Download
M	impl/memory/context.go	View	1	4 chunks	+12 lines, -4 lines	0 comments	Download
M	impl/memory/datastore.go	View		1 chunk	+11 lines, -12 lines	0 comments	Download
M	impl/memory/datastore_test.go	View	1 2	1 chunk	+4 lines, -3 lines	0 comments	Download
M	impl/memory/info.go	View		2 chunks	+4 lines, -3 lines	0 comments	Download
M	impl/memory/info_test.go	View	1 2	1 chunk	+2 lines, -4 lines	0 comments	Download

Messages

Total messages: 11 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dnj

Description was changed from ========== filter/txnBuf: Use fully-qualified App ID. Use the fully-qualified app ID ...

4 years, 7 months ago (2016-04-29 18:06:48 UTC) #3

Description was changed from

==========
filter/txnBuf: Use fully-qualified App ID.

Use the fully-qualified app ID when creating the in-memory datastore for
transaction buffer. Previously, transaction buffer would use the
standard App ID, while production datastore would use the
fully-qualified App ID, causing key incompatibility errors in
production.

Cleanup impl/memory's NewDatastore function to not install superfluous
services into the Context. This draws a cleaner line between
impl/memory's dual purposes as both a rich testing enviornment and a
production-grade in-memory datastore.

In addition, have impl/memory's info service differentiate between the
two. This causes tests for filter/txnBuf to properly fail lacking this
fix, enabling regression protection.

To support this, impl/memory's Info service will now parse the supplied
App ID. If it contains a "~" character, it is split into fully-qualified
and regular AppID. Otherwise, the two will be the same.

Default to using the "mem~" prefix instaed of the "dev~" prefix to
differentiate from devappserver.

BUG=
==========

to

==========
filter/txnBuf: Use fully-qualified App ID.

Use the fully-qualified app ID when creating the in-memory datastore for
transaction buffer. Previously, transaction buffer would use the
standard App ID, while production datastore would use the
fully-qualified App ID, causing key incompatibility errors in
production.

Cleanup impl/memory's NewDatastore function to not install superfluous
services into the Context. This draws a cleaner line between
impl/memory's dual purposes as both a rich testing enviornment and a
production-grade in-memory datastore.

In addition, have impl/memory's info service differentiate between the
two. This causes tests for filter/txnBuf to properly fail lacking this
fix, enabling regression protection.

To support this, impl/memory's Info service will now parse the supplied
App ID. If it contains a "~" character, it is split into fully-qualified
and regular AppID. Otherwise, the two will be the same.

BUG=
==========

iannucci

https://codereview.chromium.org/1929423002/diff/20001/impl/memory/datastore_test.go File impl/memory/datastore_test.go (right): https://codereview.chromium.org/1929423002/diff/20001/impl/memory/datastore_test.go#newcode692 impl/memory/datastore_test.go:692: c = infoS.Get(c).MustNamespace("ns") I don't think I like that ...

4 years, 7 months ago (2016-04-29 19:02:34 UTC) #4

dnj

4 years, 7 months ago (2016-04-29 19:08:07 UTC) #5

commit-bot: I haz the power

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

4 years, 7 months ago (2016-04-29 19:14:00 UTC) #8

commit-bot: I haz the power

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

4 years, 7 months ago (2016-04-29 19:14:07 UTC) #9

commit-bot: I haz the power

Description was changed from ========== filter/txnBuf: Use fully-qualified App ID. Use the fully-qualified app ID ...

4 years, 7 months ago (2016-04-29 19:19:12 UTC) #10

Message was sent while issue was closed.

Description was changed from

==========
filter/txnBuf: Use fully-qualified App ID.

Use the fully-qualified app ID when creating the in-memory datastore for
transaction buffer. Previously, transaction buffer would use the
standard App ID, while production datastore would use the
fully-qualified App ID, causing key incompatibility errors in
production.

Cleanup impl/memory's NewDatastore function to not install superfluous
services into the Context. This draws a cleaner line between
impl/memory's dual purposes as both a rich testing enviornment and a
production-grade in-memory datastore.

In addition, have impl/memory's info service differentiate between the
two. This causes tests for filter/txnBuf to properly fail lacking this
fix, enabling regression protection.

To support this, impl/memory's Info service will now parse the supplied
App ID. If it contains a "~" character, it is split into fully-qualified
and regular AppID. Otherwise, the two will be the same.

BUG=
==========

to

==========
filter/txnBuf: Use fully-qualified App ID.

Use the fully-qualified app ID when creating the in-memory datastore for
transaction buffer. Previously, transaction buffer would use the
standard App ID, while production datastore would use the
fully-qualified App ID, causing key incompatibility errors in
production.

Cleanup impl/memory's NewDatastore function to not install superfluous
services into the Context. This draws a cleaner line between
impl/memory's dual purposes as both a rich testing enviornment and a
production-grade in-memory datastore.

In addition, have impl/memory's info service differentiate between the
two. This causes tests for filter/txnBuf to properly fail lacking this
fix, enabling regression protection.

To support this, impl/memory's Info service will now parse the supplied
App ID. If it contains a "~" character, it is split into fully-qualified
and regular AppID. Otherwise, the two will be the same.

BUG=

Committed:
https://github.com/luci/gae/commit/5e312adb9e0e0ce0abaa10d4d94f5bd5e11671e3
==========

commit-bot: I haz the power

4 years, 7 months ago (2016-04-29 19:19:13 UTC) #11

Message was sent while issue was closed.

Committed patchset #3 (id:40001) as
https://github.com/luci/gae/commit/5e312adb9e0e0ce0abaa10d4d94f5bd5e11671e3

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages