Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[expo-updates][ios] add more database fields for error recovery manager #14610

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

esamelson
Copy link
Contributor

Why

ENG-1493

#14397 (comment)

How

Add the following database fields to updates:

  • successful_launch_count -- how many times the update has launched and we've successfully received RCTContentDidAppearNotification. Once this is > 0 we want to mark all older updates as outdated and will no longer fall back to an older update. [This could be a boolean field, but since booleans are integers in SQLite anyway I thought it could be useful in the future to keep track of the number of launches 🤷 ]
  • failed_launch_count -- how many times the update has failed to launch, i.e. thrown an error before the initial render. Once this is > 0 we won't try to launch the update again, and we might fall back to an older update if a newer one isn't available. [again, could be a boolean]
  • is_outdated -- whether or not a "newer" (determined by selection policy) update has been launched successfully. If this is true we won't launch the update again. (It might still be kept around for its assets, to make server-based rollbacks quicker.)

Test Plan

Tested as part of the full PR stack

Todo

Once this PR stack is finalized, write the full DB migration

Checklist

  • Documentation is up to date to reflect these changes (eg: https://docs.expo.io and README.md).
  • This diff will work correctly for expo build (eg: updated @expo/xdl).
  • This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

@esamelson esamelson requested review from ide and jkhales October 1, 2021 04:07
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Oct 1, 2021
Copy link
Contributor

@jkhales jkhales left a comment

Choose a reason for hiding this comment

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

Is it possible that we can get by with just one new "boolean" field?

NSString *sql = [NSString stringWithFormat:@"SELECT *\
FROM updates\
WHERE scope_key = ?1\
AND is_outdated = 0\
AND (successful_launch_count > 0 OR failed_launch_count < 1)\
Copy link
Contributor

Choose a reason for hiding this comment

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

If an update has a failed_launch_count > 0 then how is it possible to ever gain a succesful_launch_count > 0?

If this is not possible, than can't we just monitor the failed_launch_count?

Copy link
Member

@ide ide Oct 4, 2021

Choose a reason for hiding this comment

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

An update can inconsistently succeed or fail e.g. it could succeed the first time and fail the second (successes = 1, failures = 1). This check is to see if the update has ever been successful or if it has never run at all. Another way to read this check is successful_launch_count > 0 OR successful_launch_count + failed_launch_count = 0.

Copy link
Contributor

@jkhales jkhales Oct 11, 2021

Choose a reason for hiding this comment

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

I see, thanks. Still feels like there is a way we could condense these columns, but maybe that more of a personal stylistic choice.

@@ -210,6 +213,35 @@ - (void)markUpdateAccessed:(EXUpdatesUpdate *)update error:(NSError ** _Nullable
[self _executeSql:updateSql withArgs:@[update.lastAccessed, update.updateId] error:error];
}

- (void)incrementSuccessfulLaunchCountForUpdate:(EXUpdatesUpdate *)update error:(NSError ** _Nullable)error
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why we just don't call markUpdatesOutdated where ever this is used instead?

And get rid of the successful_launch_count column?

@ide
Copy link
Member

ide commented Oct 4, 2021

The is_outdated column might be a duplicate source of truth -- if an update in the database has successful_launch_count > 0, aren't all older updates that target the same RTV outdated? Seems better to run the query (plus it's a small table anyway) instead of adding a column.

@esamelson
Copy link
Contributor Author

esamelson commented Oct 6, 2021

The is_outdated column is a duplicate source of truth as long as the following assumptions hold:

  • once an update has run successfully, nothing will prevent expo-updates from running it again as long as it's the most up-to-date compatible update
  • the selection policy will always choose the most recent compatible update to run
  • newer updates will never be deleted before older updates

If we're comfortable that these assumptions will always hold then I'm fine with removing this column; the main reason I added it was to keep track of this data on a per-update basis rather than depending on the existence of other updates to derive it.

@esamelson esamelson force-pushed the @eric/updates-error-recovery-0 branch from 273062e to 3d262c5 Compare October 12, 2021 21:22
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Oct 12, 2021
@esamelson esamelson merged commit 5611373 into master Oct 12, 2021
@esamelson esamelson deleted the @eric/updates-error-recovery-0 branch October 12, 2021 21:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants