Skip to content

Conversation

@UvgenGen
Copy link
Contributor

@UvgenGen UvgenGen commented Oct 5, 2022

Description

  • The inheritance-related code is safely removed from the codebase.
  • Tests related to inheritance from old mongo are fixed

Useful information to include:
openedx/public-engineering#79

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 5, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @UvgenGen! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@UvgenGen UvgenGen force-pushed the sagirov/EDXOLDMNG-134 branch 2 times, most recently from 919e838 to 643d241 Compare October 5, 2022 14:09
@ormsbee
Copy link
Contributor

ormsbee commented Oct 12, 2022

@jristau1984: this will not merge until Monday at the earliest (2022-10-17).

Copy link
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

Code changes look good. Minor points:

  1. Can we get rid of some of this performance testing code rather than adjusting the numbers upward?
  2. Removal of features should have a feat!: conventional commit syntax.

FYI @jristau1984: If there is an unexpected use of Old Mongo for course content, you may see increased MongoDB load in New Relic after this PR merges, because it removes part of Old Mongo's internal caching layer. I don't really expect this to happen, and if it does happen, I think that the relatively small number of Old Mongo courses means that it won't threaten site stability. But it might be a useful canary test before the next set of removals.

@jmbowman: There is a chance that this PR merge will temporarily make the test suite slower (until #31134 merges after it).

(3, 17, 15, 16, 12),
(1, 25, 23, 26, 20),
(2, 51, 49, 52, 46),
(3, 141, 139, 142, 136),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these numbers higher because this test is running against Old Mongo instead of Split? If so, can we convert it to split? And if it's purely testing Old Mongo performance (i.e. not correctness), can we get rid of it altogether rather than running it more slowly?

(3, 34),
(1, 52),
(2, 82),
(3, 176),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above about whether this can be converted to Split or removed altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this changes will be converted to Split in #31134. I leaved this test with Old Mongo modulestore for checking. Should I convert to split in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rechecked #31134 , and this test are broken when I use split modulestore. So I just added skip for this tests in that PR(#31134). I think we really can get rid of this tests, can't we?.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can get rid of them.

  • If there are tests that never worked for Split Mongo, we should get rid of them.
  • If there are tests that specifically test Old Mongo performance, we should probably get rid of them.
  • If there are tests that exercise Old Mongo functionality that we intend to remove, we can get rid of them now, or save them for another PR where removing them is more natural.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests have been updated, the rest of the tests will be in the next PR

@UvgenGen UvgenGen force-pushed the sagirov/EDXOLDMNG-134 branch from 643d241 to 230633f Compare October 19, 2022 09:31
@UvgenGen
Copy link
Contributor Author

@ormsbee commit message updated with feat!:, PR rebased

@UvgenGen UvgenGen force-pushed the sagirov/EDXOLDMNG-134 branch from 230633f to 3575b3b Compare October 20, 2022 08:19
Copy link
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

@jristau1984: As a heads up, I plan to merge this Old Mongo PR on Monday morning.

@ormsbee ormsbee merged commit 3f3d0d2 into openedx:master Oct 24, 2022
@openedx-webhooks
Copy link

@UvgenGen 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants