Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(308)

Issue 2302743002: Interface update, per-method Contexts. (Closed)

Created:
4 years, 3 months ago by dnj
Modified:
4 years, 3 months ago
Reviewers:
iannucci
CC:
chromium-reviews, infra-reviews+luci-gae_chromium.org
Target Ref:
refs/heads/master
Project:
luci-gae
Visibility:
Public.

Description

Interface update, per-method Contexts. This is a very large patch that fixes #53. It carries some additional changes. This is an API change, and will break virtually all users of any "luci/gae" packages. The API update is trivial, though. This patch: - Remove user-facing "Interface" interfaces, when present, in favor of package-level methods that each individually accept a Context parameter. - Remove package-level "Get" methods for services in favor of directly using package-level methods. - Standardize all service/... packages' primary interfaces as "RawInterface" instead of "Interface". - Add a datastore Transaction tracking interface, allowing a user to get or set the current datastore transaction in the Context. This replaces GetNoTxn-style methods in favor of setting a "nil" transaction which, as it happens, is the only implemented part of the interface. - Introduce "KeyContext" to "services/datastore", which retains an AppID and Namespace tuple and uses them as a base to generate GAE keys. Rewrite several internal methods to use KeyContext. - Rename "Testable" interface method to "GetTestable" since it's top-level now and it would conflict with the actual interface definition. - Remove the boolean return value from the info service's "GetNamespace" now that the empty string equals the default namespace. - Change *Multi "service/taskqueue" and "service/memcache" APIs to use singular vararg versions. The transaction interface was added here (as opposed to another patch set) because it is complementary to the API changes required in datastore and taskqueue services to use Contexts directly. This is somewhat at odds with the *NoTxn-style function calls, which have consequently been removed in favor of setting a "nil" Transaction. The datastore Transaction interface is something that has been desired for a while, so supporting nil Transactions should be a good starting point without over-committing (e.g., actually hacking transaction manipulation into "impl/prod"). Migrating code to the new-style API should be fairly straightforward: - Calls to "service/*" methods will no longer call "Get" to get an Interface instance; instead, use package-level calls and pass the Context as an extra parameter. BEFORE: ds := datastore.Get(c) if err := ds.Get(obj0, obj1, ...); err != nil { ... } NOW: if err := datastore.Get(c, obj0, obj1, ...); err != nil { ... } - The "*NoTxn"-style calls are gone, replaced by installing a nil datastore Transaction. BEFORE: datastore.GetNoTxn(c).Put(obj0) taskqueue.GetNoTxn(c).Add(task) prod.AEContextNoTxn(c) NOW: datastore.Put(datastore.WithoutTransaction(c), obj0) taskqueue.Add(datastore.WithoutTransaction(c), task) prod.AEContext(datastore.WithoutTransaction(c)) - Methods generating full datastore keys (manually specifying app ID and namespace) now must use a KeyContext instance. This isn't expected to be a problem, since most external packages should not be using direct key generation, but should, instead, be using the datastore-bound versions of those methods. BEFORE: key := datastore.MakeKey("aid", "ns", "kind", "stringID") NOW: key := datastore.KeyContext{"aid", "ns"}.MakeKey("kind", "stringID") - Methods using *Multi versions of public interfaces should use singular versions. Additonally, to support this, the queue name and task list parameter positions have been swapped. If the error return value is being examined for anything more than non-nil-ness, keep an eye out for the single-argument case, which returns the error directly instead of wrapping it in a MultiError. BEFORE: taskqueue.Get(c).AddMulti([]*taskqueue.Task{t0, t1, t2}, "queueName") NOW: taskqueue.Add(c, "queueName", t0, t1, t2) BUG=None TEST=local - All unit tests pass locally. - Ran "impl/cloud" tests successfully against emulator. - Ran `goapp test` on "impl/prod" successfully. patch from issue 2302743002 at patchset 1 (http://crrev.com/2302743002#ps1) Committed: https://github.com/luci/gae/commit/3715a27a534b6e3896e23662e229fa8caaeb8a60

Patch Set 1 #

Total comments: 60

Patch Set 2 : WithoutTransaction, comments, fixes, cleanup. #

Total comments: 4

Patch Set 3 : Better comments, identity WithoutTransaction. #

Patch Set 4 : Rebase error thing. #

Patch Set 5 : Rebase w/ multi change. #

Patch Set 6 : Lightning talk licenses. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3508 lines, -3393 lines) Patch
M doc.go View 1 4 chunks +2 lines, -21 lines 0 comments Download
M docs/present/lightning/dummy.go View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M docs/present/lightning/gae_test.go View 1 2 3 4 5 2 chunks +6 lines, -3 lines 0 comments Download
M docs/present/lightning/native_test.go View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M filter/count/count_test.go View 1 2 3 4 10 chunks +28 lines, -39 lines 0 comments Download
M filter/count/gi.go View 3 chunks +7 lines, -6 lines 0 comments Download
M filter/count/mail.go View 2 chunks +5 lines, -5 lines 0 comments Download
M filter/count/mod.go View 2 chunks +3 lines, -3 lines 0 comments Download
M filter/count/rds.go View 1 1 chunk +9 lines, -2 lines 0 comments Download
M filter/count/tq.go View 1 chunk +2 lines, -2 lines 0 comments Download
M filter/count/user.go View 2 chunks +5 lines, -5 lines 0 comments Download
M filter/dsQueryBatch/filter_test.go View 3 chunks +4 lines, -4 lines 0 comments Download
M filter/dscache/context.go View 2 chunks +8 lines, -10 lines 0 comments Download
M filter/dscache/ds.go View 4 chunks +10 lines, -8 lines 0 comments Download
M filter/dscache/ds_txn_state.go View 4 chunks +7 lines, -6 lines 0 comments Download
M filter/dscache/dscache_test.go View 1 2 3 4 15 chunks +95 lines, -101 lines 0 comments Download
M filter/dscache/globalconfig.go View 4 chunks +11 lines, -10 lines 0 comments Download
M filter/dscache/serialize.go View 1 chunk +1 line, -1 line 0 comments Download
M filter/dscache/support.go View 5 chunks +10 lines, -11 lines 0 comments Download
M filter/featureBreaker/featurebreaker_test.go View 1 2 3 4 2 chunks +13 lines, -13 lines 0 comments Download
M filter/featureBreaker/gi.go View 1 1 chunk +2 lines, -2 lines 0 comments Download
M filter/featureBreaker/mail.go View 1 chunk +5 lines, -5 lines 0 comments Download
M filter/featureBreaker/mod.go View 5 chunks +9 lines, -9 lines 0 comments Download
M filter/featureBreaker/rds.go View 1 1 chunk +10 lines, -2 lines 0 comments Download
M filter/featureBreaker/tq.go View 1 chunk +2 lines, -2 lines 0 comments Download
M filter/featureBreaker/user.go View 6 chunks +8 lines, -8 lines 0 comments Download
M filter/txnBuf/context.go View 1 chunk +0 lines, -14 lines 0 comments Download
M filter/txnBuf/doc.go View 1 1 chunk +6 lines, -0 lines 0 comments Download
M filter/txnBuf/ds_txn.go View 1 1 chunk +11 lines, -2 lines 0 comments Download
M filter/txnBuf/race_test.go View 3 chunks +5 lines, -10 lines 0 comments Download
M filter/txnBuf/state.go View 4 chunks +5 lines, -10 lines 0 comments Download
M filter/txnBuf/txnbuf_test.go View 1 2 3 4 26 chunks +195 lines, -242 lines 0 comments Download
M impl/cloud/datastore.go View 1 2 3 4 12 chunks +42 lines, -26 lines 0 comments Download
M impl/cloud/datastore_test.go View 1 2 3 4 7 chunks +50 lines, -43 lines 0 comments Download
M impl/cloud/info.go View 3 chunks +29 lines, -28 lines 0 comments Download
A impl/cloud/info_test.go View 1 chunk +55 lines, -0 lines 0 comments Download
M impl/dummy/dummy.go View 1 8 chunks +50 lines, -38 lines 0 comments Download
M impl/dummy/dummy_test.go View 1 chunk +18 lines, -18 lines 0 comments Download
M impl/memory/context.go View 1 6 chunks +15 lines, -14 lines 0 comments Download
M impl/memory/datastore.go View 1 7 chunks +45 lines, -49 lines 0 comments Download
M impl/memory/datastore_data.go View 1 2 3 4 10 chunks +20 lines, -20 lines 0 comments Download
M impl/memory/datastore_index.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M impl/memory/datastore_index_selection.go View 4 chunks +4 lines, -5 lines 0 comments Download
M impl/memory/datastore_query.go View 2 chunks +3 lines, -4 lines 0 comments Download
M impl/memory/datastore_query_execution.go View 1 2 3 4 11 chunks +19 lines, -17 lines 0 comments Download
M impl/memory/datastore_query_execution_test.go View 12 chunks +38 lines, -44 lines 0 comments Download
M impl/memory/datastore_query_test.go View 5 chunks +10 lines, -5 lines 0 comments Download
M impl/memory/datastore_test.go View 1 2 3 4 26 chunks +225 lines, -194 lines 0 comments Download
M impl/memory/info.go View 5 chunks +9 lines, -20 lines 0 comments Download
M impl/memory/info_test.go View 1 chunk +7 lines, -7 lines 0 comments Download
M impl/memory/mail.go View 3 chunks +9 lines, -12 lines 0 comments Download
M impl/memory/mail_test.go View 11 chunks +40 lines, -40 lines 0 comments Download
M impl/memory/memcache.go View 2 chunks +5 lines, -3 lines 0 comments Download
M impl/memory/memcache_test.go View 7 chunks +52 lines, -51 lines 0 comments Download
M impl/memory/module.go View 2 chunks +6 lines, -11 lines 0 comments Download
M impl/memory/module_test.go View 1 chunk +4 lines, -5 lines 0 comments Download
M impl/memory/race_test.go View 1 2 3 4 7 chunks +13 lines, -15 lines 0 comments Download
M impl/memory/taskqueue.go View 9 chunks +13 lines, -25 lines 0 comments Download
M impl/memory/taskqueue_data.go View 3 chunks +4 lines, -2 lines 0 comments Download
M impl/memory/taskqueue_test.go View 1 16 chunks +91 lines, -94 lines 0 comments Download
M impl/memory/testing_utils_test.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
A impl/memory/transaction.go View 1 chunk +44 lines, -0 lines 0 comments Download
M impl/memory/user.go View 2 chunks +5 lines, -7 lines 0 comments Download
M impl/memory/user_test.go View 3 chunks +28 lines, -27 lines 0 comments Download
M impl/prod/context.go View 1 2 6 chunks +56 lines, -37 lines 0 comments Download
M impl/prod/datastore_key.go View 2 chunks +2 lines, -3 lines 0 comments Download
M impl/prod/everything_test.go View 1 2 3 4 7 chunks +74 lines, -77 lines 0 comments Download
M impl/prod/info.go View 1 3 chunks +35 lines, -23 lines 0 comments Download
M impl/prod/mail.go View 2 chunks +4 lines, -5 lines 0 comments Download
M impl/prod/module.go View 1 chunk +3 lines, -4 lines 0 comments Download
M impl/prod/raw_datastore.go View 1 10 chunks +49 lines, -22 lines 0 comments Download
M impl/prod/raw_datastore_type_converter.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M impl/prod/taskqueue.go View 2 chunks +3 lines, -6 lines 0 comments Download
M impl/prod/user.go View 2 chunks +4 lines, -3 lines 0 comments Download
M service/datastore/checkfilter.go View 1 5 chunks +10 lines, -9 lines 0 comments Download
M service/datastore/checkfilter_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M service/datastore/context.go View 1 6 chunks +19 lines, -45 lines 0 comments Download
M service/datastore/context_test.go View 3 chunks +13 lines, -8 lines 0 comments Download
M service/datastore/datastore.go View 1 2 3 2 chunks +0 lines, -537 lines 0 comments Download
M service/datastore/datastore_test.go View 1 2 3 4 69 chunks +180 lines, -270 lines 0 comments Download
M service/datastore/dumper/dumper.go View 1 2 3 4 7 chunks +13 lines, -14 lines 0 comments Download
M service/datastore/dumper/dumper_example_test.go View 1 2 3 4 2 chunks +10 lines, -10 lines 0 comments Download
M service/datastore/finalized_query.go View 2 chunks +6 lines, -6 lines 0 comments Download
M service/datastore/interface.go View 1 2 3 1 chunk +683 lines, -306 lines 0 comments Download
M service/datastore/key.go View 19 chunks +77 lines, -56 lines 0 comments Download
M service/datastore/key_test.go View 6 chunks +54 lines, -43 lines 0 comments Download
M service/datastore/meta/eg.go View 3 chunks +8 lines, -8 lines 0 comments Download
M service/datastore/meta/eg_test.go View 1 2 3 4 2 chunks +12 lines, -11 lines 0 comments Download
M service/datastore/meta/namespaces.go View 3 chunks +5 lines, -4 lines 0 comments Download
M service/datastore/meta/namespaces_test.go View 2 chunks +10 lines, -9 lines 0 comments Download
M service/datastore/multiarg.go View 1 2 3 4 5 chunks +7 lines, -7 lines 0 comments Download
M service/datastore/properties_test.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M service/datastore/query_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M service/datastore/raw_interface.go View 1 2 1 chunk +12 lines, -3 lines 0 comments Download
M service/datastore/serialize/serialize.go View 1 2 3 4 8 chunks +14 lines, -15 lines 0 comments Download
M service/datastore/serialize/serialize_test.go View 1 2 3 4 13 chunks +30 lines, -27 lines 0 comments Download
M service/datastore/size_test.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
A service/datastore/transaction.go View 1 2 1 chunk +40 lines, -0 lines 0 comments Download
M service/info/context.go View 2 chunks +3 lines, -3 lines 0 comments Download
M service/info/interface.go View 4 chunks +132 lines, -23 lines 0 comments Download
A service/info/support/namespace.go View 1 chunk +24 lines, -0 lines 0 comments Download
D service/info/wrapper.go View 1 chunk +0 lines, -30 lines 0 comments Download
M service/mail/context.go View 4 chunks +10 lines, -10 lines 0 comments Download
M service/mail/interface.go View 1 chunk +23 lines, -3 lines 0 comments Download
M service/memcache/context.go View 1 chunk +2 lines, -2 lines 0 comments Download
M service/memcache/interface.go View 1 chunk +172 lines, -67 lines 0 comments Download
D service/memcache/memcache.go View 1 chunk +0 lines, -130 lines 0 comments Download
M service/memcache/types.go View 1 chunk +1 line, -1 line 0 comments Download
M service/module/context.go View 3 chunks +11 lines, -11 lines 0 comments Download
M service/module/interface.go View 2 chunks +55 lines, -2 lines 0 comments Download
M service/taskqueue/context.go View 4 chunks +10 lines, -29 lines 0 comments Download
M service/taskqueue/interface.go View 1 chunk +105 lines, -34 lines 0 comments Download
M service/taskqueue/raw_interface.go View 1 chunk +1 line, -1 line 0 comments Download
D service/taskqueue/taskqueue.go View 1 chunk +0 lines, -77 lines 0 comments Download
M service/user/context.go View 4 chunks +10 lines, -10 lines 0 comments Download
M service/user/interface.go View 2 chunks +62 lines, -3 lines 0 comments Download

Messages

Total messages: 27 (12 generated)
dnj
This is more of a nice comfort food patch than something urgent. However, with "luci/gae" ...
4 years, 3 months ago (2016-09-01 15:25:41 UTC) #6
viperphish
Highlevel comment on CL description: key := datastore.KeyContext{"aid", "ns"}.MakeKey("kind", "stringID") Can we make KeyContext a ...
4 years, 3 months ago (2016-09-01 16:16:20 UTC) #7
dnj
On 2016/09/01 16:16:20, viperphish wrote: > Highlevel comment on CL description: > > key := ...
4 years, 3 months ago (2016-09-01 16:17:44 UTC) #8
dnj
On 2016/09/01 16:17:44, dnj wrote: > On 2016/09/01 16:16:20, viperphish wrote: > > Highlevel comment ...
4 years, 3 months ago (2016-09-01 16:23:18 UTC) #9
iannucci
On 2016/09/01 at 16:23:18, dnj wrote: > On 2016/09/01 16:17:44, dnj wrote: > > On ...
4 years, 3 months ago (2016-09-03 00:49:36 UTC) #10
dnj
On 2016/09/03 00:49:36, iannucci wrote: > On 2016/09/01 at 16:23:18, dnj wrote: > > On ...
4 years, 3 months ago (2016-09-03 00:53:52 UTC) #11
iannucci
lgtm. Do you think we should use caching contexts when we derive new ones? It ...
4 years, 3 months ago (2016-09-16 01:01:14 UTC) #12
dnj
PS2 was "how may wrong ways can I spell 'transaction'?" https://chromiumcodereview.appspot.com/2302743002/diff/1/doc.go File doc.go (right): https://chromiumcodereview.appspot.com/2302743002/diff/1/doc.go#newcode180 ...
4 years, 3 months ago (2016-09-16 05:44:43 UTC) #13
dnj
Oh WRT context caching: I think it is probably still a microoptimization. The CPU's caches ...
4 years, 3 months ago (2016-09-16 05:48:08 UTC) #14
iannucci
lgtm https://chromiumcodereview.appspot.com/2302743002/diff/20001/impl/prod/context.go File impl/prod/context.go (right): https://chromiumcodereview.appspot.com/2302743002/diff/20001/impl/prod/context.go#newcode172 impl/prod/context.go:172: // cancellation from "c". that's unfortunate :/. I ...
4 years, 3 months ago (2016-09-16 07:39:43 UTC) #15
dnj
I'll plan to land this after the single/multi patch is in and adjusted. https://chromiumcodereview.appspot.com/2302743002/diff/20001/impl/prod/context.go File ...
4 years, 3 months ago (2016-09-16 16:09:21 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2302743002/80001
4 years, 3 months ago (2016-09-18 16:12:20 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: Luci-GAE Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3156e39c8f45f010)
4 years, 3 months ago (2016-09-18 16:22:49 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2302743002/100001
4 years, 3 months ago (2016-09-19 02:12:37 UTC) #25
commit-bot: I haz the power
4 years, 3 months ago (2016-09-19 02:19:09 UTC) #27
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://github.com/luci/gae/commit/3715a27a534b6e3896e23662e229fa8caaeb8a60

Powered by Google App Engine
This is Rietveld 408576698