Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

1609149 - Remove ping_type from pings. #653

Merged
merged 4 commits into from
Feb 12, 2020
Merged

1609149 - Remove ping_type from pings. #653

merged 4 commits into from
Feb 12, 2020

Conversation

badboy
Copy link
Member

@badboy badboy commented Jan 15, 2020

@badboy badboy marked this pull request as ready for review January 24, 2020 13:37
@auto-assign auto-assign bot requested a review from travis79 January 24, 2020 13:38
@badboy badboy requested a review from Dexterp37 January 24, 2020 13:38
Dexterp37
Dexterp37 requested changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema change was merged. Does it need to be deployed as well? Is it already live?

glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt Show resolved Hide resolved
@badboy badboy changed the title Remove ping_type from pings. 1609149 - Remove ping_type from pings. Jan 24, 2020
@badboy
Copy link
Member Author

badboy commented Jan 24, 2020

The schema change was merged. Does it need to be deployed as well? Is it already live?

I'll check if it is live with the team. This only becomes relevant once this lands in a release used by applications though.

I'm adding a changelog entry, so we don't forget.

@codecov-io
Copy link

codecov-io commented Jan 24, 2020 •
edited
Loading

Codecov Report

Merging #653 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #653   +/-   ##
=========================================
  Coverage     75.52%   75.52%           
  Complexity      266      266           
=========================================
  Files            98       98           
  Lines          6191     6191           
  Branches        751      751           
=========================================
  Hits           4676     4676           
  Misses          969      969           
  Partials        546      546
Impacted Files Coverage Δ Complexity Δ
glean-core/src/ping/mod.rs 65.78% <ø> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24257f7...9ce09c4. Read the comment docs.

travis79
travis79 approved these changes Jan 24, 2020
View reviewed changes
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ from me with @Dexterp37's change addressed

@badboy
Copy link
Member Author

badboy commented Jan 24, 2020

I added back the checks where it makes sense.
In some parts we were already checking against the URL (in the helper functions), so we can avoid the additional check there.

@badboy badboy requested a review from Dexterp37 January 24, 2020 15:14
@badboy
Copy link
Member Author

badboy commented Jan 24, 2020 

java.lang.AssertionError: Task queue is cleared expected:<0> but was:<3>

but only for testReleaseUnitTest.
Might be a race condition due to the recent changes? Need to look into that further first.

@Dexterp37
Copy link
Contributor

Might be a race condition due to the recent changes? Need to look into that further first.

Maybe due to my async flush?

@badboy badboy mentioned this pull request Feb 6, 2020
Merged
2 tasks
@mdboom mdboom mentioned this pull request Feb 6, 2020
Merged
@badboy badboy added the blocked Blocked pull requests and issues label Feb 7, 2020
@badboy
Copy link
Member Author

badboy commented Feb 7, 2020

Waiting for reason codes/baseline PRs to land and a release first.

@badboy badboy removed the blocked Blocked pull requests and issues label Feb 12, 2020
@badboy badboy merged commit 8d2630b into master Feb 12, 2020
@badboy badboy deleted the drop-ping-type branch February 12, 2020 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travis79 travis79 travis79 approved these changes

@Dexterp37 Dexterp37 Dexterp37 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@badboy @codecov-io @Dexterp37 @travis79