chore: add ELASTIC_SEARCH_INDEX_PREFIX setting to prefix indices #130
Conversation
|
Thanks for the pull request, @keithgg! 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:
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. |
openedx-webhooks
added
the
open-source-contribution
|
@keithgg I've approved the tests, and the tests are running this time. |
|
@e0d do you mind kicking off the tests here again? |
|
@keithgg tests approved. |
|
@keithgg looks like your tests have passed - is this ready for review? If so, you'll need to address the branch date error in the meantime. Thanks! |
keithgg
force-pushed
the
keith/shared-elasticsearch
branch
from
d03070b
to
2a9a78a
Compare
|
@mphilbrick211 Great! I've rebased against |
|
Great, @keithgg! Are you planning to re-run the tests? |
|
@keithgg kind of looks like this branch is stuck again, not seeing the tests actually being queue up and codecov/patch seems stuck. |
|
@e0d that's a bummer. Should I create another PR? |
|
Can you try pushing a trivial commit first and try creating a new or if
that doesn't work?
I'm not seeing any obvious feedback that would explain this.
…On Thu, Feb 9, 2023, 7:32 AM Keith Grootboom ***@***.***> wrote:
@e0d <https://github.com/e0d> that's a bummer. Should I create another PR?
—
Reply to this email directly, view it on GitHub
<#130 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJWEARIXBAZXWZM34RTZMLWWTPVFANCNFSM6AAAAAAUGIEQX4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@e0d sure. I've pushed an empty commit. If that doesn't work, I'll push another with a code change. |
|
@keithgg that seems to have done it. Tests approved and running. |
|
@keithgg - Now that tests are working/passing, please let me know when this is ready for review. Thanks! |
|
Thank you @e0d. @mphilbrick211 it's (finally) ready for review. |
|
Hi @jristau1984! Would you mind taking a look at this for review/merge? |
|
Hi @jristau1984! Friendly ping on this :) |
|
Is there any chance to revisit this PR? @keithgg originally mentioned a PR for groove but the Large Instances Working Group is also expecting this to land soon. While reviewing openedx/openedx-k8s-harmony#13 we did some basic testing and everything seemed to work just fine. |
|
This feature is not currently used on the edx.org instance, so there is no need for 2U to review this PR. Thanks, |
|
Cool @jristau1984. @feanil (soz for the ping, you're the latest committer 🙂) are you able to assist with a review? |
keithgg
force-pushed
the
keith/shared-elasticsearch
branch
3 times, most recently
from
43d9480
to
92b4781
Compare
|
Thank you @feanil, I've rebased and updated the README. |
keithgg
force-pushed
the
keith/shared-elasticsearch
branch
2 times, most recently
from
af56ca8
to
a9b5111
Compare
mphilbrick211
added
the
needs test run
e0d
removed
the
needs test run
|
@feanil ready for review. |
|
@keithgg thanks for adding the doc, can you also bump the version so that we can do a release of the package once this lands? |
This helps with multitenancy so that indexes can be split per tenant. Each index can be prefixed by the string defined which will allow multiple tenants per ES cluster running the same code.
keithgg
force-pushed
the
keith/shared-elasticsearch
branch
from
a9b5111
to
f03fb8e
Compare
|
@keithgg 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
<!--
🌴🌴
🌴🌴🌴🌴 🌴 Note: the Palm master branch has been created. Please consider whether your change
🌴🌴🌴🌴 should also be applied to Palm. If so, make another pull request against the
🌴🌴🌴🌴 open-release/palm.master branch, or ask in the #wg-build-test-release Slack channel
🌴🌴 if you have any questions or need help.
🫒🫒🫒🫒🫒🫒 🫒 Note: the Olive release is still supported.
Please consider whether your change should be applied to Olive as well.
Please give your pull request a short but descriptive title.
Use conventional commits to separate and summarize commits logically:
https://open-edx-proposals.readthedocs.io/en/latest/oep-0051-bp-conventional-commits.html
Use this template as a guide. Omit sections that don't apply.
You may link to information rather than copy it, but only if the link is publicly
readable. If you must linked information must be private (because it has secrets),
clearly label the link as private.
-->
## Description
This PR aims to update the version of the edx-search library to [3.5.0](https://github.com/openedx/edx-search/releases/tag/v3.5.0). This update will enable operators to have a shared ElasticSearch installation for multiple OpenedX instances. More details about this feature can be found [here](openedx/edx-search#130).
## Supporting information
In the Large Instances Working Group we added a feature to have a [shared ElasticSearch installation](openedx/openedx-k8s-harmony#4) for multiple OpenedX instances in a Kubernetes cluster. To take advantage of this feature in the Palm release, we require the upgrade of the edx-search version.
|
Should this setting have an annotation so it will be included in the Technical Reference docs: https://edx.readthedocs.io/projects/edx-platform-technical/en/latest/index.html ? |
The feature was added to edx-search here: openedx/edx-search#130 (comment)
|
@pdpinch good catch, created a PR here, openedx/edx-platform#32428 |
Description
This PR is a redo of #128, because there was an issue with the tests running
Add a new setting
ELASTIC_SEARCH_INDEX_PREFIXthat will be prepended to all ElasticSearch indexes. This helps with multitenancy so that indexes can be split per "tenant"Testing Instructions
make test_with_esSupporting information