fix: let es_index.create return versioned index name #1660
Conversation
There was a problem hiding this comment.
I think this is a smart little solution to the problem.
Pending the index management functionality this has my approval.
For safety, I added a review reqest for @lukavdplas. I'm not that familiar with the indexing procedure. So any possible interactions this may cause come to mind?
|
Ah, I'd asked you because you did the most recent changes in this file. Thanks for the approval, but let's see if @lukavdplas still spots anything we both may have missed indeed. |
| @@ -93,20 +99,26 @@ def create(client: Elasticsearch, corpus: Corpus, add: bool = False, clear: bool | |||
| settings=settings, | |||
| mappings=es_mapping, | |||
| ) | |||
| return index_name | |||
There was a problem hiding this comment.
If you're using the --add flag (to add documents to an existing index), this function won't reach this line, but return None on line 69. I expect that will cause an issue when populate() is called with versioned_index_name = None.
This should be straightforward to fix: just return the name of the existing index in that case.
There was a problem hiding this comment.
Yes, and luckily, this is already caught by a unit test. :-)
| """get the name of the current corpus' associated index""" | ||
| alias = corpus.es_alias or corpus.es_index | ||
| indices = client.indices.get(index="{}".format(alias)) | ||
| return max(sorted(indices.keys())) |
There was a problem hiding this comment.
Is this sorting alphabetically? That would mean that if my_corpus_9 and my_corpus_10 both exist, you'd use my_corpus_9, no?
For now, this PR should fix #1656 . Perhaps not the prettiest solution, but I wanted to avoid repeated requests to the Elasticsearch client just to reaffirm the current version number. Perhaps in the long run, some database bookkeeping of index versions would not be a bad idea, but let's discuss this at a later point.