Queue show notes downloading #1390

geekygecko · 2023-09-20T07:41:12Z

Description

After doing some debugging with the Background Task Inspector, I noticed that when you download multiple episodes at once, the show notes tasks run in parallel. A single queue that caches the show notes seems to be a better approach. If you download multiple episodes from the same podcast the show notes will be cached with the first call, before the same file would be downloaded multiple times until the cache was populated.

This PR also stops some reports to Sentry, I will comment on them inline.

Testing Instructions

Add the "package:mine UpdateShowNotesTask"
Subscribe to some podcasts
Go to a podcast page
Long press an episode
Press the toolbar overflow menu and select all
Tap download in the toolbar
✅ Verify after the first "Worker completed" line, the time taken is much shorter indicating the cache is being used.

geekygecko · 2023-09-20T07:44:55Z

...ces/model/src/main/java/au/com/shiftyjelly/pocketcasts/models/converter/DateTypeConverter.kt

-    fun toDate(value: Long?): Date? {
-        return if (value == null) null else Date(value)
+    fun toDate(value: Long?): Date {
+        return if (value == null) Date() else Date(value)


This should stop the following Sentry error.

au.com.shiftyjelly.pocketcasts.models.db.dao.EpisodeDao_Impl$44 in call IllegalStateException Expected non-null java.util.Date, but it was null.

It seems like the database episode has a publish date of null. I can't find how this is possible but this should stop the crash happening when the data is read.

Nice job avoiding this crash. I wonder if returning a dynamic date might not be the best approach though.

I'm not sure about this, but maybe we'd be better off returning a static date object (i.e., something like the UNIX epoch), so the behavior is consistent and doesn't return something different each time it is called with null. 🤔 This is just me thinking out loud. I'm seriously not sure here.

I think it would be good to log a Sentry breadcrumb here if the Long is null, so that if this somehow causes a problem later on that shows up in Sentry, this will get flagged for us. Something like:

if (value == null) { Sentry.addBreadcrumb("DateTypeConverter.toDate() Long parameter is null. Returning default date") Date.from(Instant.EPOCH) } else Date(value)

Maybe LogBuffer.log would be better than just adding the breadcrumb.

Excellent idea to log and return a constant date. I have made this change.

That log just saved me! It gets called with dates that need to be null. I'm reverting this issue fix.

geekygecko · 2023-09-20T07:48:28Z

...s/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/PodcastRatingsViewModel.kt

+                    Timber.i(e, message)
+                } else {
+                    Timber.e(e, message)
+                }


This should stop the following Sentry error as I can see the previous call is the ratings endpoint.

HttpException HTTP 404

geekygecko · 2023-09-20T07:49:32Z

...c/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/DownloadEpisodeTask.kt

@@ -125,13 +126,13 @@ class DownloadEpisodeTask @AssistedInject constructor(
            this.episode = runBlocking { episodeManager.findEpisodeByUuid(episodeUUID!!) } ?: return Result.failure()

            if (this.episode.downloadTaskId != id.toString()) {
-                LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Mismatched download task id for episode ${episode.title}. Cancelling.")
+                LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Mismatched download task id for episode ${episode.title}. Cancelling.")


There are a few issues reported in this class that are being sent to Sentry. If we aren't going address them I believe they should just be the log level of info.

I agree with the other places you've made this change, but I wonder if we should keep this one since if it shot up all of a sudden that might be an issue we'd want to know about.

This isn't something you changed, but while we're here, do you think it would be worth including the downloadTaskId and id in the error message. Seems like that would be helpful in tracking down why these are occurring. Just a thought, I don't feel strongly.

I have kept this and added the extra ids in the message. Thanks for the suggestions.

geekygecko · 2023-09-20T11:53:45Z

...c/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt

-                SentryHelper.recordException(e)
-            }
+            info("Worker failed - took ${SystemClock.elapsedRealtime() - startTime} ms")
+            val logPriority = if (e is HttpException) Log.INFO else Log.ERROR


I wonder if there is a better way to do this. Maybe I should add a common way to add network exceptions which doesn't report to Sentry. 🤔

That's not a bad idea

mchowning

Really nice work. Just had a couple of comments/questions, but nothing big. This is a great improvement. 🤩

mchowning · 2023-09-20T14:50:13Z

...ces/model/src/main/java/au/com/shiftyjelly/pocketcasts/models/converter/DateTypeConverter.kt

-    fun toDate(value: Long?): Date? {
-        return if (value == null) null else Date(value)
+    fun toDate(value: Long?): Date {
+        return if (value == null) Date() else Date(value)


Nice job avoiding this crash. I wonder if returning a dynamic date might not be the best approach though.

I'm not sure about this, but maybe we'd be better off returning a static date object (i.e., something like the UNIX epoch), so the behavior is consistent and doesn't return something different each time it is called with null. 🤔 This is just me thinking out loud. I'm seriously not sure here.

I think it would be good to log a Sentry breadcrumb here if the Long is null, so that if this somehow causes a problem later on that shows up in Sentry, this will get flagged for us. Something like:

if (value == null) { Sentry.addBreadcrumb("DateTypeConverter.toDate() Long parameter is null. Returning default date") Date.from(Instant.EPOCH) } else Date(value)

mchowning · 2023-09-20T14:56:22Z

...c/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt

+                .addTag(episode.uuid)
+                .setConstraints(constraints)
+                .build()
+            WorkManager.getInstance(context).beginUniqueWork(TASK_NAME, ExistingWorkPolicy.APPEND, workRequest).enqueue()


Is APPEND here better than APPEND_OR_REPLACE? I'm not particularly familiar with this, so I may be misunderstanding, but I was thinking that APPEND_OR_REPLACE might be better to avoid dropping subsequent tasks if an early one fails.

The show notes task always returns Result.success() so I think this shouldn't matter but it seems like a safer option, I will change it.

mchowning · 2023-09-20T14:58:05Z

...c/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt

-                SentryHelper.recordException(e)
-            }
+            info("Worker failed - took ${SystemClock.elapsedRealtime() - startTime} ms")
+            val logPriority = if (e is HttpException) Log.INFO else Log.ERROR


That's not a bad idea

mchowning · 2023-09-20T15:01:09Z

...ces/model/src/main/java/au/com/shiftyjelly/pocketcasts/models/converter/DateTypeConverter.kt

-    fun toDate(value: Long?): Date? {
-        return if (value == null) null else Date(value)
+    fun toDate(value: Long?): Date {
+        return if (value == null) Date() else Date(value)


Maybe LogBuffer.log would be better than just adding the breadcrumb.

mchowning · 2023-09-20T15:04:32Z

...c/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/DownloadEpisodeTask.kt

@@ -125,13 +126,13 @@ class DownloadEpisodeTask @AssistedInject constructor(
            this.episode = runBlocking { episodeManager.findEpisodeByUuid(episodeUUID!!) } ?: return Result.failure()

            if (this.episode.downloadTaskId != id.toString()) {
-                LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Mismatched download task id for episode ${episode.title}. Cancelling.")
+                LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Mismatched download task id for episode ${episode.title}. Cancelling.")


I agree with the other places you've made this change, but I wonder if we should keep this one since if it shot up all of a sudden that might be an issue we'd want to know about.

This isn't something you changed, but while we're here, do you think it would be worth including the downloadTaskId and id in the error message. Seems like that would be helpful in tracking down why these are occurring. Just a thought, I don't feel strongly.

mchowning · 2023-09-20T15:05:39Z

...c/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt

        return try {
-            showNotesManager.downloadShowNotes(podcastUuid = podcastUuid, episodeUuid = episodeUuid)
+            showNotesManager.downloadToCacheShowNotes(podcastUuid = podcastUuid)
+            info("Worker completed - took ${SystemClock.elapsedRealtime() - startTime} ms")


Including the timing here is nice!

mchowning · 2023-09-20T15:14:16Z

modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/log/LogBuffer.kt

+    fun log(priority: Int, tag: String, throwable: Throwable, message: String, vararg args: Any) {
+        addLog(priority, tag, throwable, message, *args)
+    }


Instead of adding a new log method, could we get the same effect by renaming addLog to log and making that public? Certainly not a big deal, but it feels a bit more straightforward.

geekygecko commented Sep 20, 2023

View reviewed changes

geekygecko marked this pull request as ready for review September 20, 2023 11:42

geekygecko requested a review from a team as a code owner September 20, 2023 11:42

geekygecko force-pushed the update/show-notes-task branch from c15ca38 to 0a578a8 Compare September 20, 2023 11:43

geekygecko commented Sep 20, 2023

View reviewed changes

mchowning approved these changes Sep 20, 2023

View reviewed changes

geekygecko added 5 commits September 21, 2023 15:11

Queue show notes downloading

9f4fee1

Added a changelog entry

71d36f6

Code clean up

da0350d

Improving show notes changes

843ecd3

Fixed CHANGELOG

5efd4f8

geekygecko force-pushed the update/show-notes-task branch from 725f6f5 to 5efd4f8 Compare September 21, 2023 05:42

geekygecko merged commit fa5a2e1 into main Sep 21, 2023

geekygecko deleted the update/show-notes-task branch September 21, 2023 06:30

ashiagr added this to the 7.48 ❄️ milestone Sep 22, 2023

mchowning mentioned this pull request Nov 29, 2023

Use type converter that cannot return null for date to avoid crash #1573

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queue show notes downloading #1390

Queue show notes downloading #1390

geekygecko commented Sep 20, 2023 •

edited

Loading

geekygecko Sep 20, 2023

mchowning Sep 20, 2023

mchowning Sep 20, 2023

geekygecko Sep 21, 2023

geekygecko Sep 21, 2023

geekygecko Sep 20, 2023

geekygecko Sep 20, 2023

mchowning Sep 20, 2023 •

edited

Loading

geekygecko Sep 21, 2023

geekygecko Sep 20, 2023

mchowning Sep 20, 2023

mchowning left a comment

mchowning Sep 20, 2023

mchowning Sep 20, 2023

geekygecko Sep 21, 2023

mchowning Sep 20, 2023

mchowning Sep 20, 2023

mchowning Sep 20, 2023 •

edited

Loading

mchowning Sep 20, 2023

mchowning Sep 20, 2023

Queue show notes downloading #1390

Queue show notes downloading #1390

Conversation

geekygecko commented Sep 20, 2023 • edited Loading

Description

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchowning Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchowning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchowning Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geekygecko commented Sep 20, 2023 •

edited

Loading

mchowning Sep 20, 2023 •

edited

Loading

mchowning Sep 20, 2023 •

edited

Loading