Replace custom blob gRPC API with ByteStream

Replace custom blob gRPC API with ByteStream

Also allow bot code to be sharded in memcached to get around the 1MB
memcached limit.

BUG=

Review-Url: https://codereview.chromium.org/2953253003
Committed: https://github.com/luci/luci-py/commit/e3c4ba92a8136e80daa32a27299f463a9e8d4f7a

[aludwin, 2017-06-23 14:06:22]

aludwin@google.com changed reviewers:
+ ryanmartens@google.com, vadimsh@google.com

[aludwin, 2017-06-23 14:06:23]

Hi Ryan, Vadim - PTAL. The majority of this change is to add OAuth and TLS to
gRPC (sadly, it doesn't reuse the existing libraries).

[aludwin, 2017-06-23 14:07:53]

+maruel for the record

[Vadim Sh., 2017-06-23 18:26:49]

vadimsh@chromium.org changed reviewers:
+ vadimsh@chromium.org

[Vadim Sh., 2017-06-23 18:26:51]

Some comments for server side, haven't looked at client side yet.

(Also, it could have been 3 neat isolated CLs instead of single big one:
 * Memcache change
 * Adding third parties
 * gRPC change)

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
File appengine/swarming/server/bot_code.py (right):

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:31: MAX_MEMCACHED_SIZE_BYTES = 1000000
how many parts do we get?

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:243: 
nit: \n (two blank lines between top-level code units)

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:246: 
nit: \n

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:250: num_parts =
get_cached_bot_entry(version, 'parts')
'parts' and 'signature' should be part of one memcache entry, they are always
fetched and stored together, no need to do multiple RPCs

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:254: content +=
get_cached_bot_entry(version, 'content', p)
please use asynchronous memcache API to fetch parts in parallel

something similar to:
ctx = ndb.get_context()
futures = [ctx.memcache_get(...) for p in range(num_parts)]
content = ''.join(f.get_result() for f in futures)

(or you can convert get_cached_swarming_bot_zip and cache_swarming_bot_zip to
tasklets and use 'yield' inside)

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:271: while len(content) > 0:
same here, it should be done in parallel

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:305: 
nit: \n

https://codereview.chromium.org/2953253003/diff/40001/client/third_party/cach...
File client/third_party/cachetools/__init__.py (right):

https://codereview.chromium.org/2953253003/diff/40001/client/third_party/cach...
client/third_party/cachetools/__init__.py:1: """Extensible memoizing collections
and decorators."""
please add README.swarming file describing where you got this package, at which
revision, etc.

[chromium-reviews, 2017-06-23 19:36:34]

I could break it up if you prefer. As you can probably guess, it grew
organically as I tried to figure out how to make it work.

On Fri, 23 Jun 2017, 2:26 pm , <vadimsh@chromium.org> wrote:

> Some comments for server side, haven't looked at client side yet.
>
> (Also, it could have been 3 neat isolated CLs instead of single big one:
> * Memcache change
> * Adding third parties
> * gRPC change)
>
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
> File appengine/swarming/server/bot_code.py (right):
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
> appengine/swarming/server/bot_code.py:31: MAX_MEMCACHED_SIZE_BYTES =
> 1000000
> how many parts do we get?
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
> appengine/swarming/server/bot_code.py:243:
> nit: \n (two blank lines between top-level code units)
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
> appengine/swarming/server/bot_code.py:246:
> nit: \n
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
> appengine/swarming/server/bot_code.py:250: num_parts =
> get_cached_bot_entry(version, 'parts')
> 'parts' and 'signature' should be part of one memcache entry, they are
> always fetched and stored together, no need to do multiple RPCs
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
> appengine/swarming/server/bot_code.py:254: content +=
> get_cached_bot_entry(version, 'content', p)
> please use asynchronous memcache API to fetch parts in parallel
>
> something similar to:
> ctx = ndb.get_context()
> futures = [ctx.memcache_get(...) for p in range(num_parts)]
> content = ''.join(f.get_result() for f in futures)
>
> (or you can convert get_cached_swarming_bot_zip and
> cache_swarming_bot_zip to tasklets and use 'yield' inside)
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
> appengine/swarming/server/bot_code.py:271: while len(content) > 0:
> same here, it should be done in parallel
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
> appengine/swarming/server/bot_code.py:305:
> nit: \n
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/client/third_party/cach...
> File client/third_party/cachetools/__init__.py (right):
>
>
>
https://codereview.chromium.org/2953253003/diff/40001/client/third_party/cach...
> client/third_party/cachetools/__init__.py:1: """Extensible memoizing
> collections and decorators."""
> please add README.swarming file describing where you got this package,
> at which revision, etc.
>
> https://codereview.chromium.org/2953253003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Vadim Sh., 2017-06-23 19:48:46]

On 2017/06/23 19:36:34, chromium-reviews wrote:
> I could break it up if you prefer. As you can probably guess, it grew
> organically as I tried to figure out how to make it work.
> 

Nah, it's fine :) Luckily, it's separable by files already, so reviewing it
isn't too hard.

[aludwin, 2017-06-26 17:12:57]

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
File appengine/swarming/server/bot_code.py (right):

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:31: MAX_MEMCACHED_SIZE_BYTES = 1000000
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> how many parts do we get?

Two currently - though in the unit test, we get 11 since I reduce this by a
factor of 10.

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:243: 
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> nit: \n (two blank lines between top-level code units)

Done.

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:246: 
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> nit: \n

Done.

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:250: num_parts =
get_cached_bot_entry(version, 'parts')
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> 'parts' and 'signature' should be part of one memcache entry, they are always
> fetched and stored together, no need to do multiple RPCs

Done.

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:250: num_parts =
get_cached_bot_entry(version, 'parts')
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> 'parts' and 'signature' should be part of one memcache entry, they are always
> fetched and stored together, no need to do multiple RPCs

Done.

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:254: content +=
get_cached_bot_entry(version, 'content', p)
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> please use asynchronous memcache API to fetch parts in parallel
> 
> something similar to:
> ctx = ndb.get_context()
> futures = [ctx.memcache_get(...) for p in range(num_parts)]
> content = ''.join(f.get_result() for f in futures)
> 
> (or you can convert get_cached_swarming_bot_zip and cache_swarming_bot_zip to
> tasklets and use 'yield' inside)

Done.

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:271: while len(content) > 0:
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> same here, it should be done in parallel

Done.

https://codereview.chromium.org/2953253003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:305: 
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> nit: \n

Missed this one but checked in locally for next upload.

https://codereview.chromium.org/2953253003/diff/40001/client/third_party/cach...
File client/third_party/cachetools/__init__.py (right):

https://codereview.chromium.org/2953253003/diff/40001/client/third_party/cach...
client/third_party/cachetools/__init__.py:1: """Extensible memoizing collections
and decorators."""
On 2017/06/23 18:26:50, Vadim Sh. wrote:
> please add README.swarming file describing where you got this package, at
which
> revision, etc.

Done. Also updated the other README.swarming.

[Vadim Sh., 2017-06-27 19:12:49]

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/bot_code.py (right):

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:258: for p in range((int)(num_parts))]
just int(num_parts)

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:259: content = ''.join(f.get_result() for
f in futures)
this will raise TypeError if some of memcache entries are missing:
''.join([None, '...']) is not a valid call.

Also it would be better to explicitly log that some memcache entry is missing
instead of logging misleading "bot .. had signature ... instead of expected ..."
error.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:279: content=content[chunk_size:]
nit: spaces around =

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:282:
futures.append(bot_memcache_async_set(meta, version, 'meta'))
this entry must be set only after all chunks are successfully stored, otherwise
get_cached_swarming_bot_zip may attempt to read chunks before they actually
stored

So: 
1. Store chunks in parallel.
2. Then, upon completion, store meta.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:283: ndb.Future.wait_all(futures)
wait_all doesn't check for errors. It just waits for all futures to complete
(successfully or not). Instead you can use:
for f in futures:
  f.check_success()

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:288: def bot_memcache_get(version, desc):
you can get rid of non-async versions and just use get_result() instead. This
will deduplicate some code.

e.g. in get_cached_swarming_bot_zip: 
meta = bot_memcache_get_async(version, 'meta').get_result()

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:293: def bot_memcache_async_get(version,
desc, part):
nit: _async is usually put in the end (by ndb conventions),
bot_memcache_get_async

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/bot_code_test.py (right):

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code_test.py:107: def
mock_memcache_async_get(version, desc, part=None):
this will be more correct mock:

@ndb.tasklet
def mock_memcache_get_async(....):
  raise ndb.Return(local_mc['store'].get(....))

Or equivalently:

def mock_memcache_get_async(....):
  fut = ndb.Future()
  fut.set_result(local_mc['store'].get(....))
  return fut

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage.py
File client/isolate_storage.py (right):

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:41: import certifi
where is this used?

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:559: credentials, _ =
google_auth.default(scopes=scopes)
If you are requiring configured Application Default Credentials on workers, it
will make a lot of sense to also use them to authenticate calls to Swarming
server (instead of having two different authentication schemes).

Not for this CL, just in general.

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:575: self._stub =
bytestream_pb2.ByteStreamStub(self._channel)
if this object thread safe? 'fetch' and 'push' will be called concurrently from
multiple threads.

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:662: logging.error('error during push: throwing as
IOError (%s)' % e)
nit: use 2 spaces for indentation

[aludwin, 2017-06-27 19:23:06]

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/bot_code.py (right):

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:259: content = ''.join(f.get_result() for
f in futures)
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> this will raise TypeError if some of memcache entries are missing:
> ''.join([None, '...']) is not a valid call.
> 
> Also it would be better to explicitly log that some memcache entry is missing
> instead of logging misleading "bot .. had signature ... instead of expected
..."
> error.

Would you recommend that I explicitly wait on the futures then? Or catch the
TypeError exception?

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:283: ndb.Future.wait_all(futures)
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> wait_all doesn't check for errors. It just waits for all futures to complete
> (successfully or not). Instead you can use:
> for f in futures:
>   f.check_success()

But what's the failure recovery? Or do you just want me to raise an exception?

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage.py
File client/isolate_storage.py (right):

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:41: import certifi
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> where is this used?

It's conditionally imported by google.auth.transport - but if it doesn't exist,
random things fail. So this is just an assertion (see the following line). Is
there some special construct I should use to represent an import that I'm not
going to use, or should I just comment it better?

[Vadim Sh., 2017-06-27 19:27:09]

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/bot_code.py (right):

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:259: content = ''.join(f.get_result() for
f in futures)
On 2017/06/27 19:23:06, aludwin wrote:
> On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > this will raise TypeError if some of memcache entries are missing:
> > ''.join([None, '...']) is not a valid call.
> > 
> > Also it would be better to explicitly log that some memcache entry is
missing
> > instead of logging misleading "bot .. had signature ... instead of expected
> ..."
> > error.
> 
> Would you recommend that I explicitly wait on the futures then? Or catch the
> TypeError exception?

Explicitly waiting is better.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:283: ndb.Future.wait_all(futures)
On 2017/06/27 19:23:06, aludwin wrote:
> On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > wait_all doesn't check for errors. It just waits for all futures to complete
> > (successfully or not). Instead you can use:
> > for f in futures:
> >   f.check_success()
> 
> But what's the failure recovery? Or do you just want me to raise an exception?

'check_success' raises exceptions in case of errors. Just don't catch them.
They'll result in HTTP 500, this is fine. 

This is what happens any time we use memcache.get(...): this call may raise an
exception (that we don't explicitly catch). This is useful property. Errors
shouldn't be ignored.

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage.py
File client/isolate_storage.py (right):

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:41: import certifi
On 2017/06/27 19:23:06, aludwin wrote:
> On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > where is this used?
> 
> It's conditionally imported by google.auth.transport - but if it doesn't
exist,
> random things fail. So this is just an assertion (see the following line). Is
> there some special construct I should use to represent an import that I'm not
> going to use, or should I just comment it better?

Yeah, leave a comment.

[aludwin, 2017-06-27 20:10:17]

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/bot_code.py (right):

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:258: for p in range((int)(num_parts))]
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> just int(num_parts)

Done.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:259: content = ''.join(f.get_result() for
f in futures)
On 2017/06/27 19:27:09, Vadim Sh. wrote:
> On 2017/06/27 19:23:06, aludwin wrote:
> > On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > > this will raise TypeError if some of memcache entries are missing:
> > > ''.join([None, '...']) is not a valid call.
> > > 
> > > Also it would be better to explicitly log that some memcache entry is
> missing
> > > instead of logging misleading "bot .. had signature ... instead of
expected
> > ..."
> > > error.
> > 
> > Would you recommend that I explicitly wait on the futures then? Or catch the
> > TypeError exception?
> 
> Explicitly waiting is better.

Done.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:279: content=content[chunk_size:]
On 2017/06/27 19:12:48, Vadim Sh. wrote:
> nit: spaces around =

Done.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:282:
futures.append(bot_memcache_async_set(meta, version, 'meta'))
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> this entry must be set only after all chunks are successfully stored,
otherwise
> get_cached_swarming_bot_zip may attempt to read chunks before they actually
> stored
> 
> So: 
> 1. Store chunks in parallel.
> 2. Then, upon completion, store meta.

Done.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:283: ndb.Future.wait_all(futures)
On 2017/06/27 19:27:09, Vadim Sh. wrote:
> On 2017/06/27 19:23:06, aludwin wrote:
> > On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > > wait_all doesn't check for errors. It just waits for all futures to
complete
> > > (successfully or not). Instead you can use:
> > > for f in futures:
> > >   f.check_success()
> > 
> > But what's the failure recovery? Or do you just want me to raise an
exception?
> 
> 'check_success' raises exceptions in case of errors. Just don't catch them.
> They'll result in HTTP 500, this is fine. 
> 
> This is what happens any time we use memcache.get(...): this call may raise an
> exception (that we don't explicitly catch). This is useful property. Errors
> shouldn't be ignored.

Done.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:288: def bot_memcache_get(version, desc):
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> you can get rid of non-async versions and just use get_result() instead. This
> will deduplicate some code.
> 
> e.g. in get_cached_swarming_bot_zip: 
> meta = bot_memcache_get_async(version, 'meta').get_result()
> 

Done.

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code.py:293: def bot_memcache_async_get(version,
desc, part):
On 2017/06/27 19:12:48, Vadim Sh. wrote:
> nit: _async is usually put in the end (by ndb conventions),
> bot_memcache_get_async

Since I'm getting rid of the non-async versions, I'm just calling this
bot_memcache_<get|set>

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/bot_code_test.py (right):

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code_test.py:107: def
mock_memcache_async_get(version, desc, part=None):
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> this will be more correct mock:
> 
> @ndb.tasklet
> def mock_memcache_get_async(....):
>   raise ndb.Return(local_mc['store'].get(....))
> 
> Or equivalently:
> 
> def mock_memcache_get_async(....):
>   fut = ndb.Future()
>   fut.set_result(local_mc['store'].get(....))
>   return fut

How would I do the counting in this case? I'd like to delay the counts until
someone says "get_result".

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage.py
File client/isolate_storage.py (right):

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:41: import certifi
On 2017/06/27 19:27:09, Vadim Sh. wrote:
> On 2017/06/27 19:23:06, aludwin wrote:
> > On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > > where is this used?
> > 
> > It's conditionally imported by google.auth.transport - but if it doesn't
> exist,
> > random things fail. So this is just an assertion (see the following line).
Is
> > there some special construct I should use to represent an import that I'm
not
> > going to use, or should I just comment it better?
> 
> Yeah, leave a comment.

Done.

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:575: self._stub =
bytestream_pb2.ByteStreamStub(self._channel)
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> if this object thread safe? 'fetch' and 'push' will be called concurrently
from
> multiple threads.

Yup: https://github.com/grpc/grpc/issues/9320

[aludwin, 2017-06-27 20:20:11]

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

[commit-bot: I haz the power, 2017-06-27 20:20:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 20:20:15]

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

[commit-bot: I haz the power, 2017-06-27 20:20:16]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started 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.

[Vadim Sh., 2017-06-27 21:14:29]

few nits

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/bot_code_test.py (right):

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code_test.py:107: def
mock_memcache_async_get(version, desc, part=None):
On 2017/06/27 20:10:17, aludwin wrote:
> On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > this will be more correct mock:
> > 
> > @ndb.tasklet
> > def mock_memcache_get_async(....):
> >   raise ndb.Return(local_mc['store'].get(....))
> > 
> > Or equivalently:
> > 
> > def mock_memcache_get_async(....):
> >   fut = ndb.Future()
> >   fut.set_result(local_mc['store'].get(....))
> >   return fut
> 
> How would I do the counting in this case? I'd like to delay the counts until
> someone says "get_result".

yeah, this will not be possible.

But why counting number of initiated calls is any worse? e.g 

@ndb.tasklet
def mock_memcache_get(...):
  local_mc['reads'] += 1
  raise ndb.Return(local_mc['store'].get(....))

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage.py
File client/isolate_storage.py (right):

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:662: logging.error('error during push: throwing as
IOError (%s)' % e)
On 2017/06/27 19:12:49, Vadim Sh. wrote:
> nit: use 2 spaces for indentation

Ping. It is 4 spaces now.

https://codereview.chromium.org/2953253003/diff/100001/appengine/swarming/ser...
File appengine/swarming/server/bot_code.py (right):

https://codereview.chromium.org/2953253003/diff/100001/appengine/swarming/ser...
appengine/swarming/server/bot_code.py:263: logging.error('bot %s was missing
some of its contents', version)
nit: 'bot code ver %s was missing ...'

https://codereview.chromium.org/2953253003/diff/100001/appengine/swarming/ser...
appengine/swarming/server/bot_code.py:269: logging.error('bot %s had signature
%s instead of expected %s', version,
same here actually

[aludwin, 2017-06-28 13:23:54]

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
File appengine/swarming/server/bot_code_test.py (right):

https://codereview.chromium.org/2953253003/diff/80001/appengine/swarming/serv...
appengine/swarming/server/bot_code_test.py:107: def
mock_memcache_async_get(version, desc, part=None):
On 2017/06/27 21:14:29, Vadim Sh. wrote:
> On 2017/06/27 20:10:17, aludwin wrote:
> > On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > > this will be more correct mock:
> > > 
> > > @ndb.tasklet
> > > def mock_memcache_get_async(....):
> > >   raise ndb.Return(local_mc['store'].get(....))
> > > 
> > > Or equivalently:
> > > 
> > > def mock_memcache_get_async(....):
> > >   fut = ndb.Future()
> > >   fut.set_result(local_mc['store'].get(....))
> > >   return fut
> > 
> > How would I do the counting in this case? I'd like to delay the counts until
> > someone says "get_result".
> 
> yeah, this will not be possible.
> 
> But why counting number of initiated calls is any worse? e.g 
> 
> @ndb.tasklet
> def mock_memcache_get(...):
>   local_mc['reads'] += 1
>   raise ndb.Return(local_mc['store'].get(....))

Just thought it would be nice to ensure that everything happened at the right
time - but I've changed it to look like this.

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage.py
File client/isolate_storage.py (right):

https://codereview.chromium.org/2953253003/diff/80001/client/isolate_storage....
client/isolate_storage.py:662: logging.error('error during push: throwing as
IOError (%s)' % e)
On 2017/06/27 21:14:29, Vadim Sh. wrote:
> On 2017/06/27 19:12:49, Vadim Sh. wrote:
> > nit: use 2 spaces for indentation
> 
> Ping. It is 4 spaces now.

Done.

https://codereview.chromium.org/2953253003/diff/100001/appengine/swarming/ser...
File appengine/swarming/server/bot_code.py (right):

https://codereview.chromium.org/2953253003/diff/100001/appengine/swarming/ser...
appengine/swarming/server/bot_code.py:263: logging.error('bot %s was missing
some of its contents', version)
On 2017/06/27 21:14:29, Vadim Sh. wrote:
> nit: 'bot code ver %s was missing ...'

Done.

https://codereview.chromium.org/2953253003/diff/100001/appengine/swarming/ser...
appengine/swarming/server/bot_code.py:269: logging.error('bot %s had signature
%s instead of expected %s', version,
On 2017/06/27 21:14:29, Vadim Sh. wrote:
> same here actually

Done.

[Vadim Sh., 2017-06-28 17:52:39]

lgtm with one last nit

https://codereview.chromium.org/2953253003/diff/120001/appengine/swarming/ser...
File appengine/swarming/server/bot_code_test.py (right):

https://codereview.chromium.org/2953253003/diff/120001/appengine/swarming/ser...
appengine/swarming/server/bot_code_test.py:101: @bot_code.ndb.tasklet
please import ndb directly in this module (somewhere after
test_env.setup_test_env() line).

[aludwin, 2017-06-28 19:19:49]

https://codereview.chromium.org/2953253003/diff/120001/appengine/swarming/ser...
File appengine/swarming/server/bot_code_test.py (right):

https://codereview.chromium.org/2953253003/diff/120001/appengine/swarming/ser...
appengine/swarming/server/bot_code_test.py:101: @bot_code.ndb.tasklet
On 2017/06/28 17:52:39, Vadim Sh. wrote:
> please import ndb directly in this module (somewhere after
> test_env.setup_test_env() line).

Done.

[aludwin, 2017-06-28 19:20:00]

The CQ bit was checked by aludwin@google.com

[aludwin, 2017-06-28 19:20:00]

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/2953253003/#ps140001
(title: "Import ndb directly to test code")

[commit-bot: I haz the power, 2017-06-28 19:20:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aludwin, 2017-06-28 19:27:39]

The CQ bit was unchecked by aludwin@google.com

[aludwin, 2017-06-28 19:27:54]

The CQ bit was checked by aludwin@google.com

[commit-bot: I haz the power, 2017-06-28 19:28:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aludwin, 2017-06-28 19:39:39]

The CQ bit was unchecked by aludwin@google.com

[aludwin, 2017-06-28 19:42:07]

The CQ bit was checked by aludwin@google.com

[commit-bot: I haz the power, 2017-06-28 19:42:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 20:05:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498678927432900,
"parent_rev": "d96432e8cebc8d0dcdb4754adfe29319a6bb5838", "commit_rev":
"e3c4ba92a8136e80daa32a27299f463a9e8d4f7a"}

[commit-bot: I haz the power, 2017-06-28 20:06:04]

Description was changed from

==========
Replace custom blob gRPC API with ByteStream

Also allow bot code to be sharded in memcached to get around the 1MB
memcached limit.

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

to

==========
Replace custom blob gRPC API with ByteStream

Also allow bot code to be sharded in memcached to get around the 1MB
memcached limit.

BUG=

Review-Url: https://codereview.chromium.org/2953253003
Committed:
https://github.com/luci/luci-py/commit/e3c4ba92a8136e80daa32a27299f463a9e8d4f7a
==========

[commit-bot: I haz the power, 2017-06-28 20:06:05]

Committed patchset #8 (id:140001) as
https://github.com/luci/luci-py/commit/e3c4ba92a8136e80daa32a27299f463a9e8d4f7a