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

Failed to retrieve oldest saved checkpoint details. #3779

Closed
anandwana001 opened this issue Sep 13, 2021 · 3 comments · Fixed by #4696
Closed

Failed to retrieve oldest saved checkpoint details. #3779

anandwana001 opened this issue Sep 13, 2021 · 3 comments · Fixed by #4696
Assignees
Labels
Impact: Low Low perceived user impact (e.g. edge cases). Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@anandwana001
Copy link
Contributor

Describe the bug
Getting error log when opening exploration for the first time.

To Reproduce
Steps to reproduce the behavior:

  1. Go to lesson tab
  2. Start any explortaion
  3. Check logcat
  4. See error

Expected behavior
Investigate what the error is about and what is the suitable solution.

Device

  • Emulator being used - Pixel XL
  • SDK version 19 and 28

Additional context

09-13 10:10:48.272 E/ExplorationActivity: Failed to retrieve oldest saved checkpoint details.
    org.oppia.android.domain.exploration.lightweightcheckpointing.ExplorationCheckpointController$ExplorationCheckpointNotFoundException: No saved checkpoints in exploration_checkpoint_database for profileId 0.
        at org.oppia.android.domain.exploration.lightweightcheckpointing.ExplorationCheckpointController$retrieveOldestSavedExplorationCheckpointDetails$1.invokeSuspend(ExplorationCheckpointController.kt:207)
        at org.oppia.android.domain.exploration.lightweightcheckpointing.ExplorationCheckpointController$retrieveOldestSavedExplorationCheckpointDetails$1.invoke(ExplorationCheckpointController.kt)
        at org.oppia.android.util.data.AsyncResult$transformAsync$2.invokeSuspend(AsyncResult.kt:90)
        at org.oppia.android.util.data.AsyncResult$transformAsync$2.invoke(AsyncResult.kt)
        at org.oppia.android.util.data.AsyncResult.transformWithResultAsync(AsyncResult.kt:147)
        at org.oppia.android.util.data.AsyncResult.transformAsync(AsyncResult.kt:89)
        at org.oppia.android.util.data.DataProviders$Companion$transformAsync$1.retrieveData(DataProviders.kt:73)
        at org.oppia.android.util.data.DataProviders$Companion$transformAsync$1$retrieveData$1.invokeSuspend(DataProviders.kt)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
        at java.lang.Thread.run(Thread.java:841)
@BenHenning
Copy link
Member

Note that this is a "known/okay error" that we may want to revise to be less noisy.

/cc @MaskedCarrot in case you have any thoughts

@MaskedCarrot
Copy link
Contributor

MaskedCarrot commented Sep 15, 2021

One possible way to make this less noisy is to modify the proto to be something like this.

message CheckpointDatabaseDetails {
    oneOf checkpointDatabaseState {
         bool checkpointDatabaseIsEmpty = 1;
         OldestCheckpointDetalis oldestCheckpointDetails = 2;
    } 
}

message oldestCheckpointDetails  {
    string exploration_title = 1;
    string exploration_id  = 2;
}

In this case, we will not have to return a failed async result if no checkpoint is saved, so no error will be logged.

@BenHenning WDYT?

@Broppia Broppia added issue_type_bug Impact: Low Low perceived user impact (e.g. edge cases). labels Jul 7, 2022
@BenHenning BenHenning added issue_type_infrastructure Z-ibt Temporary label for Ben to keep track of issues he's triaged. and removed issue_type_bug labels Sep 15, 2022
@adhiamboperes adhiamboperes self-assigned this Nov 1, 2022
BenHenning added a commit that referenced this issue Nov 9, 2022
This error (corresponding to #3779) is noise since it's not actually a
real "error."
@BenHenning
Copy link
Member

Taking this since I can easily address it as part of #4696 (since I'm changing related code, anyway).

I think we can just use default instance here, no need for the oneof (though we could certainly move to that in a binary compatible way in the future).

BenHenning added a commit that referenced this issue Nov 11, 2022
## Explanation
Fixes #4632
Fixes #3779

Adds support for upgrading checkpoints to support the latest versions of
explorations if the checkpoint was saved for an older version, in cases
where it's very likely reasonable to upgrade the checkpoints. The
methodology applied is to simulate "replaying" the full lesson using the
past answers that were submitted in the checkpointing, and verifying
that the same outcome would more or less be achieved by ensuring:
- The content IDs for received feedback haven't changed (but the HTML
might, e.g. for typos).
- The destination of a state hasn't changed.
- The pending & all completed states still exist.

The cases that currently can't be handled (but may be reasonable to in
the future):
- State renaming (these are treated as failures).
- Reconstituting a compatible hint/solution state (the state is just
reset for now so the user can re-trigger the hints if they get stuck
after returning to the lesson).

As part of testing this:
- A bunch of new assets were introduced to directly demonstrate
upgrading flows that lead to both compatibilities and incompatibilities
(these were generated).
- A fake ``ExplorationRetriever`` needed to be introduced in order to
support proxying exploration loading (so that newer versions can be
force-loaded). This required splitting the existing ExplorationRetriever
into an interface & implementation.
- ``ExplorationStorageModule`` has been moved one package up for better
organization (which led to a bunch of other files being changed).
- Test-only checkpoint database storage sizes have been 150 bytes in all
related tests, so this has been made as the test-specific constant in
the new ExplorationStorageTestModule.

Separately, ExplorationRetriever was updated to mostly force background
thread interaction by using a suspend method for actual exploration
loading (which led to some small test changes).

#3779 was also fixed (since I was changing this code, anyway) by
returning a default details object instead of failing whenever a
checkpoint can't be found for a given exploration. Note that no test was
added for this solution since doing so would be a bit painful (as it
requires loading and verifying an exploration at the activity level
where a bunch of the lower-level tools from StateFragment{Local}Test
aren't available), and the failure case here is something that will
definitely be caught by e2e tests so the risk is low for a serious
regression.

## Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
While this PR does affect UI flows for checkpoints, I'm opting to not
include a walkthrough since it's difficult and time consuming to
simulate the local upgrade flow. We'll be testing this specially during
manual QA testing for this release to ensure it works as expected.

Similarly, app module tests haven't changed in a major way so I'm
skipping showing Espresso results here. The Robolectric tests results
ought to be a sufficiently good proxy for correctness.
Repository owner moved this from Todo to Done in [Team] Core Learner and Mastery flows & UI Frontend - Android Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact: Low Low perceived user impact (e.g. edge cases). Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

Successfully merging a pull request may close this issue.

5 participants