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.

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

Add --pubsubIdAttribute beam option for dedup when reading pubsub #1542

Merged
merged 2 commits into from
Feb 17, 2021

Conversation

relud
Copy link
Contributor

@relud relud commented Jan 20, 2021

for Bug 1627758

needs tests

@relud relud force-pushed the beam-pubsub-dedup branch 3 times, most recently from e5ccc66 to 5f1dcdf Compare January 21, 2021 00:08
@relud relud marked this pull request as ready for review January 21, 2021 19:24
@relud relud requested a review from jklukas January 21, 2021 19:25
@relud

This comment has been minimized.

@whd

This comment has been minimized.

@relud

This comment has been minimized.

@codecov-io
Copy link

Codecov Report

Merging #1542 (6fcf829) into master (789828a) will decrease coverage by 3.90%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1542      +/-   ##
============================================
- Coverage     87.06%   83.15%   -3.91%     
+ Complexity      718      619      -99     
============================================
  Files           104       76      -28     
  Lines          4376     3343    -1033     
  Branches        404      359      -45     
============================================
- Hits           3810     2780    -1030     
- Misses          444      449       +5     
+ Partials        122      114       -8     
Flag Coverage Δ Complexity Δ
ingestion_beam 83.15% <100.00%> (-0.05%) 0.00 <0.00> (ø)
ingestion_edge ? ?
ingestion_sink ? ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...ava/com/mozilla/telemetry/options/SinkOptions.java 82.75% <ø> (ø) 7.00 <0.00> (ø)
...m/src/main/java/com/mozilla/telemetry/io/Read.java 81.53% <100.00%> (+0.58%) 1.00 <0.00> (ø)
.../java/com/mozilla/telemetry/options/InputType.java 81.81% <100.00%> (ø) 1.00 <0.00> (ø)
...tion/core/transform/PubsubMessageToObjectNode.java 72.02% <0.00%> (-11.89%) 3.00% <0.00%> (ø%)
...etry/ingestion/core/util/DerivedAttributesMap.java 96.00% <0.00%> (-4.00%) 15.00% <0.00%> (-1.00%)
...va/com/mozilla/telemetry/decoder/GeoIspLookup.java 89.47% <0.00%> (-1.76%) 6.00% <0.00%> (ø%)
...a/com/mozilla/telemetry/decoder/GeoCityLookup.java 90.56% <0.00%> (-0.95%) 10.00% <0.00%> (ø%)
...lla/telemetry/ingestion/sink/util/TimedFuture.java
...ngestion/sink/transform/StringToPubsubMessage.java
...ozilla/telemetry/ingestion/sink/io/PubsubLite.java
... and 25 more

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 789828a...dbd71c9. Read the comment docs.

@jklukas
Copy link
Contributor

jklukas commented Jan 21, 2021

Up to @whd as to whether we should delay merging right now, or if you'd like this to go to staging right away.

@whd
Copy link
Member

whd commented Jan 21, 2021

Up to @whd as to whether we should delay merging right now, or if you'd like this to go to staging right away.

Let's merge this on Monday. The tentative plan:

  1. merge on the 25th, auto-deploy to stage, observe stage
  2. deploy prod (telemetry decoder only) before UTC 26th
  3. observe performance of the decoder, run until at least UTC 27th
  4. inspect dedup performance from e.g. pbe for the 26th
  5. decide whether to roll forward the structured decoder or roll back the telemetry decoder
  6. assuming roll forward, continue deploys as per usual

@whd
Copy link
Member

whd commented Jan 25, 2021

I'm holding off on testing this until an unrelated support case with Google is resolved.

@whd
Copy link
Member

whd commented Feb 5, 2021

Looks like the issue is resolved, so we can set this test up next week.

EDIT: planning to run this tomorrow for UTC Feb 10.

@whd
Copy link
Member

whd commented Feb 9, 2021

I'm pushing this test out to next week (UTC Feb 17) due to #1560 and because I'll be out after Wednesday this week.

@whd
Copy link
Member

whd commented Feb 16, 2021

I'm planning to merge this after #1566 is deployed.

EDIT: this will likely be tomorrow (UTC Feb 18), since #1566 didn't end up being merged before UTC 17th.

@whd whd merged commit 107d9ca into master Feb 17, 2021
@whd whd deleted the beam-pubsub-dedup branch February 17, 2021 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants