Skip to content

RUM-2014 Add the synchronous equivalent of readNextBatch and confirmBatchRead in Storage API#1768

Merged
mariusc83 merged 1 commit into
developfrom
mconstantin/rum-2014/switch-to-sync-api-for-batches-upload
Dec 19, 2023
Merged

RUM-2014 Add the synchronous equivalent of readNextBatch and confirmBatchRead in Storage API#1768
mariusc83 merged 1 commit into
developfrom
mconstantin/rum-2014/switch-to-sync-api-for-batches-upload

Conversation

@mariusc83

@mariusc83 mariusc83 commented Dec 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

In this PR we are replacing the async methods readNextBatch and confirmBatchRead from Storage interface with their synchronous equivalents.

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@mariusc83 mariusc83 self-assigned this Dec 15, 2023
@mariusc83
mariusc83 force-pushed the mconstantin/rum-2014/switch-to-sync-api-for-batches-upload branch from 7d8e3a0 to 6aa4e11 Compare December 15, 2023 09:02
@mariusc83 mariusc83 changed the title RUM-2014 Use the synchronous equivalent of readNextBatch API into the… RUM-2014 Use the synchronous equivalent of readNextBatch API into the Uploader Dec 15, 2023
@mariusc83 mariusc83 changed the title RUM-2014 Use the synchronous equivalent of readNextBatch API into the Uploader RUM-2014 Use the synchronous equivalent of readNextBatch API in the Uploader Dec 15, 2023
@mariusc83
mariusc83 force-pushed the mconstantin/rum-2014/switch-to-sync-api-for-batches-upload branch 2 times, most recently from 8c4946f to 22c0a1c Compare December 15, 2023 12:30
@mariusc83
mariusc83 marked this pull request as ready for review December 15, 2023 12:39
@mariusc83
mariusc83 requested review from a team as code owners December 15, 2023 12:39
xgouchet
xgouchet suggested changes Dec 15, 2023
View reviewed changes

@xgouchet xgouchet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

We still have asynchronous confirmBatchRead in the DataUploader, so we might want to also have a synchronous method there WDYT?

@mariusc83

mariusc83 commented Dec 15, 2023

Copy link
Copy Markdown
Member Author

Warning

We still have asynchronous confirmBatchRead in the DataUploader, so we might want to also have a synchronous method there WDYT?

I actually wanted to do that but then @0xnm said that it might still have sense to call that method only with a callback.
@0xnm what do you think ?

@mariusc83
mariusc83 force-pushed the mconstantin/rum-2014/switch-to-sync-api-for-batches-upload branch from 22c0a1c to 1183b5e Compare December 15, 2023 14:09
@codecov-commenter

codecov-commenter commented Dec 15, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.90909% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.55%. Comparing base (2716cb6) to head (c699fc4).
⚠️ Report is 3787 commits behind head on develop.

Files with missing lines Patch % Lines
...dog/android/core/internal/persistence/BatchData.kt 72.22% 0 Missing and 5 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1768      +/-   ##
===========================================
+ Coverage    83.42%   83.55%   +0.12%     
===========================================
  Files          469      469              
  Lines        16434    16431       -3     
  Branches      2454     2465      +11     
===========================================
+ Hits         13710    13728      +18     
+ Misses        2053     2030      -23     
- Partials       671      673       +2
Files with missing lines Coverage Δ
.../android/core/internal/data/upload/UploadWorker.kt 86.49% <100.00%> (-0.36%) ⬇️
...core/internal/data/upload/v2/DataUploadRunnable.kt 96.77% <100.00%> (-0.37%) ⬇️
...droid/core/internal/persistence/AbstractStorage.kt 100.00% <100.00%> (ø)
...d/core/internal/persistence/ConsentAwareStorage.kt 98.70% <100.00%> (-0.08%) ⬇️
...dog/android/core/internal/persistence/BatchData.kt 72.22% <72.22%> (ø)

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mariusc83
mariusc83 force-pushed the mconstantin/rum-2014/switch-to-sync-api-for-batches-upload branch 2 times, most recently from 418a509 to ce6cd6e Compare December 18, 2023 13:17
@mariusc83 mariusc83 changed the title RUM-2014 Use the synchronous equivalent of readNextBatch API in the Uploader RUM-2014 Add the synchronous equivalent of readNextBatch and confirmBatchRead in Storage API Dec 18, 2023
@mariusc83
mariusc83 force-pushed the mconstantin/rum-2014/switch-to-sync-api-for-batches-upload branch from ce6cd6e to 28258ec Compare December 18, 2023 14:15
@mariusc83
mariusc83 requested a review from xgouchet December 18, 2023 14:16
0xnm
0xnm approved these changes Dec 19, 2023
View reviewed changes

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm! need also to delete duplicate DataUploader, DataFlusher, etc. files.

@mariusc83
mariusc83 force-pushed the mconstantin/rum-2014/switch-to-sync-api-for-batches-upload branch 2 times, most recently from fcd74a9 to 6e89ab6 Compare December 19, 2023 13:15
@mariusc83
mariusc83 force-pushed the mconstantin/rum-2014/switch-to-sync-api-for-batches-upload branch from 6e89ab6 to c699fc4 Compare December 19, 2023 13:15
@mariusc83
mariusc83 merged commit 1972792 into develop Dec 19, 2023
@mariusc83
mariusc83 deleted the mconstantin/rum-2014/switch-to-sync-api-for-batches-upload branch December 19, 2023 13:45
@xgouchet xgouchet added this to the 2.4.0 milestone Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xnm 0xnm 0xnm approved these changes
@jonathanmos jonathanmos jonathanmos approved these changes
+1 more reviewer
@xgouchet xgouchet xgouchet approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mariusc83 mariusc83

Labels

None yet

Projects

None yet

Milestone

2.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@mariusc83 @codecov-commenter @xgouchet @0xnm @jonathanmos