datastore: variadic Get, Put, Exists, Delete.

datastore: variadic Get, Put, Exists, Delete.

Fixes #51.

Update Get, Put, Exists, and Delete to be variadic. This will make the
interface more usable and get rid of boilerplate in client code.

Get, Put, Exists, and Delete now accept variadic arguments. Any one of
these arguments can be an argument that was previously valid or
the *Multi version of those functions. This effectively obsoletes the *Multi
versions, but they are left in for backwards compatiblility with the stated
intention of removing them at a later date.

Get/Put/Exists/Delete will now return a single error in the one-argument
case or a MultiError in a multiple-argument case. Note that now that single
argument case can take a slice type, the single error may itself be a
MultiError if the single argument is a slice.

Committed: https://github.com/luci/gae/commit/ee5266ebb4c1957a848000175f03ddb6845d01ed

[dnj, 2016-05-25 05:26:33]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-25 05:26:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2011773002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2011773002/1

[dnj, 2016-05-25 05:27:14]

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

[dnj, 2016-05-25 05:27:16]

PTAL. I've written a few annotations, but hopefully mostly straightforward. I
think there was some funny stuff in multiarg so I (hope that I) cleaned it up a
bit.

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/da...
File service/datastore/datastore.go (left):

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/da...
service/datastore/datastore.go:251: func isOkType(t reflect.Type) error {
This check is moved into "multiarg.go".

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/da...
File service/datastore/datastore.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/da...
service/datastore/datastore.go:298: it.next(func(mat *multiArgType, slot
reflect.Value) error {
Just a quick explanation, since Rietveld sorted this first:
- "it" is a flattening iterator over the (potentially nested) input space.
- Any error returned here is stored in a (potentially nested) MultiError by
"it".

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/da...
service/datastore/datastore.go:310: err = it.error()
(This returns the MultiError accumulated by "it").

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/da...
File service/datastore/datastore_test.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/da...
service/datastore/datastore_test.go:434: Convey("Testing Put", func() {
(Most of this is just an indentation. The new stuff is at the bottom. Same for
Get.)

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/mu...
File service/datastore/multiarg.go (left):

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/mu...
service/datastore/multiarg.go:23: func (mat *multiArgType) GetKeysPMs(aid, ns
string, slice reflect.Value, meta bool) ([]*Key, []PropertyMap, error) {
This has been moved to the bottom, and is now a function in metaMultiArg.

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/mu...
File service/datastore/multiarg.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/mu...
service/datastore/multiarg.go:31: // If a map type implements an interface, its
pointer is also considered to
(This is why the map thing was so wonky. I didn't know this, but see:
https://play.golang.org/p/mV6UDNsF_0 )

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/mu...
service/datastore/multiarg.go:151: return newKeyObjErr(aid, ns,
getMGS(slot.Addr().Interface()))
This was a bug. MGS overrides wouldn't be honored. New vararg tests this now.

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/mu...
service/datastore/multiarg.go:188: err := toPLS(slot).Load(pm)
I'll collapse this in the next PS.

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/mu...
service/datastore/multiarg.go:204: return newKeyObjErr(aid, ns,
getMGS(slot.Elem().Interface()))
This, I think, was a bug.

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/mu...
service/datastore/multiarg.go:274: type metaMultiArg struct {
*so meta*

But if there's a better name I might be open to it.

This should be pretty efficient.

[commit-bot: I haz the power, 2016-05-25 05:32:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 05:32:23]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 05:32:31]

Dry run: None

[iannucci, 2016-05-25 17:36:47]

high-level comments, will make lower-level comments when I have more bandwidth.

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/in...
File service/datastore/interface.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/in...
service/datastore/interface.go:207: // this interface at some point in the
future.
as in: right after this lands and we refactor luci-go/infra?

https://chromiumcodereview.appspot.com/2011773002/diff/1/service/datastore/in...
service/datastore/interface.go:264: DeleteMulti(keys ...*Key) error
do you plan to reconcile the Delete interface with the Put/Get interface? I'm
not sure I like the discrepancy :/

Why not deprecate DeleteMulti and make Delete take ...*Key?

Why did we decide against making delete also take ...interface{} where those
things can be exactly the same objects that we passed to Put/Get (and extract a
*Key using the same mechanism, assuming that the object isn't actually a *Key
itself)? I remember talking about it, but I don't remember the outcome.

[dnj, 2016-05-27 02:26:57]

Description was changed from

==========
datastore: variadic Get, Put, Exists, Delete.

Fixes #51.

Update Get, Put, Exists, and Delete to be variadic. This will make the
interface more usable and get rid of boilerplate in client code.

Get and Put now accept variadic arguments. Any one of these arguments
can be an argument that was previously valid for Get/Put or
GetMulti/PutMulti. This effectively obsoletes the *Multi versions of
these functions, but they are left in for backwards compatiblility with
the stated intention of removing them at a later date.

Get/Put will now return a single error in the one-argument case or a
MultiError in a multiple-argument case. Note that now that single
argument case can take a slice type, the single error may itself be a
MultiError if the single argument is a slice.

DeleteMulti and ExistsMulti now take a variadic key argument instead of
a key slice. This allows a cleaner expression of keys to check/delete.

Note that this introduces API-incompatible changes in DeleteMulti and
ExistsMulti. Client code will have to be trivially updated by either
using the new variadic option or adding "..." after the keys array.
Any existing usage of Get or Put will continue to work as expected.

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

to

==========
datastore: variadic Get, Put, Exists, Delete.

Fixes #51.

Update Get, Put, Exists, and Delete to be variadic. This will make the
interface more usable and get rid of boilerplate in client code.

Get, Put, Exists, and Delete now accept variadic arguments. Any one of
these arguments can be an argument that was previously valid or
the *Multi version of those functions. This effectively obsoletes the *Multi
versions, but they are left in for backwards compatiblility with the stated
intention of removing them at a later date.

Get/Put/Exists/Delete will now return a single error in the one-argument
case or a MultiError in a multiple-argument case. Note that now that single
argument case can take a slice type, the single error may itself be a
MultiError if the single argument is a slice.
==========

[dnj (Google), 2016-05-27 16:49:53]

The CQ bit was checked by dnj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 16:49:55]

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

[commit-bot: I haz the power, 2016-05-27 16:49:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 16:49:59]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

[dnj (Google), 2016-05-27 17:53:16]

Patchset #4 (id:60001) has been deleted

[dnj (Google), 2016-05-27 18:12:40]

The CQ bit was checked by dnj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 18:12:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2011773002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2011773002/80001

[commit-bot: I haz the power, 2016-05-27 18:12:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 18:12:52]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

[dnj, 2016-05-27 20:32:21]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 20:32:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2011773002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2011773002/80001

[commit-bot: I haz the power, 2016-05-27 20:37:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 20:37:37]

Dry run: This issue passed the CQ dry run.

[iannucci, 2016-05-28 02:50:53]

lgtm

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
File service/datastore/interface.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/interface.go:144: //	- []I where i is some interface type.
Each elemet of the slice must
elemet!

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/interface.go:157: // return a MultiError containing a nexted
MultiError for each slice argument.
return a return a

nexted

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
File service/datastore/multiarg.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/multiarg.go:29: func parseArg(et reflect.Type, keysOnly bool)
*multiArgType {
s/keysOnly/allowKeys

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/multiarg.go:101: case reflect.Map:
I think chans also can hit this.

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/multiarg.go:409: func (mac *metaMultiArgIterator) next(fn
func(*multiArgType, reflect.Value) error) {
s/mac/it ?

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/multiarg.go:479: et.sliceErrors =
make(map[int]errors.LazyMultiError)
let's just allocate the full 1st dimension map on the first non-nil error

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
File service/datastore/types.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/types.go:95: slices []BoolList
bitfield?

[dnj, 2016-05-28 17:47:21]

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
File service/datastore/interface.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/interface.go:144: //	- []I where i is some interface type.
Each elemet of the slice must
On 2016/05/28 02:50:53, iannucci wrote:
> elemet!

Done.

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/interface.go:157: // return a MultiError containing a nexted
MultiError for each slice argument.
On 2016/05/28 02:50:53, iannucci wrote:
> return a return a
> 
> nexted

Done.

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
File service/datastore/multiarg.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/multiarg.go:29: func parseArg(et reflect.Type, keysOnly bool)
*multiArgType {
On 2016/05/28 02:50:53, iannucci wrote:
> s/keysOnly/allowKeys

Done.

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/multiarg.go:101: case reflect.Map:
On 2016/05/28 02:50:53, iannucci wrote:
> I think chans also can hit this.

Done. Unfortunately, there is *no way* to communicate the capacity of the
channel at the moment. I made a comment to this extent. I am reluctant to pour
time into that because neither of us can imagine a good use case for a channel
PLS.

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/multiarg.go:409: func (mac *metaMultiArgIterator) next(fn
func(*multiArgType, reflect.Value) error) {
On 2016/05/28 02:50:53, iannucci wrote:
> s/mac/it ?

Done.

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/multiarg.go:479: et.sliceErrors =
make(map[int]errors.LazyMultiError)
On 2016/05/28 02:50:53, iannucci wrote:
> let's just allocate the full 1st dimension map on the first non-nil error 

Am building in-place. There is a bit of reflection per error, but I think this
is probably not worse than a hash lookup.

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
File service/datastore/types.go (right):

https://chromiumcodereview.appspot.com/2011773002/diff/80001/service/datastor...
service/datastore/types.go:95: slices []BoolList
On 2016/05/28 02:50:53, iannucci wrote:
> bitfield?

IMO BoolList is more Go-friendly, since it actually can be treated/casted as a
slice of booleans. Agree bitfield would work, but I am going to stick with more
idiomatic BoolList.

[dnj, 2016-05-28 17:49:16]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-05-28 17:49:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2011773002/#ps120001 (title:
"s/chn/chan/")

[commit-bot: I haz the power, 2016-05-28 17:49:23]

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

[commit-bot: I haz the power, 2016-05-28 17:54:18]

Description was changed from

==========
datastore: variadic Get, Put, Exists, Delete.

Fixes #51.

Update Get, Put, Exists, and Delete to be variadic. This will make the
interface more usable and get rid of boilerplate in client code.

Get, Put, Exists, and Delete now accept variadic arguments. Any one of
these arguments can be an argument that was previously valid or
the *Multi version of those functions. This effectively obsoletes the *Multi
versions, but they are left in for backwards compatiblility with the stated
intention of removing them at a later date.

Get/Put/Exists/Delete will now return a single error in the one-argument
case or a MultiError in a multiple-argument case. Note that now that single
argument case can take a slice type, the single error may itself be a
MultiError if the single argument is a slice.
==========

to

==========
datastore: variadic Get, Put, Exists, Delete.

Fixes #51.

Update Get, Put, Exists, and Delete to be variadic. This will make the
interface more usable and get rid of boilerplate in client code.

Get, Put, Exists, and Delete now accept variadic arguments. Any one of
these arguments can be an argument that was previously valid or
the *Multi version of those functions. This effectively obsoletes the *Multi
versions, but they are left in for backwards compatiblility with the stated
intention of removing them at a later date.

Get/Put/Exists/Delete will now return a single error in the one-argument
case or a MultiError in a multiple-argument case. Note that now that single
argument case can take a slice type, the single error may itself be a
MultiError if the single argument is a slice.

Committed:
https://github.com/luci/gae/commit/ee5266ebb4c1957a848000175f03ddb6845d01ed
==========

[commit-bot: I haz the power, 2016-05-28 17:54:19]

Committed patchset #6 (id:120001) as
https://github.com/luci/gae/commit/ee5266ebb4c1957a848000175f03ddb6845d01ed