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.

Sign up for GitHub

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

Jump to bottom

Fix #3319: Create controller to save and retrieve checkpoints #3337

Merged
merged 8 commits into from
Jul 6, 2021
Merged

Fix #3319: Create controller to save and retrieve checkpoints #3337

merged 8 commits into from
Jul 6, 2021

Conversation

MaskedCarrot
Copy link
Contributor

Explanation

Fixes #3319

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

aggarwalpulkit596
aggarwalpulkit596 reviewed Jun 18, 2021
View reviewed changes
...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt Show resolved Hide resolved
...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt Outdated
) {

/** Different stages in which the exploration checkpoint database can exist. */
enum class ExplorationCheckpointDatabaseState {
Copy link
Contributor

Choose a reason for hiding this comment

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

@BenHenning @rt4914 The enum class here and below and replicated should we use a single enum class here or using two different classes is fine? I prefer we should use a single enum class instead of duplicating them for the particular scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aggarwalpulkit596 Yes we can wrap this into single enum.

...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt
}

/** Indicates that no checkpoint was found for the specified explorationId and profileId. */
class ExplorationCheckpointNotFoundException(message: String) : Exception(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use exceptionsController here @vinitamurthi correct?

...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt Show resolved Hide resolved
...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt Show resolved Hide resolved
...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt Show resolved Hide resolved
...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt Show resolved Hide resolved
@MaskedCarrot
Copy link
Contributor Author

@aggarwalpulkit596 PTAL.

@oppiabot oppiabot bot assigned aggarwalpulkit596 and unassigned MaskedCarrot Jun 18, 2021
@oppiabot
Copy link

oppiabot bot commented Jun 18, 2021

Unassigning @MaskedCarrot since a re-review was requested. @MaskedCarrot, please make sure you have addressed all review comments. Thanks!

rt4914
rt4914 approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Approving for code-owner file.

@rt4914 rt4914 removed their assignment Jun 21, 2021
@oppiabot
Copy link

oppiabot bot commented Jun 21, 2021

Assigning @fsharpasharp for code owner reviews. Thanks!

rt4914
rt4914 suggested changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@MaskedCarrot Just realised that I am secondary reviwer.

@oppiabot
Copy link

oppiabot bot commented Jun 21, 2021

Hi @MaskedCarrot, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks!

rt4914
rt4914 previously requested changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@MaskedCarrot PTAL

...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt Outdated
) {

/** Different stages in which the exploration checkpoint database can exist. */
enum class ExplorationCheckpointDatabaseState {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aggarwalpulkit596 Yes we can wrap this into single enum.

...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt Show resolved Hide resolved
...va/org/oppia/android/domain/exploration/lightweightcheckpointing/ExplorationStorageModule.kt Show resolved Hide resolved
...a/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointControllerTest.kt Outdated Show resolved Hide resolved
...a/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointControllerTest.kt Outdated Show resolved Hide resolved
fsharpasharp
fsharpasharp approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@fsharpasharp fsharpasharp left a comment

Choose a reason for hiding this comment

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

Approving for BUILD.bazel change

@oppiabot
Copy link

oppiabot bot commented Jun 22, 2021

Unassigning @fsharpasharp since they have already approved the PR.

@vinitamurthi vinitamurthi removed their assignment Jun 24, 2021
@MaskedCarrot MaskedCarrot removed their assignment Jun 30, 2021
aggarwalpulkit596
aggarwalpulkit596 approved these changes Jul 1, 2021
View reviewed changes
...oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt
*
* SUCCESS corresponds to successful AsyncResult with value as null.
*/
enum class ExplorationCheckpointActionStatus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this moved or not? I am bit confused.

...va/org/oppia/android/domain/exploration/lightweightcheckpointing/ExplorationStorageModule.kt Show resolved Hide resolved
@aggarwalpulkit596
Copy link
Contributor

One of the tests are failing @MaskedCarrot PTAL. Otherwise looks good to me

@oppiabot
Copy link

oppiabot bot commented Jul 1, 2021

Unassigning @aggarwalpulkit596 since they have already approved the PR.

@MaskedCarrot
Copy link
Contributor Author

@aggarwalpulkit596 The failing checks are not related to the changes made in this PR.

@aggarwalpulkit596 aggarwalpulkit596 enabled auto-merge (squash) July 1, 2021 08:51
@aggarwalpulkit596
Copy link
Contributor

@BenHenning @rt4914 Can you approve this? Being code owners this PR cannot move forward.

@BenHenning
Copy link
Sponsor Member

I plan to look at this tomorrow for codeowners.

@MaskedCarrot MaskedCarrot mentioned this pull request Jul 3, 2021
Merged
8 tasks
BenHenning
BenHenning approved these changes Jul 6, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @MaskedCarrot. Approved for codeowners (BUILD files). I didn't review anything else--please let me know if you'd like me to.

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Jul 6, 2021
@aggarwalpulkit596
Copy link
Contributor

@rt4914 Can you check this.

rt4914
rt4914 approved these changes Jul 6, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@aggarwalpulkit596 aggarwalpulkit596 merged commit 7347136 into develop Jul 6, 2021
@aggarwalpulkit596 aggarwalpulkit596 deleted the add-controller-for-checkpointing branch July 6, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@vinitamurthi vinitamurthi vinitamurthi approved these changes

@BenHenning BenHenning BenHenning approved these changes

@fsharpasharp fsharpasharp fsharpasharp approved these changes

@aggarwalpulkit596 aggarwalpulkit596 aggarwalpulkit596 approved these changes

Assignees

@rt4914 rt4914

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create controller to save and retrieve checkpoints
6 participants
@MaskedCarrot @aggarwalpulkit596 @BenHenning @rt4914 @vinitamurthi @fsharpasharp