Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168173003:
[Findit] Post-process findit-to-fracas results. (Closed)
|
Created:
4 years, 5 months ago by Sharu Jiang Modified:
4 years, 3 months ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, stgao Base URL:
https://chromium.googlesource.com/infra/infra.git@2-face Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Findit]Post-process findit-to-fracas results.
(1) Add feedback_url to crash analysis.
(2) Delete 'reason' in each suspected cl.
BUG=624928, 609621
Committed: https://chromium.googlesource.com/infra/infra/+/e9c78ef32eb9f14eda83c20b8e47721b31ec3265
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase. #
Total comments: 1
Depends on Patchset: Messages
Total messages: 16 (6 generated)
Description was changed from ========== Post process results. BUG= ========== to ========== [Findit]Post-process findit-to-fracas results. (1) Add feedback_url to crash analysis. (2) Delete 'reason' in each suspected cl. BUG=624928, 609621 ==========
katesonia@chromium.org changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org
PTAL :)
https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... File appengine/findit/crash/fracas_crash_pipeline.py (right): https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... appengine/findit/crash/fracas_crash_pipeline.py:90: if analysis_result['found']: I'm not familiar with the data structure you're using, but will analysis_result always have key 'found'? Do you need to change it to if analysis_result.get('found')? https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... appengine/findit/crash/fracas_crash_pipeline.py:104: result = self.PostProcessResults(analysis, crash_identifiers) Don't you want to update analysis.result?
style lgtm but not 100% sure about functionality
https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... File appengine/findit/crash/fracas_crash_pipeline.py (right): https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... appengine/findit/crash/fracas_crash_pipeline.py:90: if analysis_result['found']: On 2016/07/26 22:47:54, chanli wrote: > I'm not familiar with the data structure you're using, but will analysis_result > always have key 'found'? Do you need to change it to > > if analysis_result.get('found')? It always has 'found', if not, there may be errors, in that case, it's better to pop it out here. https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... appengine/findit/crash/fracas_crash_pipeline.py:104: result = self.PostProcessResults(analysis, crash_identifiers) On 2016/07/26 22:47:54, chanli wrote: > Don't you want to update analysis.result? I also need analysis.client_id to construct final results.
https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... File appengine/findit/crash/fracas_crash_pipeline.py (right): https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... appengine/findit/crash/fracas_crash_pipeline.py:104: result = self.PostProcessResults(analysis, crash_identifiers) On 2016/07/26 23:03:48, sharu jiang wrote: > On 2016/07/26 22:47:54, chanli wrote: > > Don't you want to update analysis.result? > > I also need analysis.client_id to construct final results. I don't get it... My question is after you post-process result, don't you want to save it back to analysis.result? Or you need the original analysis.result for something else?
https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... File appengine/findit/crash/fracas_crash_pipeline.py (right): https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... appengine/findit/crash/fracas_crash_pipeline.py:104: result = self.PostProcessResults(analysis, crash_identifiers) On 2016/07/28 17:47:22, chanli wrote: > On 2016/07/26 23:03:48, sharu jiang wrote: > > On 2016/07/26 22:47:54, chanli wrote: > > > Don't you want to update analysis.result? > > > > I also need analysis.client_id to construct final results. > > I don't get it... My question is after you post-process result, don't you want > to save it back to analysis.result? Or you need the original analysis.result for > something else? The original analysis is used for internal monitoring and contains more information (some of them are not useful for Fracas).
On 2016/07/28 17:49:13, sharu jiang wrote: > https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... > File appengine/findit/crash/fracas_crash_pipeline.py (right): > > https://codereview.chromium.org/2168173003/diff/1/appengine/findit/crash/frac... > appengine/findit/crash/fracas_crash_pipeline.py:104: result = > self.PostProcessResults(analysis, crash_identifiers) > On 2016/07/28 17:47:22, chanli wrote: > > On 2016/07/26 23:03:48, sharu jiang wrote: > > > On 2016/07/26 22:47:54, chanli wrote: > > > > Don't you want to update analysis.result? > > > > > > I also need analysis.client_id to construct final results. > > > > I don't get it... My question is after you post-process result, don't you want > > to save it back to analysis.result? Or you need the original analysis.result > for > > something else? > > The original analysis is used for internal monitoring and contains more > information (some of them are not useful for Fracas). lgtm
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2168173003/#ps20001 (title: "Rebase.")
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]Post-process findit-to-fracas results. (1) Add feedback_url to crash analysis. (2) Delete 'reason' in each suspected cl. BUG=624928, 609621 ========== to ========== [Findit]Post-process findit-to-fracas results. (1) Add feedback_url to crash analysis. (2) Delete 'reason' in each suspected cl. BUG=624928, 609621 Committed: https://chromium.googlesource.com/infra/infra/+/e9c78ef32eb9f14eda83c20b8e477... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/infra/infra/+/e9c78ef32eb9f14eda83c20b8e477...
Message was sent while issue was closed.
stgao@chromium.org changed reviewers: + stgao@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2168173003/diff/20001/appengine/findit/crash/... File appengine/findit/crash/fracas_crash_pipeline.py (right): https://codereview.chromium.org/2168173003/diff/20001/appengine/findit/crash/... appengine/findit/crash/fracas_crash_pipeline.py:27: _FINDIT_FEEDBACK_URL_TEMPLATE = ('https://findit-for-me.googleplex.com/crash/' The hostname should go to the config instead. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
