Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2391823006:
[Findit] Add iterator and crash_iterator for delta test (Closed)
|
Created:
4 years, 2 months ago by Sharu Jiang Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, chanli, lijeffrey, Martin Barbella Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Findit] Add iterator and crash_iterator for delta test.
BUG=605369
Committed: https://chromium.googlesource.com/infra/infra/+/78a5e1474f30feafe0488ed6e3b0d7791edca139
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address comments. #Patch Set 3 : Fix nits. #
Total comments: 20
Patch Set 4 : Address comments. #
Total comments: 16
Patch Set 5 : Seperate --date argument. #
Total comments: 11
Patch Set 6 : Delete unused imports. #Patch Set 7 : Change wording in README. #
Total comments: 2
Patch Set 8 : Delete README #Patch Set 9 : Update doc strings. #Dependent Patchsets: Messages
Total messages: 44 (22 generated)
Description was changed from ========== [Findit] Add crash_iterator and client_crash_iterator. BUG= ========== to ========== [Findit] Add crash_iterator and client_crash_iterator. BUG=605369 ==========
katesonia@chromium.org changed reviewers: + inferno@chromium.org, mbarbella@google.com, stgao@chromium.org, wrengr@chromium.org
Patchset #1 (id:1) has been deleted
katesonia@chromium.org changed reviewers: - mbarbella@google.com
Description was changed from ========== [Findit] Add crash_iterator and client_crash_iterator. BUG=605369 ========== to ========== [Findit] Add crash_iterator and client_crash_iterator for delta test. BUG=605369 ==========
PTAL :)
https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/README.md:8: ## . setup_enviroments.sh should be "setup_environment"; the plural is weird. But also, no such file exists. I'd suggest `source README.md` except that everything here is commented out. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/README.md:15: ## Since findit don't want to cover the tests of "don't" -> "doesn't" "the tests of"... of what? https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/client_crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:33: def GetCrashInfoFields(client_id): The objects encapsulating the various CrashClients should keep track of their own info-field sets. Putting this stuff here introduces too much coupling and will make it harder to refactor later. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:50: def GetAnalysisClassForClient(client_id): Again, this is too fragile and introduces unnecessary dependencies. There should be a CrashClient class, instances of which know their own id and have a method for returning the appropriate class. Putting all this sort of stuff into such a class (a) makes it obvious who has responsability for doing what, and (b) ensures that client code —like this file— need not worry about the details of how that works. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:71: return query.filter(cls.requested_time >= start_date).filter( Filtering in one pass avoids constructing an intermediate |query|. (Unless you just so happen to have implemented |cls.query| in a special way that automatically fuses the repeated passes.) https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:76: callback_func, Rather than passing a callback function, why not simply define this function (and the underlying (crash_iterator.IterateCrashes) as generators? Using generators leads to much cleaner code, as it avoids callback hell. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_iterator.py:14: def GetCrashInfo(crash, crash_info_fields): This seems very unclean. Why not just use the |crash| directly? If you want certain properties to default to None, then use a class that does so directly. (E.g., have a factory that will construct a class from |crash_info_fields|, and then just pass the |crash| to the constructor for that class.) https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_iterator.py:28: callback_func, See my note about callback functions vs generators in the previous file https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:24: 'If it\' 2016-08-01.., default end_date to today, if it\'s ' -> "If the end date is missing, then it defaults to today. If the start date is missing, then it defaults to a year before the end date." https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:32: help=('Should be one in fracas/cracas/clusterfuzz. Right now, only ' -> "Possible values are: fracas, cracas, clusterfuzz." https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:38: if not start_date: Is this not redundant?
Patchset #3 (id:60001) has been deleted
Description was changed from ========== [Findit] Add crash_iterator and client_crash_iterator for delta test. BUG=605369 ========== to ========== [Findit] Add iterator and crash_iterator for delta test. BUG=605369 ==========
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/README.md:8: ## . setup_enviroments.sh On 2016/10/05 18:41:58, wrengr wrote: > should be "setup_environment"; the plural is weird. > > But also, no such file exists. I'd suggest `source README.md` except that > everything here is commented out. This is the practice I used in delta test in clusterfuzz, so people won't accidentally check in their paths, instead they can make their own local .sh copy. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/README.md:15: ## Since findit don't want to cover the tests of On 2016/10/05 18:41:58, wrengr wrote: > "don't" -> "doesn't" > > "the tests of"... of what? Done. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/client_crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:33: def GetCrashInfoFields(client_id): On 2016/10/05 18:41:58, wrengr wrote: > The objects encapsulating the various CrashClients should keep track of their > own info-field sets. Putting this stuff here introduces too much coupling and > will make it harder to refactor later. There is no need for this function any more, since the channal and historical_metadata can be included in customized_data, just used the COMMON_CRASH_INFO_FIELDS for all the clients. We can still have a function to get the COMMON_CRASH_INFO_FIELDS in the objects encapsulating the various CrashClients, however we don't want that class knowing what info-fields delta test needs to run. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:50: def GetAnalysisClassForClient(client_id): On 2016/10/05 18:41:58, wrengr wrote: > Again, this is too fragile and introduces unnecessary dependencies. There should > be a CrashClient class, instances of which know their own id and have a method > for returning the appropriate class. Putting all this sort of stuff into such a > class (a) makes it obvious who has responsability for doing what, and (b) > ensures that client code —like this file— need not worry about the details of > how that works. Added a TODO. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:71: return query.filter(cls.requested_time >= start_date).filter( On 2016/10/05 18:41:58, wrengr wrote: > Filtering in one pass avoids constructing an intermediate |query|. (Unless you > just so happen to have implemented |cls.query| in a special way that > automatically fuses the repeated passes.) Done. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:76: callback_func, On 2016/10/05 18:41:58, wrengr wrote: > Rather than passing a callback function, why not simply define this function > (and the underlying (crash_iterator.IterateCrashes) as generators? Using > generators leads to much cleaner code, as it avoids callback hell. This is a good idea :) https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_iterator.py:14: def GetCrashInfo(crash, crash_info_fields): On 2016/10/05 18:41:58, wrengr wrote: > This seems very unclean. Why not just use the |crash| directly? If you want > certain properties to default to None, then use a class that does so directly. > (E.g., have a factory that will construct a class from |crash_info_fields|, and > then just pass the |crash| to the constructor for that class.) This is for saving memory especially for the batch_run=True, since there are many fields in the CrashAnalysis that are not used. This crashes will be PIPE to subprocess to run findit, better keep it small. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_iterator.py:28: callback_func, On 2016/10/05 18:41:58, wrengr wrote: > See my note about callback functions vs generators in the previous file Acknowledged. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:24: 'If it\' 2016-08-01.., default end_date to today, if it\'s ' On 2016/10/05 18:41:59, wrengr wrote: > -> "If the end date is missing, then it defaults to today. If the start date is > missing, then it defaults to a year before the end date." Done. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:32: help=('Should be one in fracas/cracas/clusterfuzz. Right now, only ' On 2016/10/05 18:41:58, wrengr wrote: > -> "Possible values are: fracas, cracas, clusterfuzz." Done. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:38: if not start_date: On 2016/10/05 18:41:58, wrengr wrote: > Is this not redundant? Oops, should be _A_YEAR_AGO
Patchset #3 (id:100001) has been deleted
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/README.md:8: ## . setup_enviroments.sh On 2016/10/06 18:16:56, sharu jiang wrote: > On 2016/10/05 18:41:58, wrengr wrote: > > should be "setup_environment"; the plural is weird. > > > > But also, no such file exists. I'd suggest `source README.md` except that > > everything here is commented out. > > This is the practice I used in delta test in clusterfuzz, so people won't > accidentally check in their paths, instead they can make their own local .sh > copy. That makes sense. It'd be clearer if there were ##-comments explaining that users should copy the file to setup_environment.sh and customize the path values, as part of the directions prior to the ##-comment about sourcing it. https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/client_crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/client_crash_iterator.py:50: def GetAnalysisClassForClient(client_id): On 2016/10/06 18:16:56, sharu jiang wrote: > On 2016/10/05 18:41:58, wrengr wrote: > > Again, this is too fragile and introduces unnecessary dependencies. There > should > > be a CrashClient class, instances of which know their own id and have a method > > for returning the appropriate class. Putting all this sort of stuff into such > a > > class (a) makes it obvious who has responsability for doing what, and (b) > > ensures that client code —like this file— need not worry about the details of > > how that works. > > Added a TODO. FWIW, I'm in the progress of combining findit_for_*.py and making the general class that captures all these sorts of things over there. I'll try to publish that CL later today. After one or the other of our CLs land, the other can be modified to update the code here to work with that new organization. In the long run, the general class I'm defining in that CL should move to ./findit/appengine rather than staying in ./findit/azalea, since it depends on ndb specifics. (I'm not sure it'd be profitable to have a variant of the class with the ndb stuff abstracted out.) Before doing that some details about the azalea API still need to be worked out though, since azalea will want to produce/consume objects/tuples which capture much of the same information cached in various ndb.Models.
https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:6: ## pulling data from ClusterFuzz through App Engine Remote API. Command: Why we still pull data from ClusterFuzz? What's the high-level decision here? We are going to port it to app engine soon, right? In that case, all data will be in the app engine app. (We could chat in person for this) https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_iterator.py:13: import zlib Need clean-up. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_iterator.py:55: return cls.query(**property_values).filter( Can we use https://cloud.google.com/appengine/docs/python/ndb/projectionqueries so that those fields don't have to be passed over into the iterator? This could make the interface cleaner. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_iterator.py:73: property_values (dict): Property values to query. nit: "to filter"? https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/crash_printer.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/crash_printer.py:9: import zlib Need clean-up https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: help=('Should be in start_date..end_date format.' Maybe give a real example too. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:40: start_date = _A_YEAR_AGO If the end date is provided, this doesn't conform to the description in #25. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:45: remote_api.EnableRemoteApi(app_id=os.getenv('APP_ID')) It seems better to handle the app_id from user code, and pass it into the function here. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:53: break Why break here? What if `entities` is not empty?
https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:6: ## pulling data from ClusterFuzz through App Engine Remote API. Command: On 2016/10/10 23:39:39, stgao wrote: > Why we still pull data from ClusterFuzz? What's the high-level decision here? > > We are going to port it to app engine soon, right? > In that case, all data will be in the app engine app. > > (We could chat in person for this) Right, forgot to clean up the comments. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_iterator.py:13: import zlib On 2016/10/10 23:39:39, stgao wrote: > Need clean-up. Done. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_iterator.py:55: return cls.query(**property_values).filter( On 2016/10/10 23:39:39, stgao wrote: > Can we use https://cloud.google.com/appengine/docs/python/ndb/projectionqueries > so that those fields don't have to be passed over into the iterator? > This could make the interface cleaner. Problem is only indexed property can be projected, for some properties like stacktrace, we don't want to index them. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_iterator.py:73: property_values (dict): Property values to query. On 2016/10/10 23:39:39, stgao wrote: > nit: "to filter"? Done. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/crash_printer.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/crash_printer.py:9: import zlib On 2016/10/10 23:39:40, stgao wrote: > Need clean-up Done. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: help=('Should be in start_date..end_date format.' On 2016/10/10 23:39:40, stgao wrote: > Maybe give a real example too. Done. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:40: start_date = _A_YEAR_AGO On 2016/10/10 23:39:40, stgao wrote: > If the end date is provided, this doesn't conform to the description in #25. Done. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:45: remote_api.EnableRemoteApi(app_id=os.getenv('APP_ID')) On 2016/10/10 23:39:40, stgao wrote: > It seems better to handle the app_id from user code, and pass it into the > function here. Done. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:53: break On 2016/10/10 23:39:40, stgao wrote: > Why break here? What if `entities` is not empty? https://cloud.google.com/appengine/docs/python/ndb/queryclass more (bool): indicating whether there are (likely) more results after this batch. If False, there are no more results; if True, there are probably more results. So using "if not more" should be safe, it seems using "if not entities" could be the same, however I am not sure, since there is no saying that if this batch is empty, there is no more entities.
https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:53: break On 2016/10/12 00:52:11, sharu jiang wrote: > On 2016/10/10 23:39:40, stgao wrote: > > Why break here? What if `entities` is not empty? > > https://cloud.google.com/appengine/docs/python/ndb/queryclass > more (bool): indicating whether there are (likely) more results after this > batch. If False, there are no more results; if True, there are probably more > results. > > So using "if not more" should be safe, it seems using "if not entities" could be > the same, however I am not sure, since there is no saying that if this batch is > empty, there is no more entities. My question is not whether to break "if not more" or "if not entities". My question is: "if not more", why we should break right away without checking the already-returned "entities" which might have the last set of results? https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: What's the entry point of the delta test? A python script or a bash shell script? Can we make the python script or shell script take care of this setup to avoid manual action if possible? https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:25: default='%s..%s' % (_A_YEAR_AGO, _TODAY), Can we separate the start and end dates into two parameters? https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:50: remote_api.EnableRemoteApi() Why we don'd expect an app id? What's the default one?
Patchset #7 (id:220001) has been deleted
https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: On 2016/10/13 06:38:51, stgao wrote: > What's the entry point of the delta test? A python script or a bash shell > script? > > Can we make the python script or shell script take care of this setup to avoid > manual action if possible? The entry point is a python script. Some user setups are needed since path of the appengine sdk, dir of local checkouts and etcetera varies from user to user. https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:25: default='%s..%s' % (_A_YEAR_AGO, _TODAY), On 2016/10/13 06:38:52, stgao wrote: > Can we separate the start and end dates into two parameters? Done. https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:50: remote_api.EnableRemoteApi() On 2016/10/13 06:38:52, stgao wrote: > Why we don'd expect an app id? What's the default one? I think this iterator can be used not only for crash, it can be potentially used by waterfall, when there is no app_id specified, it just falls back to whatever default value the remote_api has.
https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: On 2016/10/13 20:31:16, sharu jiang wrote: > On 2016/10/13 06:38:51, stgao wrote: > > What's the entry point of the delta test? A python script or a bash shell > > script? > > > > Can we make the python script or shell script take care of this setup to avoid > > manual action if possible? > > The entry point is a python script. > > Some user setups are needed since path of the appengine sdk, Why this can't be taken care by the python script itself? Checkout of the infra repo has a stable dir structure. > dir of local checkouts and etcetera varies from user to user. Can these be input parameters and be part of the help description of the python script? https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:50: remote_api.EnableRemoteApi() On 2016/10/13 20:31:16, sharu jiang wrote: > On 2016/10/13 06:38:52, stgao wrote: > > Why we don'd expect an app id? What's the default one? > > I think this iterator can be used not only for crash, it can be potentially used > by waterfall, when there is no app_id specified, it just falls back to whatever > default value the remote_api has. It could be reused, but this style of default value could make it really hard to debug when error occurs. https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_iterator.py:10: dev_appserver.fix_sys_path() Should this be handled by the caller at the top of the call path instead? https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: '--after', Why we have two names here? https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:31: '--before', same here.
https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_iterator.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_iterator.py:10: dev_appserver.fix_sys_path() On 2016/10/14 00:55:01, stgao wrote: > Should this be handled by the caller at the top of the call path instead? Deleted this, it is legacy import from testing and not needed anymore. https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: '--after', On 2016/10/14 00:55:01, stgao wrote: > Why we have two names here? This is similar to args of git log command: https://git-scm.com/docs/git-log Just give people more options to remember. https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:31: '--before', On 2016/10/14 00:55:01, stgao wrote: > same here. Ditto.
Ping.
https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: On 2016/10/14 00:55:01, stgao wrote: > On 2016/10/13 20:31:16, sharu jiang wrote: > > On 2016/10/13 06:38:51, stgao wrote: > > > What's the entry point of the delta test? A python script or a bash shell > > > script? > > > > > > Can we make the python script or shell script take care of this setup to > avoid > > > manual action if possible? > > > > The entry point is a python script. > > > > Some user setups are needed since path of the appengine sdk, > Why this can't be taken care by the python script itself? Checkout of the infra > repo has a stable dir structure. > > > dir of local checkouts and etcetera varies from user to user. > Can these be input parameters and be part of the help description of the python > script? And ping again https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:50: remote_api.EnableRemoteApi() On 2016/10/14 00:55:01, stgao wrote: > On 2016/10/13 20:31:16, sharu jiang wrote: > > On 2016/10/13 06:38:52, stgao wrote: > > > Why we don'd expect an app id? What's the default one? > > > > I think this iterator can be used not only for crash, it can be potentially > used > > by waterfall, when there is no app_id specified, it just falls back to > whatever > > default value the remote_api has. > > It could be reused, but this style of default value could make it really hard to > debug when error occurs. Ping. https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: '--after', On 2016/10/14 18:14:49, sharu jiang wrote: > On 2016/10/14 00:55:01, stgao wrote: > > Why we have two names here? > > This is similar to args of git log command: > https://git-scm.com/docs/git-log > > Just give people more options to remember. I rarely see such a usage. What's the consideration behind that? Why people want two options? Does it really help out?
Sorry, I missed some comments in old patches. https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/180001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:53: break On 2016/10/13 06:38:51, stgao wrote: > On 2016/10/12 00:52:11, sharu jiang wrote: > > On 2016/10/10 23:39:40, stgao wrote: > > > Why break here? What if `entities` is not empty? > > > > https://cloud.google.com/appengine/docs/python/ndb/queryclass > > more (bool): indicating whether there are (likely) more results after this > > batch. If False, there are no more results; if True, there are probably more > > results. > > > > So using "if not more" should be safe, it seems using "if not entities" could > be > > the same, however I am not sure, since there is no saying that if this batch > is > > empty, there is no more entities. > > My question is not whether to break "if not more" or "if not entities". > My question is: "if not more", why we should break right away without checking > the already-returned "entities" which might have the last set of results? Oops, done. https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: On 2016/10/19 00:49:47, stgao wrote: > On 2016/10/14 00:55:01, stgao wrote: > > On 2016/10/13 20:31:16, sharu jiang wrote: > > > On 2016/10/13 06:38:51, stgao wrote: > > > > What's the entry point of the delta test? A python script or a bash shell > > > > script? > > > > > > > > Can we make the python script or shell script take care of this setup to > > avoid > > > > manual action if possible? > > > > > > The entry point is a python script. > > > > > > Some user setups are needed since path of the appengine sdk, > > Why this can't be taken care by the python script itself? Checkout of the > infra > > repo has a stable dir structure. > > > > > dir of local checkouts and etcetera varies from user to user. > > Can these be input parameters and be part of the help description of the > python > > script? > > And ping again This can be taken care by the python script itself. The reasons why I didn't do it are: 1. For every scripts under crash_queries, we need to add a lot duplicate sys.path.insert and they all need to be changed if the structure changes. 2. For App id and check out dir, if we make those parameters of python script, users need to pass app_id and checkout_dir every time they run the script. To avoid that, they needs to set default value directly in the code (which I don't think is a good practice and may be accidentally checked in). 3. This is the practice for clusterfuzz and findit for clusterfuzz local scripts(including delta test script). We agreed on this practice. https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/iterator.py (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/iterator.py:50: remote_api.EnableRemoteApi() On 2016/10/19 00:49:47, stgao wrote: > On 2016/10/14 00:55:01, stgao wrote: > > On 2016/10/13 20:31:16, sharu jiang wrote: > > > On 2016/10/13 06:38:52, stgao wrote: > > > > Why we don'd expect an app id? What's the default one? > > > > > > I think this iterator can be used not only for crash, it can be potentially > > used > > > by waterfall, when there is no app_id specified, it just falls back to > > whatever > > > default value the remote_api has. > > > > It could be reused, but this style of default value could make it really hard > to > > debug when error occurs. > > Ping. Done. https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: '--after', On 2016/10/19 00:49:47, stgao wrote: > On 2016/10/14 18:14:49, sharu jiang wrote: > > On 2016/10/14 00:55:01, stgao wrote: > > > Why we have two names here? > > > > This is similar to args of git log command: > > https://git-scm.com/docs/git-log > > > > Just give people more options to remember. > > I rarely see such a usage. > > What's the consideration behind that? Why people want two options? Does it > really help out? Maybe when people can not remember clearly whether parameters are (since, until) or (after, before), they can use either of the cases. I am ok either way(2 options or 1), if you think 1 name is better, I can remove the (after, before) names.
katesonia@chromium.org changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org
lg2m if the readme.MD is removed. We don't want to have such setup step in the appengine version. https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: On 2016/10/19 20:12:44, sharu jiang wrote: > On 2016/10/19 00:49:47, stgao wrote: > > On 2016/10/14 00:55:01, stgao wrote: > > > On 2016/10/13 20:31:16, sharu jiang wrote: > > > > On 2016/10/13 06:38:51, stgao wrote: > > > > > What's the entry point of the delta test? A python script or a bash > shell > > > > > script? > > > > > > > > > > Can we make the python script or shell script take care of this setup to > > > avoid > > > > > manual action if possible? > > > > > > > > The entry point is a python script. > > > > > > > > Some user setups are needed since path of the appengine sdk, > > > Why this can't be taken care by the python script itself? Checkout of the > > infra > > > repo has a stable dir structure. > > > > > > > dir of local checkouts and etcetera varies from user to user. > > > Can these be input parameters and be part of the help description of the > > python > > > script? > > > > And ping again > > This can be taken care by the python script itself. The reasons why I didn't do > it are: > 1. For every scripts under crash_queries, we need to add a lot duplicate > sys.path.insert and they all need to be changed if the structure changes. In that case, can we extract and share just like appengine/findit/util_scripts/remote_api.py? Could remote_api.py just be reused here? Structure changes should be rare. We have been using remote_api.py for over a year and a half, and we don't have any trouble at all. > 2. For App id and check out dir, if we make those parameters of python script, > users need to pass app_id and checkout_dir every time they run the script. To > avoid that, they needs to set default value directly in the code (which I don't > think is a good practice and may be accidentally checked in). Have you thought thoroughly on possible alternatives like below? 1. Have reasonable default values to avoid manual input all the time, but also provide message to notify users of the default values. 2. Respect values set command line, and then in env like in .bashrc. Why check out dir can not be derived with relative path from the python script? > 3. This is the practice for clusterfuzz and findit for clusterfuzz local > scripts(including delta test script). We agreed on this practice. I agreed there, but not here :) As we are redesigning, we could rethink. https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: '--after', On 2016/10/19 20:12:44, sharu jiang wrote: > On 2016/10/19 00:49:47, stgao wrote: > > On 2016/10/14 18:14:49, sharu jiang wrote: > > > On 2016/10/14 00:55:01, stgao wrote: > > > > Why we have two names here? > > > > > > This is similar to args of git log command: > > > https://git-scm.com/docs/git-log > > > > > > Just give people more options to remember. > > > > I rarely see such a usage. > > > > What's the consideration behind that? Why people want two options? Does it > > really help out? > > Maybe when people can not remember clearly whether parameters are (since, until) > or (after, before), they can use either of the cases. > > I am ok either way(2 options or 1), if you think 1 name is better, I can remove > the (after, before) names. When people don't remember, they should run "script.py -h". That's the common usage.
Style lgtm https://codereview.chromium.org/2391823006/diff/300001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/300001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:30: default=_TODAY, nit: is there a reason you include the -c shortcut for client but not -u or -s for since/util?
https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: On 2016/10/20 01:18:37, stgao wrote: > On 2016/10/19 20:12:44, sharu jiang wrote: > > On 2016/10/19 00:49:47, stgao wrote: > > > On 2016/10/14 00:55:01, stgao wrote: > > > > On 2016/10/13 20:31:16, sharu jiang wrote: > > > > > On 2016/10/13 06:38:51, stgao wrote: > > > > > > What's the entry point of the delta test? A python script or a bash > > shell > > > > > > script? > > > > > > > > > > > > Can we make the python script or shell script take care of this setup > to > > > > avoid > > > > > > manual action if possible? > > > > > > > > > > The entry point is a python script. > > > > > > > > > > Some user setups are needed since path of the appengine sdk, > > > > Why this can't be taken care by the python script itself? Checkout of the > > > infra > > > > repo has a stable dir structure. > > > > > > > > > dir of local checkouts and etcetera varies from user to user. > > > > Can these be input parameters and be part of the help description of the > > > python > > > > script? > > > > > > And ping again > > > > This can be taken care by the python script itself. The reasons why I didn't > do > > it are: > > 1. For every scripts under crash_queries, we need to add a lot duplicate > > sys.path.insert and they all need to be changed if the structure changes. > > In that case, can we extract and share just like > appengine/findit/util_scripts/remote_api.py? > Could remote_api.py just be reused here? > Structure changes should be rare. We have been using remote_api.py for over a > year and a half, and we don't have any trouble at all. > Yes, those findit_dir, third_party_dir, appengine_dir inserts can be shared, though util_script_dir insert cannot be avoided for every script, but I think this little amount of duplication is acceptable. > > 2. For App id and check out dir, if we make those parameters of python script, > > users need to pass app_id and checkout_dir every time they run the script. To > > avoid that, they needs to set default value directly in the code (which I > don't > > think is a good practice and may be accidentally checked in). > > Have you thought thoroughly on possible alternatives like below? > 1. Have reasonable default values to avoid manual input all the time, but also > provide message to notify users of the default values. I mentioned "To avoid that, they needs to set default value directly in the code". Ok, I can set default values. However the only concern remains right now we are testing in findit-for-me-dev, if we defaults to that, later we have to directly change code to change the default value. > 2. Respect values set command line, and then in env like in .bashrc. > > Why check out dir can not be derived with relative path from the python script? Yes, we can, however we'd better put it in the home directory to avoid checking into repo. > > > 3. This is the practice for clusterfuzz and findit for clusterfuzz local > > scripts(including delta test script). We agreed on this practice. > > I agreed there, but not here :) > As we are redesigning, we could rethink. https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: '--after', On 2016/10/20 01:18:37, stgao wrote: > On 2016/10/19 20:12:44, sharu jiang wrote: > > On 2016/10/19 00:49:47, stgao wrote: > > > On 2016/10/14 18:14:49, sharu jiang wrote: > > > > On 2016/10/14 00:55:01, stgao wrote: > > > > > Why we have two names here? > > > > > > > > This is similar to args of git log command: > > > > https://git-scm.com/docs/git-log > > > > > > > > Just give people more options to remember. > > > > > > I rarely see such a usage. > > > > > > What's the consideration behind that? Why people want two options? Does it > > > really help out? > > > > Maybe when people can not remember clearly whether parameters are (since, > until) > > or (after, before), they can use either of the cases. > > > > I am ok either way(2 options or 1), if you think 1 name is better, I can > remove > > the (after, before) names. > > When people don't remember, they should run "script.py -h". > That's the common usage. Acknowledged. https://codereview.chromium.org/2391823006/diff/300001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/300001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:30: default=_TODAY, On 2016/10/20 15:35:46, lijeffrey wrote: > nit: is there a reason you include the -c shortcut for client but not -u or -s > for since/util? Done.
Patchset #11 (id:320001) has been deleted
lgtm https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: On 2016/10/20 20:38:20, sharu jiang wrote: > On 2016/10/20 01:18:37, stgao wrote: > > On 2016/10/19 20:12:44, sharu jiang wrote: > > > On 2016/10/19 00:49:47, stgao wrote: > > > > On 2016/10/14 00:55:01, stgao wrote: > > > > > On 2016/10/13 20:31:16, sharu jiang wrote: > > > > > > On 2016/10/13 06:38:51, stgao wrote: > > > > > > > What's the entry point of the delta test? A python script or a bash > > > shell > > > > > > > script? > > > > > > > > > > > > > > Can we make the python script or shell script take care of this > setup > > to > > > > > avoid > > > > > > > manual action if possible? > > > > > > > > > > > > The entry point is a python script. > > > > > > > > > > > > Some user setups are needed since path of the appengine sdk, > > > > > Why this can't be taken care by the python script itself? Checkout of > the > > > > infra > > > > > repo has a stable dir structure. > > > > > > > > > > > dir of local checkouts and etcetera varies from user to user. > > > > > Can these be input parameters and be part of the help description of the > > > > python > > > > > script? > > > > > > > > And ping again > > > > > > This can be taken care by the python script itself. The reasons why I didn't > > do > > > it are: > > > 1. For every scripts under crash_queries, we need to add a lot duplicate > > > sys.path.insert and they all need to be changed if the structure changes. > > > > In that case, can we extract and share just like > > appengine/findit/util_scripts/remote_api.py? > > Could remote_api.py just be reused here? > > Structure changes should be rare. We have been using remote_api.py for over a > > year and a half, and we don't have any trouble at all. > > > Yes, those findit_dir, third_party_dir, appengine_dir inserts can be shared, > though util_script_dir insert cannot be avoided for every script, but I think > this little amount of duplication is acceptable. > > > > 2. For App id and check out dir, if we make those parameters of python > script, > > > users need to pass app_id and checkout_dir every time they run the script. > To > > > avoid that, they needs to set default value directly in the code (which I > > don't > > > think is a good practice and may be accidentally checked in). > > > > Have you thought thoroughly on possible alternatives like below? > > 1. Have reasonable default values to avoid manual input all the time, but also > > provide message to notify users of the default values. > > I mentioned "To avoid that, they needs to set default value directly in the > code". Ok, I can set default values. However the only concern remains right now > we are testing in findit-for-me-dev, if we defaults to that, later we have to > directly change code to change the default value. We could check environment setting as in my last comment for such values before using the default values. > > > 2. Respect values set command line, and then in env like in .bashrc. > > > > Why check out dir can not be derived with relative path from the python > script? > > Yes, we can, however we'd better put it in the home directory to avoid checking > into repo. > > > > > > 3. This is the practice for clusterfuzz and findit for clusterfuzz local > > > scripts(including delta test script). We agreed on this practice. > > > > I agreed there, but not here :) > > As we are redesigning, we could rethink. >
https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/README.md (right): https://codereview.chromium.org/2391823006/diff/200001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/README.md:8: ## pulling data from datastore App Engine Remote API. Command: On 2016/10/21 01:36:34, stgao wrote: > On 2016/10/20 20:38:20, sharu jiang wrote: > > On 2016/10/20 01:18:37, stgao wrote: > > > On 2016/10/19 20:12:44, sharu jiang wrote: > > > > On 2016/10/19 00:49:47, stgao wrote: > > > > > On 2016/10/14 00:55:01, stgao wrote: > > > > > > On 2016/10/13 20:31:16, sharu jiang wrote: > > > > > > > On 2016/10/13 06:38:51, stgao wrote: > > > > > > > > What's the entry point of the delta test? A python script or a > bash > > > > shell > > > > > > > > script? > > > > > > > > > > > > > > > > Can we make the python script or shell script take care of this > > setup > > > to > > > > > > avoid > > > > > > > > manual action if possible? > > > > > > > > > > > > > > The entry point is a python script. > > > > > > > > > > > > > > Some user setups are needed since path of the appengine sdk, > > > > > > Why this can't be taken care by the python script itself? Checkout of > > the > > > > > infra > > > > > > repo has a stable dir structure. > > > > > > > > > > > > > dir of local checkouts and etcetera varies from user to user. > > > > > > Can these be input parameters and be part of the help description of > the > > > > > python > > > > > > script? > > > > > > > > > > And ping again > > > > > > > > This can be taken care by the python script itself. The reasons why I > didn't > > > do > > > > it are: > > > > 1. For every scripts under crash_queries, we need to add a lot duplicate > > > > sys.path.insert and they all need to be changed if the structure changes. > > > > > > In that case, can we extract and share just like > > > appengine/findit/util_scripts/remote_api.py? > > > Could remote_api.py just be reused here? > > > Structure changes should be rare. We have been using remote_api.py for over > a > > > year and a half, and we don't have any trouble at all. > > > > > Yes, those findit_dir, third_party_dir, appengine_dir inserts can be shared, > > though util_script_dir insert cannot be avoided for every script, but I think > > this little amount of duplication is acceptable. > > > > > > 2. For App id and check out dir, if we make those parameters of python > > script, > > > > users need to pass app_id and checkout_dir every time they run the script. > > To > > > > avoid that, they needs to set default value directly in the code (which I > > > don't > > > > think is a good practice and may be accidentally checked in). > > > > > > Have you thought thoroughly on possible alternatives like below? > > > 1. Have reasonable default values to avoid manual input all the time, but > also > > > provide message to notify users of the default values. > > > > I mentioned "To avoid that, they needs to set default value directly in the > > code". Ok, I can set default values. However the only concern remains right > now > > we are testing in findit-for-me-dev, if we defaults to that, later we have to > > directly change code to change the default value. > > We could check environment setting as in my last comment for such values before > using the default values. > > > > > > 2. Respect values set command line, and then in env like in .bashrc. > > > > > > Why check out dir can not be derived with relative path from the python > > script? > > > > Yes, we can, however we'd better put it in the home directory to avoid > checking > > into repo. > > > > > > > > > 3. This is the practice for clusterfuzz and findit for clusterfuzz local > > > > scripts(including delta test script). We agreed on this practice. > > > > > > I agreed there, but not here :) > > > As we are redesigning, we could rethink. > > > Done.
Patchset #12 (id:340002) has been deleted
Patchset #12 (id:370001) has been deleted
Patchset #12 (id:390001) has been deleted
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lijeffrey@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2391823006/#ps410001 (title: "Update doc strings.")
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 ========== [Findit] Add iterator and crash_iterator for delta test. BUG=605369 ========== to ========== [Findit] Add iterator and crash_iterator for delta test. BUG=605369 Committed: https://chromium.googlesource.com/infra/infra/+/78a5e1474f30feafe0488ed6e3b0d... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:410001) as https://chromium.googlesource.com/infra/infra/+/78a5e1474f30feafe0488ed6e3b0d...
Message was sent while issue was closed.
https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... File appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py (right): https://codereview.chromium.org/2391823006/diff/240001/appengine/findit/util_... appengine/findit/util_scripts/crash_queries/crash_printer/print_crash.py:23: '--after', On 2016/10/20 01:18:37, stgao wrote: > On 2016/10/19 20:12:44, sharu jiang wrote: > > On 2016/10/19 00:49:47, stgao wrote: > > > On 2016/10/14 18:14:49, sharu jiang wrote: > > > > On 2016/10/14 00:55:01, stgao wrote: > > > > > Why we have two names here? > > > > > > > > This is similar to args of git log command: > > > > https://git-scm.com/docs/git-log > > > > > > > > Just give people more options to remember. > > > > > > I rarely see such a usage. > > > > > > What's the consideration behind that? Why people want two options? Does it > > > really help out? > > > > Maybe when people can not remember clearly whether parameters are (since, > until) > > or (after, before), they can use either of the cases. > > > > I am ok either way(2 options or 1), if you think 1 name is better, I can > remove > > the (after, before) names. > > When people don't remember, they should run "script.py -h". > That's the common usage. FWIW, I agree with stgao here. Having multiple ways to spell a flag/argument is generally a bad idea. For standard *nix tools, the only time they have redundant flags is for legacy reasons, and they typically mark one set as being deprecated. Since we have no such legacy to support, we should keep it clean. Git has both a terrible UI, and one that isn't mirrored by the rest of the ecosystem; there's no reason to try to emulate it. Just have a -h, --help, and/or -? flag which people can query if they forget.
Message was sent while issue was closed.
Patchset #4 (id:160001) has been deleted
Message was sent while issue was closed.
Patchset #8 (id:280001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:40001) has been deleted |
