Feat: no downtime reindex, second attempt #37
Conversation
... instead of opensearch_dsl's one. Because we might need to add some logic in there :)
* The Document Index name does not match a concrete OpenSearch index anymore * Concrete OpenSearch indices are named after the Document Index name, but with a suffix (which can be set explicitely or automatically) * The Document Index name is set as an alias to a chosen concrete OpenSearch index * This allows multiple indices living alongside for a same Document, and the ability to switch the alias from an index to another
* Index creation is now done through the Document.init() to allow naming through a given suffix * Index rebuild makes no sense anymore
David-Guillot
force-pushed
the
feat/no-downtime-reindex-v2
branch
from
e884701 to
4328341
Compare
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #37 +/- ##
==========================================
- Coverage 95.65% 94.26% -1.40%
==========================================
Files 12 12
Lines 576 628 +52
Branches 80 92 +12
==========================================
+ Hits 551 592 +41
- Misses 21 30 +9
- Partials 4 6 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@qcoumes did you have some time to think about this? |
|
Sorry, had a lot of work last month, I'll look at this tomorrow |
There was a problem hiding this comment.
Apart from the two questions below, everything seems good.
I tested the CLI, and everything worked perfectly. It does not work with already existing indices, but it was to be expected.
I will work on this project in the coming month to refactor the CLI and allow more customization on the indexing process (see #39). This will probably lead to other breaking changes so this feature will not be released immediately if you're OK with it.
| if len(actions_on_aliases) == 1 and cls._index.exists(): | ||
| cls._index.delete() |
There was a problem hiding this comment.
I'm not sure I understand what you're doing here.
There was a problem hiding this comment.
This line is supposed to handle the case of a pre-existing non-versioned index: if there is only one action to apply (no index alias to remove from a versioned index), but the index exists, let's delete it. I have seen it working, but it's not unit-tested, and you seem to have seen it not working 😅
| subparser.add_argument( | ||
| "--index-suffix", | ||
| type=str, | ||
| default=None, | ||
| help="The suffix for the index name (if you don't provide one, the current index will be used). Required for `migrate` subcommand.", | ||
| ) |
There was a problem hiding this comment.
Would it not make more sense for the migrate action to be in the index subcommand, since it doesn't really after the documents?
There was a problem hiding this comment.
IMO it's actually plainly related to Documents: a Document is represented by one or more indices, and this migrate subcommand is all about switching from one representation to another. Plus, it doesn't do anything to indices themselves except setting/unsetting aliases, which is an implementation detail of switching representations of a Document.
Of course I'm okay with this, we're already relying on our fork's branch to benefit from this feature, we can continue doing so for a couple of months. |
Fixes #2
Hi @qcoumes ,
This PR supersedes #23 in my attempts to implement #2 . This is a very different approach, based on the
Documentclass much more than onIndexclass:Documentsubclass to have multiple indices, by leveraging the ability ofopensearch_dslDocumentclass to do soindex rebuildwas removed because it was not making any sense anymore, anddocument migratehas been added, but it doesn't do anything more than switching an alias (you need to callindex createthendocument indexto actually put documents into your new index)index createis working, as well asdocument migrate, which delete the concrete index if it existsI'll let you judge if this architecture fits your vision of this lib. If it doesn't, I'll stop trying to implement #2 because it's been a lot of energy on my end, and if it needs another rewrite I'll just find a way to integrate this solution somewhere in our codebase, as an extension of this lib.