Closed
Bug 783054
Opened 12 years ago
Closed 12 years ago
ping files that can't be read should be deleted
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(4 files, 1 obsolete file)
2.04 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
4.52 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
Currently they'll just sit around and sit around and we'll keep error on reading them.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #652172 -
Flags: review?(taras.mozilla)
Comment 2•12 years ago
|
||
Comment on attachment 652172 [details] [diff] [review] patch 99% sure this .remove will fail on windows since the file is still open. Should add a testcase
Attachment #652172 -
Flags: review?(taras.mozilla) → review-
Comment 3•12 years ago
|
||
Since the stream is closed, this _might_ work. However, indeed, a testcase should remove all doubt.
Assignee | ||
Comment 4•12 years ago
|
||
We're going to need the ability to write fake ping files in test_TelemetryPing.js; we might as well have a function for just doing it.
Attachment #652172 -
Attachment is obsolete: true
Attachment #663408 -
Flags: review?(taras.mozilla)
Assignee | ||
Comment 5•12 years ago
|
||
Closing streams appears to be idempotent, so making sure the stream is closed prior to deleting the file shouldn't cause any problems.
Attachment #663409 -
Flags: review?(taras.mozilla)
Assignee | ||
Comment 6•12 years ago
|
||
This became much easier when I realized I didn't have to shoehorn it into the ping/response logic earlier in the file.
Updated•12 years ago
|
Attachment #663408 -
Flags: review?(taras.mozilla) → review+
Comment 7•12 years ago
|
||
Comment on attachment 663409 [details] [diff] [review] part 2 - delete ping files if there's an error whilst reading them This is good. We should however record success/fail ratios here.
Attachment #663409 -
Flags: review?(taras.mozilla) → review+
Comment 8•12 years ago
|
||
Comment on attachment 663410 [details] [diff] [review] part 3 - add tests This looks good, but as I mentioned above we should record success/failure in a histogram and check the histogram gets recorded correctly in the testcase. Please post that as a followup patch.
Attachment #663410 -
Flags: review?(taras.mozilla) → review+
Assignee | ||
Comment 9•12 years ago
|
||
I can add more complete checking of the contents of the histogram if you like, but it didn't seem worth it.
Attachment #664569 -
Flags: review?(taras.mozilla)
Updated•12 years ago
|
Attachment #664569 -
Flags: review?(taras.mozilla) → review+
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ddcdc6f0619 https://hg.mozilla.org/integration/mozilla-inbound/rev/624c912580fb https://hg.mozilla.org/integration/mozilla-inbound/rev/08764617f060 https://hg.mozilla.org/integration/mozilla-inbound/rev/96fd99a249cd
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ddcdc6f0619 https://hg.mozilla.org/mozilla-central/rev/624c912580fb https://hg.mozilla.org/mozilla-central/rev/08764617f060 https://hg.mozilla.org/mozilla-central/rev/96fd99a249cd
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•