Issue 223140043: Support for date filters. (Closed)
|
Created:
10 years, 1 month ago by cmassaro Modified:
10 years, 1 month ago CC:
swarming-eng_googlegroups.com Base URL:
https://code.google.com/p/swarming@master Visibility:
Public. |
DescriptionSupport for date filters.
R=maruel@chromium.org, vadimsh@chromium.org
BUG=
Committed: https://code.google.com/p/swarming/source/detail?repo=default&r=a838e3a24c5076e027caa318f7b5fe4f736cb460
Patch Set 1 #
Total comments: 24
Patch Set 2 : Code Cleanup, Tighter Handling of Defaults #Patch Set 3 : Removed Change to index.yaml #
Total comments: 25
Patch Set 4 : Better Error Messages #
Total comments: 12
Patch Set 5 : Code Cleanup and Lint #
Total comments: 4
Patch Set 6 : Final Cleanup #
MessagesTotal messages: 11
https://codereview.appspot.com/223140043/diff/1/appengine/swarming/index.yaml File appengine/swarming/index.yaml (right): https://codereview.appspot.com/223140043/diff/1/appengine/swarming/index.yaml... appengine/swarming/index.yaml:41: - name: __key__ Not needed, use asc instead https://codereview.appspot.com/223140043/diff/1/appengine/swarming/server/tas... File appengine/swarming/server/task_result.py (right): https://codereview.appspot.com/223140043/diff/1/appengine/swarming/server/tas... appengine/swarming/server/task_result.py:927: suffix = random.getrandbits(16) Remove, no randomness is needed here. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/server/tas... appengine/swarming/server/task_result.py:982: tags_filter = reduce(ndb.AND, [ Technically, you could simply do: for tag in task_tags: query = query.filter(task_request.TaskRequest.tags == tag) I think it'd be easier to understand. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/server/tas... appengine/swarming/server/task_result.py:987: state_filter = task_request.TaskRequest.state == state TaskRequest doesn't have a .state. You can skip support and just refuse anything but all for now as an initial implementation. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... File appengine/swarming/swarming_rpcs.py (right): https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:81: end = message_types.DateTimeField(2, required=True) it's not required, it'll default to Now. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:84: limit = messages.IntegerField(3, default=1000) This is actually batch_size https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:85: start = message_types.DateTimeField(4, required=True) Do not require it, just use end-24h when not specified by default https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:136: state = messages.StringField(3) It's not needed? https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:159: limit = messages.IntegerField(3, default=1000) It's not really a limit, more of a batch_size. We should rename accordingly. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:164: bot_id = messages.StringField(1) Put it later, so that the order of the initial elements are similar between BotTasksRequest and TasksRequest. bot_id is required. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:165: end = message_types.DateTimeField(2, required=True) Not required, defaults to now https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:167: limit = messages.IntegerField(4, default=1000) batch_size https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:168: start = message_types.DateTimeField(5, required=True) Not required, defaults to end-24h
Sign in to reply to this message.
https://codereview.appspot.com/223140043/diff/1/appengine/swarming/server/tas... File appengine/swarming/server/task_result.py (right): https://codereview.appspot.com/223140043/diff/1/appengine/swarming/server/tas... appengine/swarming/server/task_result.py:927: suffix = random.getrandbits(16) On 2015/04/03 01:37:00, M-A wrote: > Remove, no randomness is needed here. Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/server/tas... appengine/swarming/server/task_result.py:982: tags_filter = reduce(ndb.AND, [ On 2015/04/03 01:37:00, M-A wrote: > Technically, you could simply do: > for tag in task_tags: > query = query.filter(task_request.TaskRequest.tags == tag) > > I think it'd be easier to understand. Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/server/tas... appengine/swarming/server/task_result.py:987: state_filter = task_request.TaskRequest.state == state On 2015/04/03 01:37:00, M-A wrote: > TaskRequest doesn't have a .state. You can skip support and just refuse anything > but all for now as an initial implementation. Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... File appengine/swarming/swarming_rpcs.py (right): https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:81: end = message_types.DateTimeField(2, required=True) On 2015/04/03 01:37:00, M-A wrote: > it's not required, it'll default to Now. Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:84: limit = messages.IntegerField(3, default=1000) On 2015/04/03 01:37:00, M-A wrote: > This is actually batch_size Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:85: start = message_types.DateTimeField(4, required=True) On 2015/04/03 01:37:00, M-A wrote: > Do not require it, just use end-24h when not specified by default Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:159: limit = messages.IntegerField(3, default=1000) On 2015/04/03 01:37:00, M-A wrote: > It's not really a limit, more of a batch_size. We should rename accordingly. Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:164: bot_id = messages.StringField(1) On 2015/04/03 01:37:00, M-A wrote: > Put it later, so that the order of the initial elements are similar between > BotTasksRequest and TasksRequest. > > bot_id is required. Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:165: end = message_types.DateTimeField(2, required=True) On 2015/04/03 01:37:00, M-A wrote: > Not required, defaults to now Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:167: limit = messages.IntegerField(4, default=1000) On 2015/04/03 01:37:00, M-A wrote: > batch_size Done. https://codereview.appspot.com/223140043/diff/1/appengine/swarming/swarming_r... appengine/swarming/swarming_rpcs.py:168: start = message_types.DateTimeField(5, required=True) On 2015/04/03 01:37:00, M-A wrote: > Not required, defaults to end-24h Done.
Sign in to reply to this message.
Hey, all, I just wanted to see whether this looked reasonable. Tests are passing and such. I did make a few possibly questionable decisions: 1) bots/list just lists all bots. I wasn't sure whether creation date was a meaningful way to limit queries. However, if we expect the number of bots to increase frequently, there should be some way to limit the number returned; should we parameterize batch_size and just order by creation date? 2) I've placed the responsibility for supplying reasonable defaults in the server code, so handlers_endpoints.py uses the current date as the end date and one day ago as the start date for all applicable queries. I'm removing the corresponding code in the client. Let me know if that's the wrong way to do it.
Sign in to reply to this message.
minor things. There was 4 holidays in Canada so I was OOO. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... File appengine/swarming/handlers_endpoints.py (right): https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:87: def _default_start(): accept an end argument https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:180: 'Only one of name, tag (1 or many) or state can be used.') 'Only one of tag (1 or many) or state can be used.') But in practice, this should be supported eventually. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:186: request.end or utils.utcnow(), state, request.batch_size) end = request.end or utils.utcnow() so you can use _default_start(end) https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:193: 'Inappropriate batch_size for task list request: ' 'Inappropriate batch_size for task list request: %s' % e ? https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:225: request.cursor, request.bot_id, request.start or _default_start(), If end is specified but not start, the code won't do the right thing. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/server... File appengine/swarming/server/task_result.py (right): https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/server... appengine/swarming/server/task_result.py:927: # Using 255 << 4, we ensure a closed bound on end_date and an open bound Frankly, I prefer just specifying 0. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/server... appengine/swarming/server/task_result.py:930: return task_request.request_id_to_key(int(request_id_base | 255 << 4 | 0x1)) Use 0xff0 instead of 255<<4 https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/server... appengine/swarming/server/task_result.py:936: raise ValueError('Inappropriate value for batch_size.') must be between 1 and 1000 ? https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... File appengine/swarming/swarming_rpcs.py (right): https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:83: # Default is not supported for DateTime, so the client will have to supply It's not necessary, so remove the comment. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:164: end = message_types.DateTimeField(2) Move cursor before end to be coherent with the rest https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:190: death_timeout = messages.IntegerField(2) Move after now to be similar with BotTask? https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:221: class BotTask(messages.Message): It's actually BotTasks? https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:224: now = message_types.DateTimeField(4) Move to 3
Sign in to reply to this message.
Requested changes have been made. Marc-Antoine, I hope you had an excellent holiday! https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... File appengine/swarming/handlers_endpoints.py (right): https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:87: def _default_start(): On 2015/04/07 18:12:29, M-A wrote: > accept an end argument Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:180: 'Only one of name, tag (1 or many) or state can be used.') On 2015/04/07 18:12:29, M-A wrote: > 'Only one of tag (1 or many) or state can be used.') > > But in practice, this should be supported eventually. Acknowledged. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:186: request.end or utils.utcnow(), state, request.batch_size) On 2015/04/07 18:12:29, M-A wrote: > end = request.end or utils.utcnow() > so you can use _default_start(end) Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:193: 'Inappropriate batch_size for task list request: ' On 2015/04/07 18:12:29, M-A wrote: > 'Inappropriate batch_size for task list request: %s' % e > ? Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:225: request.cursor, request.bot_id, request.start or _default_start(), On 2015/04/07 18:12:29, M-A wrote: > If end is specified but not start, the code won't do the right thing. Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/server... File appengine/swarming/server/task_result.py (right): https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/server... appengine/swarming/server/task_result.py:927: # Using 255 << 4, we ensure a closed bound on end_date and an open bound On 2015/04/07 18:12:29, M-A wrote: > Frankly, I prefer just specifying 0. Hmm, so get rid of the bit mask altogether? https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/server... appengine/swarming/server/task_result.py:936: raise ValueError('Inappropriate value for batch_size.') On 2015/04/07 18:12:29, M-A wrote: > must be between 1 and 1000 ? Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... File appengine/swarming/swarming_rpcs.py (right): https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:83: # Default is not supported for DateTime, so the client will have to supply On 2015/04/07 18:12:29, M-A wrote: > It's not necessary, so remove the comment. Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:164: end = message_types.DateTimeField(2) On 2015/04/07 18:12:29, M-A wrote: > Move cursor before end to be coherent with the rest Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:190: death_timeout = messages.IntegerField(2) On 2015/04/07 18:12:29, M-A wrote: > Move after now to be similar with BotTask? Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:221: class BotTask(messages.Message): On 2015/04/07 18:12:29, M-A wrote: > It's actually BotTasks? Done. https://codereview.appspot.com/223140043/diff/40001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:224: now = message_types.DateTimeField(4) On 2015/04/07 18:12:29, M-A wrote: > Move to 3 Done.
Sign in to reply to this message.
Requested changes have been made. Marc-Antoine, I hope you had an excellent holiday!
Sign in to reply to this message.
Yes, holidays were great, lots of saturated fat filled industrial chocolate due to children not liking dark chocolate. A few minor details I missed earlier; https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/handle... File appengine/swarming/handlers_endpoints.py (right): https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:87: def _default_start(end): Can you add a docstring about the fact it relates to certain message types that accept a datetime range. I'm almost tempted to instead ask for a function that accepts a message that contains .start and .end and returns both, e.g. call sites would look like: start, end = _get_range(request) I think it'd be more readable overall. https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... File appengine/swarming/server/task_result.py (right): https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:927: # Using 255 << 4, we ensure a closed bound on end_date and an open bound Comment is stale https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:933: def get_run_results(cursor_str, bot_id, start_date, end_date, batch_size=1000): I'd prefer no default argument for batch_size https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:937: query = TaskRunResult.query() query = TaskRunResult.query(TaskRunResult.bot_id == bot_id) https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:939: if start_date is not None: it will always be not none? https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:973: query = query.filter( same as above, e.g.: # Inequalities are <= and >= because keys are in reverse chronological order. query = task_request.TaskRequest.query( task_request.TaskRequest.key <= _convert_to_request_key(start_date), task_request.TaskRequest.key >= _convert_to_request_key(end_date)) https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:977: # Inequalities are <= and >= because keys are in reverse chronological order. It's weird that the comment is after the relevant code
Sign in to reply to this message.
https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/handle... File appengine/swarming/handlers_endpoints.py (right): https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/handle... appengine/swarming/handlers_endpoints.py:87: def _default_start(end): On 2015/04/08 00:30:44, M-A wrote: > Can you add a docstring about the fact it relates to certain message types that > accept a datetime range. > > I'm almost tempted to instead ask for a function that accepts a message that > contains .start and .end and returns both, e.g. call sites would look like: > > start, end = _get_range(request) > > I think it'd be more readable overall. Done. https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... File appengine/swarming/server/task_result.py (right): https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:933: def get_run_results(cursor_str, bot_id, start_date, end_date, batch_size=1000): On 2015/04/08 00:30:44, M-A wrote: > I'd prefer no default argument for batch_size Done. https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:939: if start_date is not None: On 2015/04/08 00:30:44, M-A wrote: > it will always be not none? Done. https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:973: query = query.filter( On 2015/04/08 00:30:44, M-A wrote: > same as above, e.g.: > > # Inequalities are <= and >= because keys are in reverse chronological order. > query = task_request.TaskRequest.query( > task_request.TaskRequest.key <= _convert_to_request_key(start_date), > task_request.TaskRequest.key >= _convert_to_request_key(end_date)) Done. https://codereview.appspot.com/223140043/diff/60001/appengine/swarming/server... appengine/swarming/server/task_result.py:977: # Inequalities are <= and >= because keys are in reverse chronological order. On 2015/04/08 00:30:44, M-A wrote: > It's weird that the comment is after the relevant code Done.
Sign in to reply to this message.
lgtm https://codereview.appspot.com/223140043/diff/80001/appengine/swarming/server... File appengine/swarming/server/task_result.py (right): https://codereview.appspot.com/223140043/diff/80001/appengine/swarming/server... appengine/swarming/server/task_result.py:934: query = TaskRunResult.query(TaskRunResult.bot_id == bot_id) query = TaskRunResult.query( TaskRunResult.bot_id == bot_id, TaskRunResult.key <= _convert_to_request_key(start_date), TaskRunResult.key >= _convert_to_request_key(end_date)) so it's similar to line 967 https://codereview.appspot.com/223140043/diff/80001/appengine/swarming/swarmi... File appengine/swarming/swarming_rpcs.py (right): https://codereview.appspot.com/223140043/diff/80001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:80: batch_size = messages.IntegerField(1, default=1000) Experience tells me 200 is a better default. https://codereview.appspot.com/223140043/diff/80001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:155: batch_size = messages.IntegerField(1, default=1000) 200 https://codereview.appspot.com/223140043/diff/80001/appengine/swarming/swarmi... appengine/swarming/swarming_rpcs.py:161: batch_size = messages.IntegerField(1, default=1000) 200 here too
Sign in to reply to this message.
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as a838e3a24c5076e027caa318f7b5fe4f736cb460 (presubmit successful).
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
