Conversation
|
🎯 Code Coverage 🔗 Commit SHA: 52ea5c3 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3151 +/- ##
===========================================
+ Coverage 70.96% 70.97% +0.02%
===========================================
Files 912 912
Lines 33548 33576 +28
Branches 5640 5645 +5
===========================================
+ Hits 23804 23830 +26
+ Misses 8173 8167 -6
- Partials 1571 1579 +8
🚀 New features to boost your workflow:
|
9296fe8 to
3bcf9e9
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bcf9e9328
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...ain/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt
Outdated
Show resolved
Hide resolved
0acf4c7 to
4adc9f2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4adc9f267e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...id-core/src/main/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorage.kt
Show resolved
Hide resolved
4adc9f2 to
e34c0d7
Compare
...ain/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt
Show resolved
Hide resolved
| val isOldFile = (it.name.toLongOrNull() ?: 0) < threshold | ||
| if (isOldFile) { | ||
| if (it.deleteSafe(internalLogger)) { | ||
| onFileDeleted(it) |
There was a problem hiding this comment.
It seems we need some sort of private method that calls onFileDeleted after deleteSafe(internalLogger). This pattern is present in the code in multiple places.
| private var previousFileItemCount: Long = 0 | ||
| private var lastFileAccessTimestamp: Long = 0L | ||
| private var lastCleanupTimestamp: Long = 0L | ||
| private var currentBatchState = CurrentBatchState(null, 0L, 0L) |
There was a problem hiding this comment.
Could you please double-check access to this currentBatchState doesn't need to be done in synchronized block?
There was a problem hiding this comment.
I confirm this class runs on a persistenceExecutorService with pool size 1.
There was a problem hiding this comment.
I confirm this class runs on a persistenceExecutorService with pool size 1.
This may change at some point though.
.../com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileOrchestrator.kt
Show resolved
Hide resolved
e34c0d7 to
52ea5c3
Compare
What does this PR do?
This is the follow-up on #2720, which was rolled back in #3124.
The original approach relied on
FileObserverto detect file deletions asynchronously. Now, instead of relying on async file system notifications, we use synchronousonFileDeletedcallbacks, and validate file existence in disk as a safeguard before returning files.This is much better than running
rootDir.listFilesSafe()everytime.Review checklist (to be filled by reviewers)