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

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

Merged

Conversation

mariusc83
Copy link
Member

@mariusc83 mariusc83 commented Dec 15, 2023

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
Copy link
Member

@xgouchet xgouchet left a comment

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
Copy link
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
Copy link

codecov-commenter commented Dec 15, 2023

Codecov Report

Merging #1768 (c699fc4) into develop (2716cb6) will increase coverage by 0.12%.
The diff coverage is 90.91%.

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 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

@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
Copy link
Member

@0xnm 0xnm left a comment

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
23 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants