Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2212073002:
Add endpoint and cron job to aggregate all dimensions and values (Closed)
|
Created:
4 years, 4 months ago by kjlubick Modified:
4 years, 4 months ago Reviewers:
M-A Ruel CC:
chromium-reviews, infra-reviews+luci-py_chromium.org Base URL:
https://chromium.googlesource.com/external/github.com/luci/luci-py@master Target Ref:
refs/heads/master Project:
luci-py Visibility:
Public. |
DescriptionAdd endpoint and cron job to aggregate all dimensions and values
BUG=631047
Committed: https://github.com/luci/luci-py/commit/942902ecc1b3cc22b30921a3a9792694e67cccb0
Patch Set 1 #Patch Set 2 : pull out extra changes #
Total comments: 32
Patch Set 3 : Do everything but add a test #Patch Set 4 : y #
Total comments: 18
Patch Set 5 : trying things #
Total comments: 6
Patch Set 6 : Make tests happy #Patch Set 7 : reuse KEY #
Total comments: 30
Patch Set 8 : Address comments #
Total comments: 14
Patch Set 9 : Address more comments #
Total comments: 4
Patch Set 10 : Address nits #
Dependent Patchsets: Messages
Total messages: 27 (11 generated)
Description was changed from ========== Add endpoint and cron job to aggregate all dimensions and values BUG= ========== to ========== Add endpoint and cron job to aggregate all dimensions and values BUG=631047 ==========
kjlubick@google.com changed reviewers: + maruel@chromium.org
please add a test https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/cron... File appengine/swarming/cron.yaml (right): https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/cron... appengine/swarming/cron.yaml:29: - description: Aggregate all dimensions for easier sorting. Aggregate all bot dimensions. otherwise it could be about tasks, which will be needed later. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... File appengine/swarming/handlers_backend.py (right): https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:100: class CronDimensionAggregationHandler(webapp2.RequestHandler): 2 lines between file level symbols https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:105: q = bot_management.BotInfo.query() inline this https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:109: k, v = i.split(':', 1) skip 'id' otherwise this won't scale. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:110: seen.setdefault(k, {})[v] = True seen.setdefault(K, set()).add(v) https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:112: for k, values in seen.iteritems(): sorted() https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:115: values=values.keys())) sorted() https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:116: logging.info('Saw dimensions %s' % dims) s/%/,/ https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:196: ('/internal/cron/aggregate_dimensions', CronDimensionAggregationHandler), include "bot" in the URL to make this clear this is not about tasks https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... File appengine/swarming/handlers_endpoints.py (right): https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:675: logging.warn('%s', request) info https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:676: agg = ndb.Key(bot_management.DimensionAggregation, "current") single quote https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:678: logging.info(dims) don't https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:679: fd = [] use list comprehension https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:684: logging.info(fd) remove https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:686: fleet_dimensions=fd) bot_dimensions? https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/serv... File appengine/swarming/server/bot_management.py (right): https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/serv... appengine/swarming/server/bot_management.py:224: add another empty line
Test added and other issues addressed. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... File appengine/swarming/handlers_backend.py (right): https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:100: class CronDimensionAggregationHandler(webapp2.RequestHandler): On 2016/08/04 at 21:01:15, M-A Ruel wrote: > 2 lines between file level symbols Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:105: q = bot_management.BotInfo.query() On 2016/08/04 at 21:01:15, M-A Ruel wrote: > inline this Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:109: k, v = i.split(':', 1) On 2016/08/04 at 21:01:15, M-A Ruel wrote: > skip 'id' otherwise this won't scale. Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:110: seen.setdefault(k, {})[v] = True On 2016/08/04 at 21:01:15, M-A Ruel wrote: > seen.setdefault(K, set()).add(v) Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:112: for k, values in seen.iteritems(): On 2016/08/04 at 21:01:14, M-A Ruel wrote: > sorted() Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:115: values=values.keys())) On 2016/08/04 at 21:01:15, M-A Ruel wrote: > sorted() Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:116: logging.info('Saw dimensions %s' % dims) On 2016/08/04 at 21:01:15, M-A Ruel wrote: > s/%/,/ Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:196: ('/internal/cron/aggregate_dimensions', CronDimensionAggregationHandler), On 2016/08/04 at 21:01:15, M-A Ruel wrote: > include "bot" in the URL to make this clear this is not about tasks Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... File appengine/swarming/handlers_endpoints.py (right): https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:675: logging.warn('%s', request) On 2016/08/04 at 21:01:15, M-A Ruel wrote: > info Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:676: agg = ndb.Key(bot_management.DimensionAggregation, "current") On 2016/08/04 at 21:01:15, M-A Ruel wrote: > single quote Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:678: logging.info(dims) On 2016/08/04 at 21:01:15, M-A Ruel wrote: > don't Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:679: fd = [] On 2016/08/04 at 21:01:15, M-A Ruel wrote: > use list comprehension Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:684: logging.info(fd) On 2016/08/04 at 21:01:15, M-A Ruel wrote: > remove Done. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:686: fleet_dimensions=fd) On 2016/08/04 at 21:01:15, M-A Ruel wrote: > bot_dimensions? Well, it's not the dimensions for just one bot. bots_dimensions sounds weird. I'll keep it as fleet_dimensions, I think. https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/serv... File appengine/swarming/server/bot_management.py (right): https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/serv... appengine/swarming/server/bot_management.py:224: On 2016/08/04 at 21:01:15, M-A Ruel wrote: > add another empty line Done.
https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... File appengine/swarming/handlers_endpoints.py (right): https://codereview.chromium.org/2212073002/diff/20001/appengine/swarming/hand... appengine/swarming/handlers_endpoints.py:686: fleet_dimensions=fd) On 2016/08/05 14:17:26, kjlubick wrote: > On 2016/08/04 at 21:01:15, M-A Ruel wrote: > > bot_dimensions? > > Well, it's not the dimensions for just one bot. bots_dimensions sounds weird. > I'll keep it as fleet_dimensions, I think. bots_dimensions is better. We don't use the word "fleet" anywhere else. Consistency is more important. On the other hand I'm fine with replacing 'bots' with 'fleet' but that would require more refactoring. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/cron... File appengine/swarming/cron.yaml (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/cron... appengine/swarming/cron.yaml:29: - description: Aggregate all bot dimensions for easier sorting. bots https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... File appengine/swarming/handlers_backend.py (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:102: """Aggregate all bot dimensions (except id) in the fleet.""" Aggregates bots https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:106: seen = {} add: now = utils.utcnow() https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:110: if k == 'id': if k != 'id': seen.setdefault(k, set()).add(v) saves you one line. OMG :P https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:121: dimensions=dims) and save ts=now here so it's at when the query started, not when the entity was saved, which is an important distinction. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... File appengine/swarming/handlers_endpoints_test.py (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_endpoints_test.py:1500: id='current', I'd prefer if you: - simulate two bots polling with different dimensions - ran the cron job explicitly instead of lines 1499-1507, so more code would be exercised. Right now we don't know if the cron job is broken and no way to test it via unit tests. Breakage would be silent. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/serv... File appengine/swarming/server/bot_management.py (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/serv... appengine/swarming/server/bot_management.py:215: dimension = ndb.StringProperty() key = ndb.StringProperty() https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/serv... appengine/swarming/server/bot_management.py:223: ts = ndb.DateTimeProperty(auto_now_add=True) ts = ndb.DateTimeProperty() KEY = ndb.Key(DimensionAggregation, 'current') then refer to DimensionAggregation.KEY.get() everywhere, instead of copy-pasting 'current'. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/swar... File appengine/swarming/swarming_rpcs.py (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/swar... appengine/swarming/swarming_rpcs.py:387: """Returns all the dimensions and dimension possibilities in the fleet.""" include: ts = message_types.DateTimeField(1) which includes then the data was generated. Useful to know if the cron job broke or any other form of inconsistency.
https://codereview.chromium.org/2212073002/diff/80001/appengine/swarming/hand... File appengine/swarming/handlers_backend.py (right): https://codereview.chromium.org/2212073002/diff/80001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:116: for k, values in sorted(seen.iteritems()): you can use a list comprehension here too. https://codereview.chromium.org/2212073002/diff/80001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:119: values=sorted(list(values)))) don't call list(), no need to rasterise the generator. https://codereview.chromium.org/2212073002/diff/80001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:121: aggregate = bot_management.DimensionAggregation.KEY.get() no need to get it, just write a new one
https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/cron... File appengine/swarming/cron.yaml (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/cron... appengine/swarming/cron.yaml:29: - description: Aggregate all bot dimensions for easier sorting. On 2016/08/05 at 14:36:04, M-A Ruel wrote: > bots Done. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... File appengine/swarming/handlers_backend.py (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:102: """Aggregate all bot dimensions (except id) in the fleet.""" On 2016/08/05 at 14:36:05, M-A Ruel wrote: > Aggregates > bots Done. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:106: seen = {} On 2016/08/05 at 14:36:04, M-A Ruel wrote: > add: > now = utils.utcnow() Done. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:110: if k == 'id': On 2016/08/05 at 14:36:04, M-A Ruel wrote: > if k != 'id': > seen.setdefault(k, set()).add(v) > > saves you one line. OMG :P Done. The electrons spared rejoiced. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:121: dimensions=dims) On 2016/08/05 at 14:36:04, M-A Ruel wrote: > and save ts=now here so it's at when the query started, not when the entity was saved, which is an important distinction. Done. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... File appengine/swarming/handlers_endpoints_test.py (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/hand... appengine/swarming/handlers_endpoints_test.py:1500: id='current', On 2016/08/05 at 14:36:05, M-A Ruel wrote: > I'd prefer if you: > - simulate two bots polling with different dimensions > - ran the cron job explicitly > > instead of lines 1499-1507, so more code would be exercised. Right now we don't know if the cron job is broken and no way to test it via unit tests. Breakage would be silent. Done. I kept this test as well as adding a new one. https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/serv... File appengine/swarming/server/bot_management.py (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/serv... appengine/swarming/server/bot_management.py:215: dimension = ndb.StringProperty() On 2016/08/05 at 14:36:05, M-A Ruel wrote: > key = ndb.StringProperty() "Don't name a property "key." This name is reserved for a special property used to store the Model key. Though it may work locally, a property named "key" will prevent deployment to App Engine." https://cloud.google.com/appengine/docs/python/ndb/entity-property-reference https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/serv... appengine/swarming/server/bot_management.py:223: ts = ndb.DateTimeProperty(auto_now_add=True) On 2016/08/05 at 14:36:05, M-A Ruel wrote: > ts = ndb.DateTimeProperty() > > > KEY = ndb.Key(DimensionAggregation, 'current') > > > then refer to DimensionAggregation.KEY.get() everywhere, instead of copy-pasting 'current'. Done. https://codereview.chromium.org/2212073002/diff/80001/appengine/swarming/hand... File appengine/swarming/handlers_backend.py (right): https://codereview.chromium.org/2212073002/diff/80001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:116: for k, values in sorted(seen.iteritems()): On 2016/08/05 at 17:09:20, M-A Ruel wrote: > you can use a list comprehension here too. Done. https://codereview.chromium.org/2212073002/diff/80001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:119: values=sorted(list(values)))) On 2016/08/05 at 17:09:20, M-A Ruel wrote: > don't call list(), no need to rasterise the generator. Done. https://codereview.chromium.org/2212073002/diff/80001/appengine/swarming/hand... appengine/swarming/handlers_backend.py:121: aggregate = bot_management.DimensionAggregation.KEY.get() On 2016/08/05 at 17:09:20, M-A Ruel wrote: > no need to get it, just write a new one Done.
I realized this should be done as part of stats_framework but that would take too much time, so ignore this sentence. :) https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/serv... File appengine/swarming/server/bot_management.py (right): https://codereview.chromium.org/2212073002/diff/60001/appengine/swarming/serv... appengine/swarming/server/bot_management.py:215: dimension = ndb.StringProperty() On 2016/08/05 17:38:58, kjlubick wrote: > On 2016/08/05 at 14:36:05, M-A Ruel wrote: > > key = ndb.StringProperty() > > "Don't name a property "key." This name is reserved for a special property used > to store the Model key. Though it may work locally, a property named "key" will > prevent deployment to App Engine." > https://cloud.google.com/appengine/docs/python/ndb/entity-property-reference Ugh, I forgot about that. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... File appengine/swarming/handlers_backend.py (right): https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_backend.py:115: dims = [bot_management.DimensionValues( unusual alignment. I'd prefer: dims = [ bot_management.DimensionValues(dimension=k, values=sorted(values)) for k, values in sorted(seen.iteritems()) ] IMHO this is an order of magnitude more readable. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_backend.py:121: aggregate=bot_management.DimensionAggregation( technically, you do not need a named variable. I'll just construct and call .put() directly on the unnamed variable. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_backend.py:122: key=bot_management.DimensionAggregation.KEY, +4 https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_backend.py:123: dimensions = dims, no spaces https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... File appengine/swarming/handlers_endpoints.py (right): https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:677: fd = [swarming_rpcs.StringListPair( same about alignment. you could probably inline it (?) https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:681: bots_dimensions=fd) add ts=dims.ts https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... File appengine/swarming/handlers_endpoints_test.py (right): https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1500: key=bot_management.DimensionAggregation.KEY, +4 https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1502: bot_management.DimensionValues(dimension='foo', please align consistently. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1507: aggregate.put() no need for a named variable https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1522: response = self.call_api('dimensions', body={}) you can probably (?) inline the call and this would still fit 80cols. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... File appengine/swarming/handlers_test.py (right): https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:502: aggregate = bot_management.DimensionAggregation.KEY.get() actual = bot_management.DimensionAggregation.KEY.get() https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:504: key=bot_management.DimensionAggregation.KEY, +4 https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:505: dimensions = [ no spacing https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:507: dimension=u'foo', alignment is incorrect. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:510: ts = now) no spacing
https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... File appengine/swarming/handlers_backend.py (right): https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_backend.py:115: dims = [bot_management.DimensionValues( On 2016/08/05 at 18:06:00, M-A Ruel wrote: > unusual alignment. I'd prefer: > > dims = [ > bot_management.DimensionValues(dimension=k, values=sorted(values)) > for k, values in sorted(seen.iteritems()) > ] > > IMHO this is an order of magnitude more readable. Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_backend.py:121: aggregate=bot_management.DimensionAggregation( On 2016/08/05 at 18:06:00, M-A Ruel wrote: > technically, you do not need a named variable. I'll just construct and call .put() directly on the unnamed variable. Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_backend.py:122: key=bot_management.DimensionAggregation.KEY, On 2016/08/05 at 18:06:00, M-A Ruel wrote: > +4 Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_backend.py:123: dimensions = dims, On 2016/08/05 at 18:06:00, M-A Ruel wrote: > no spaces Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... File appengine/swarming/handlers_endpoints.py (right): https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:677: fd = [swarming_rpcs.StringListPair( On 2016/08/05 at 18:06:01, M-A Ruel wrote: > same about alignment. > > you could probably inline it (?) Done. No inline. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:681: bots_dimensions=fd) On 2016/08/05 at 18:06:01, M-A Ruel wrote: > add ts=dims.ts ts added to swarming_rpcs.BotsDimensions as well. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... File appengine/swarming/handlers_endpoints_test.py (right): https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1500: key=bot_management.DimensionAggregation.KEY, On 2016/08/05 at 18:06:01, M-A Ruel wrote: > +4 Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1502: bot_management.DimensionValues(dimension='foo', On 2016/08/05 at 18:06:01, M-A Ruel wrote: > please align consistently. Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1507: aggregate.put() On 2016/08/05 at 18:06:01, M-A Ruel wrote: > no need for a named variable Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1522: response = self.call_api('dimensions', body={}) On 2016/08/05 at 18:06:01, M-A Ruel wrote: > you can probably (?) inline the call and this would still fit 80cols. Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... File appengine/swarming/handlers_test.py (right): https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:502: aggregate = bot_management.DimensionAggregation.KEY.get() On 2016/08/05 at 18:06:01, M-A Ruel wrote: > actual = bot_management.DimensionAggregation.KEY.get() Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:504: key=bot_management.DimensionAggregation.KEY, On 2016/08/05 at 18:06:01, M-A Ruel wrote: > +4 Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:505: dimensions = [ On 2016/08/05 at 18:06:02, M-A Ruel wrote: > no spacing Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:507: dimension=u'foo', On 2016/08/05 at 18:06:02, M-A Ruel wrote: > alignment is incorrect. Done. https://codereview.chromium.org/2212073002/diff/120001/appengine/swarming/han... appengine/swarming/handlers_test.py:510: ts = now) On 2016/08/05 at 18:06:02, M-A Ruel wrote: > no spacing Done.
https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... File appengine/swarming/handlers_endpoints.py (right): https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:675: logging.info('%s', request) not really useful since there's no request, remove. (I just realized) https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:678: swarming_rpcs.StringListPair(key=d.dimension,value=d.values) space after , https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:681: return swarming_rpcs.BotsDimensions( probably fits one line https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... File appengine/swarming/handlers_endpoints_test.py (right): https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1506: values=['gamma', 'delta', 'epsilon']) keep trailing comma. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1508: ts=now it's fine to merge 1508 and 1509 on one line. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... File appengine/swarming/handlers_test.py (right): https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_test.py:506: bot_management.DimensionValues(dimension='foo', weird alignment. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/swa... File appengine/swarming/swarming_rpcs.py (right): https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/swa... appengine/swarming/swarming_rpcs.py:389: ts = message_types.DateTimeField(2) Add: # Time at which this summary was calculated.
https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... File appengine/swarming/handlers_endpoints.py (right): https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:675: logging.info('%s', request) On 2016/08/05 at 18:41:45, M-A Ruel wrote: > not really useful since there's no request, remove. (I just realized) Done. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:678: swarming_rpcs.StringListPair(key=d.dimension,value=d.values) On 2016/08/05 at 18:41:45, M-A Ruel wrote: > space after , Done. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints.py:681: return swarming_rpcs.BotsDimensions( On 2016/08/05 at 18:41:45, M-A Ruel wrote: > probably fits one line Done. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... File appengine/swarming/handlers_endpoints_test.py (right): https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1506: values=['gamma', 'delta', 'epsilon']) On 2016/08/05 at 18:41:45, M-A Ruel wrote: > keep trailing comma. Done. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1508: ts=now On 2016/08/05 at 18:41:46, M-A Ruel wrote: > it's fine to merge 1508 and 1509 on one line. Done. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... File appengine/swarming/handlers_test.py (right): https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/han... appengine/swarming/handlers_test.py:506: bot_management.DimensionValues(dimension='foo', On 2016/08/05 at 18:41:46, M-A Ruel wrote: > weird alignment. Fixed, I think. https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/swa... File appengine/swarming/swarming_rpcs.py (right): https://codereview.chromium.org/2212073002/diff/140001/appengine/swarming/swa... appengine/swarming/swarming_rpcs.py:389: ts = message_types.DateTimeField(2) On 2016/08/05 at 18:41:46, M-A Ruel wrote: > Add: > # Time at which this summary was calculated. Done.
lgtm with 2 style nits https://codereview.chromium.org/2212073002/diff/160001/appengine/swarming/han... File appengine/swarming/handlers_endpoints_test.py (right): https://codereview.chromium.org/2212073002/diff/160001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1503: bot_management.DimensionValues(dimension='foo', bot_management.DimensionValues( dimension='foo', values=['alpha', 'beta']), is the same number of lines and is more readable, since the arguments are on one line instead of spread on two unaligned lines. https://codereview.chromium.org/2212073002/diff/160001/appengine/swarming/han... File appengine/swarming/handlers_test.py (right): https://codereview.chromium.org/2212073002/diff/160001/appengine/swarming/han... appengine/swarming/handlers_test.py:507: dimension='foo', dimension='foo', values=['alpha', 'beta'])
https://codereview.chromium.org/2212073002/diff/160001/appengine/swarming/han... File appengine/swarming/handlers_endpoints_test.py (right): https://codereview.chromium.org/2212073002/diff/160001/appengine/swarming/han... appengine/swarming/handlers_endpoints_test.py:1503: bot_management.DimensionValues(dimension='foo', On 2016/08/05 at 18:55:27, M-A Ruel wrote: > bot_management.DimensionValues( > dimension='foo', values=['alpha', 'beta']), > > is the same number of lines and is more readable, since the arguments are on one line instead of spread on two unaligned lines. Done. https://codereview.chromium.org/2212073002/diff/160001/appengine/swarming/han... File appengine/swarming/handlers_test.py (right): https://codereview.chromium.org/2212073002/diff/160001/appengine/swarming/han... appengine/swarming/handlers_test.py:507: dimension='foo', On 2016/08/05 at 18:55:27, M-A Ruel wrote: > dimension='foo', values=['alpha', 'beta']) Done.
The CQ bit was checked by kjlubick@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2212073002/#ps180001 (title: "Address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3074e6193f3ca510)
The CQ bit was checked by kjlubick@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kjlubick@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add endpoint and cron job to aggregate all dimensions and values BUG=631047 ========== to ========== Add endpoint and cron job to aggregate all dimensions and values BUG=631047 Committed: https://github.com/luci/luci-py/commit/942902ecc1b3cc22b30921a3a9792694e67cccb0 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://github.com/luci/luci-py/commit/942902ecc1b3cc22b30921a3a9792694e67cccb0 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
