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

Support sharing Elasticsearch indices across multiple wikis #430

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

AndrewKostka
Copy link
Contributor

No description provided.

Copy link
Contributor

@tarrow tarrow left a comment

Choose a reason for hiding this comment

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

Looks fine to me; I don't want to hold trying this up on staging but I still think this could make sense perhaps to move to the platform api if possible. I'm struggling to see if we can avoid this surgery in the validate index method and instead successfully validate the alias.

* @return \Elastica\Index being updated
*/
public function getRealIndex() {
return $this->indexSharedName ? $this->getSharedIndex() : $this->getIndex();
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: It's nice that this is a no-op without the extra ENV being set

/**
* @return \Elastica\Index being updated
*/
public function getRealIndex() {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: this can probably be a private method (I really hope we never see a reason to call this from anywhere else)

suggestion: should this have a name other than "real"? perhaps "getNotAliasedIndex"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I updated both in the latest commit

@AndrewKostka
Copy link
Contributor Author

Looks fine to me; I don't want to hold trying this up on staging but I still think this could make sense perhaps to move to the platform api if possible. I'm struggling to see if we can avoid this surgery in the validate index method and instead successfully validate the alias.

Let's see how this works on staging but it's definitely possible to create aliases from the platform API

@AndrewKostka AndrewKostka merged commit 1199a8a into main Mar 15, 2024
9 checks passed
@AndrewKostka AndrewKostka deleted the shared-elastic-index branch March 15, 2024 13:10
AndrewKostka added a commit that referenced this pull request Mar 20, 2024
AndrewKostka added a commit that referenced this pull request Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants