buildbucket: put_batch endpoint

buildbucket: put_batch endpoint

Added put_batch endpoint that adds up multiple builds.

R=vadimsh@chromium.org
BUG=461620
Committed: https://chromium.googlesource.com/infra/infra/+/093008bb0272b825d0fbceec323ec0a98553190a

[nodir, 2015-04-14 18:43:55]

Patchset #1 (id:1) has been deleted

[nodir, 2015-04-14 18:44:53]

PTAL,

In https://codereview.chromium.org/1058893004/ esprehn@ said we should send one
http request to schedule multiple builds.

[Vadim Sh., 2015-04-14 18:56:40]

Giant XG transaction concerns me.

https://codereview.chromium.org/1082303002/diff/20001/appengine/cr-buildbucke...
File appengine/cr-buildbucket/api.py (right):

https://codereview.chromium.org/1082303002/diff/20001/appengine/cr-buildbucke...
appengine/cr-buildbucket/api.py:258: def put_batch(self, request):
btw, apiary APIs exposed by Google support batching automagically on Endpoints
level, e.g. https://cloud.google.com/compute/docs/api/how-tos/batch

Maybe Cloud Endpoints can do it too?..

https://codereview.chromium.org/1082303002/diff/20001/appengine/cr-buildbucke...
File appengine/cr-buildbucket/service.py (right):

https://codereview.chromium.org/1082303002/diff/20001/appengine/cr-buildbucke...
appengine/cr-buildbucket/service.py:138: @ndb.transactional(xg=True)  # pylint:
disable=no-value-for-parameter
I don't think adding them all transactionally is a good idea:
1) Weird unavoidable limit of 25.
2) It is slow, with 25 groups probably very slow (have you tried it?).

One way of not triggering already triggered builds on retry is to introduce a
new "client_operation_id" into API that server can use to identify retries. E.g.
put_batch([builds], operation_id) -> ([triggered], [existing]). Then from client
point of view put_batch(..., some client id) is essentially idempotent.

[nodir, 2015-04-14 22:07:10]

Patchset #2 (id:40001) has been deleted

[nodir, 2015-04-14 22:12:38]

Patchset #2 (id:60001) has been deleted

[nodir, 2015-04-14 22:13:05]

https://codereview.chromium.org/1082303002/diff/20001/appengine/cr-buildbucke...
File appengine/cr-buildbucket/api.py (right):

https://codereview.chromium.org/1082303002/diff/20001/appengine/cr-buildbucke...
appengine/cr-buildbucket/api.py:258: def put_batch(self, request):
On 2015/04/14 18:56:40, Vadim Sh. wrote:
> btw, apiary APIs exposed by Google support batching automagically on Endpoints
> level, e.g. https://cloud.google.com/compute/docs/api/how-tos/batch
> 
> Maybe Cloud Endpoints can do it too?..

It cannot

https://codereview.chromium.org/1082303002/diff/20001/appengine/cr-buildbucke...
File appengine/cr-buildbucket/service.py (right):

https://codereview.chromium.org/1082303002/diff/20001/appengine/cr-buildbucke...
appengine/cr-buildbucket/service.py:138: @ndb.transactional(xg=True)  # pylint:
disable=no-value-for-parameter
On 2015/04/14 18:56:40, Vadim Sh. wrote:
> I don't think adding them all transactionally is a good idea:
> 1) Weird unavoidable limit of 25.
> 2) It is slow, with 25 groups probably very slow (have you tried it?).
> 
> One way of not triggering already triggered builds on retry is to introduce a
> new "client_operation_id" into API that server can use to identify retries.
E.g.
> put_batch([builds], operation_id) -> ([triggered], [existing]). Then from
client
> point of view put_batch(..., some client id) is essentially idempotent.

Done.

[nodir, 2015-04-14 23:23:40]

Added client_operation_id in the response

[Vadim Sh., 2015-04-15 01:02:18]

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
File appengine/cr-buildbucket/model.py (right):

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
appengine/cr-buildbucket/model.py:162: """Stores a reference to a build. Keyed
by client-supplied operation id.
explain why you need it

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
File appengine/cr-buildbucket/service.py (right):

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:93: @ndb.transactional_tasklet(xg=True)  #
pylint: disable=no-value-for-parameter
If requests that do not use client_operation_id are frequent enough, it would be
better for performance to not use transaction if client_operation_id is None. If
fact, it almost always better to put transaction only around code that needs it.
In this case all validation and ACL checks should be outside the transaction.
I'd put transaction only around two 'put' calls:

def run():
  build.put()
  op.put()
ndb.transaction(run)

(not sure how to express this in async code)

The longer transaction is open, the more a chance of a collision.

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:109: client_operation_id (str or int):
client-supplied operation id. If an
just make it 'str', no need to introduce unnecessary flexibility

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:128: client_operation_id)
prefix operation ids with identity.to_bytes() so that each caller has its own
warm and fuzzy namespace for IDs.

[Vadim Sh., 2015-04-15 01:04:05]

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
File appengine/cr-buildbucket/service.py (right):

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:93: @ndb.transactional_tasklet(xg=True)  #
pylint: disable=no-value-for-parameter
On 2015/04/15 01:02:18, Vadim Sh. wrote:
> If requests that do not use client_operation_id are frequent enough, it would
be
> better for performance to not use transaction if client_operation_id is None.
If
> fact, it almost always better to put transaction only around code that needs
it.
> In this case all validation and ACL checks should be outside the transaction.
> I'd put transaction only around two 'put' calls:
> 
> def run():
>   build.put()
>   op.put()
> ndb.transaction(run)
> 
> (not sure how to express this in async code)
> 
> The longer transaction is open, the more a chance of a collision.

Hm.. you need to do "get client op" in the same transaction, so just two puts
won't do. Anyhow, general intention is the same: minimize time the transaction
is open, or avoid it at all, if possible.

[Vadim Sh., 2015-04-15 01:10:22]

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
File appengine/cr-buildbucket/service.py (right):

https://codereview.chromium.org/1082303002/diff/100001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:128: client_operation_id)
On 2015/04/15 01:02:17, Vadim Sh. wrote:
> prefix operation ids with identity.to_bytes() so that each caller has its own
> warm and fuzzy namespace for IDs.

Actually, it might be good to force client to use UUID format
(de305d54-75b4-431b-adb2-eb6b9e546014). I'm concerned that these operation IDs
do not expire. If client tries to use something dumb, it may hit existing client
IDs by mistake.

Also I'm not super excited about keeping this garbage in the datastore
indefinitely. That's why I proposed memcache initially: no transactions, no need
to cleanup, but non zero chance of not working correctly. Considering
operation_client_id mechanism is supposed to be used only when retrying failed
requests (already exceptional situation), non zero chance of not working
correctly might be acceptable. Think about that :)

[nodir, 2015-04-15 23:59:06]

Patchset #3 (id:100001) has been deleted

[nodir, 2015-04-15 23:59:16]

Patchset #3 (id:120001) has been deleted

[nodir, 2015-04-15 23:59:56]

I've accidently deleted my prev patchset and your comments

I've switched to memcache and made client_operation_id always str in docstring

[Vadim Sh., 2015-04-16 00:12:21]

https://codereview.chromium.org/1082303002/diff/140001/appengine/cr-buildbuck...
File appengine/cr-buildbucket/service.py (right):

https://codereview.chromium.org/1082303002/diff/140001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:112: instead.
mention that expires within 1 min

https://codereview.chromium.org/1082303002/diff/140001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:130: identity.to_bytes(),
client_operation_id))
forbid "/" in client_operation_id
or hash it before putting into memcache key.

https://codereview.chromium.org/1082303002/diff/140001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:154:
memcache.set(client_operation_cache_key, build.key.id(), 60)
do it async too, ndb supports memcache (forgot exact calls)

https://codereview.chromium.org/1082303002/diff/140001/appengine_module/testi...
File appengine_module/testing_utils/testing.py (right):

https://codereview.chromium.org/1082303002/diff/140001/appengine_module/testi...
appengine_module/testing_utils/testing.py:32: consistency_policy = None
hm.. can you please commit it in a separate CL (verifying all tests pass)? Also,
why is it needed?

[nodir, 2015-04-16 00:36:20]

https://codereview.chromium.org/1082303002/diff/140001/appengine/cr-buildbuck...
File appengine/cr-buildbucket/service.py (right):

https://codereview.chromium.org/1082303002/diff/140001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:112: instead.
On 2015/04/16 00:12:21, Vadim Sh. wrote:
> mention that expires within 1 min

Done.

https://codereview.chromium.org/1082303002/diff/140001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:130: identity.to_bytes(),
client_operation_id))
On 2015/04/16 00:12:21, Vadim Sh. wrote:
> forbid "/" in client_operation_id
> or hash it before putting into memcache key.

Done.

https://codereview.chromium.org/1082303002/diff/140001/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:154:
memcache.set(client_operation_cache_key, build.key.id(), 60)
On 2015/04/16 00:12:21, Vadim Sh. wrote:
> do it async too, ndb supports memcache (forgot exact calls)

Done.

https://codereview.chromium.org/1082303002/diff/140001/appengine_module/testi...
File appengine_module/testing_utils/testing.py (right):

https://codereview.chromium.org/1082303002/diff/140001/appengine_module/testi...
appengine_module/testing_utils/testing.py:32: consistency_policy = None
On 2015/04/16 00:12:21, Vadim Sh. wrote:
> hm.. can you please commit it in a separate CL (verifying all tests pass)?
Also,
> why is it needed?

not needed anymore, since xg transactions are not used

[Vadim Sh., 2015-04-16 00:42:27]

lgtm

https://codereview.chromium.org/1082303002/diff/130007/appengine/cr-buildbuck...
File appengine/cr-buildbucket/service.py (right):

https://codereview.chromium.org/1082303002/diff/130007/appengine/cr-buildbuck...
appengine/cr-buildbucket/service.py:13: from google.appengine.api import
memcache
no longer used probably

[nodir, 2015-04-16 00:45:32]

The CQ bit was checked by nodir@chromium.org

[nodir, 2015-04-16 00:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/1082303002/#ps170001
(title: "removed memcache import")

[commit-bot: I haz the power, 2015-04-16 00:45:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082303002/170001

[commit-bot: I haz the power, 2015-04-16 00:49:38]

Committed patchset #5 (id:170001) as
https://chromium.googlesource.com/infra/infra/+/093008bb0272b825d0fbceec323ec...