Derive the Elasticsearch index name of a store in one place (#4929) - #4938
Open
batrived wants to merge 1 commit into
Open
Derive the Elasticsearch index name of a store in one place (#4929)#4938batrived wants to merge 1 commit into
batrived wants to merge 1 commit into
Conversation
…ph#4929) Every read and write path mapped a JanusGraph store name to an Elasticsearch index name through generateIndexStoreName, which lowercases the store. RestElasticSearchClient.clearStore composed the same name by hand and did not lowercase it. A mixed index stores its JanusGraph index name as the store name verbatim, and JanusGraph index names are case sensitive. So for an index named vertexByName, documents were written to and queried from <indexName>_vertexbyname, while DISCARD_INDEX asked Elasticsearch to delete <indexName>_vertexByName. An Elasticsearch index name is always lowercase, so that name cannot exist. Either the existence check returned false and the call did nothing, or Elasticsearch rejected the name and ManagementSystem reported that the backend does not support index removal. The documents stayed, and the schema was marked DISCARDED, so JanusGraph believed the data was gone. Remove the second derivation instead of correcting it. ElasticSearchIndex now passes the name it already derived, so ElasticSearchClient.clearStore takes the Elasticsearch index name rather than the pair it used to rebuild. Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes mixed-index DISCARD_INDEX cleanup by ensuring the Elasticsearch index-store name is derived in one place (via getIndexStoreName) and passed through to clearStore unchanged, avoiding case-related mismatches.
Changes:
- Refactors
ElasticSearchClient.clearStoreto accept a pre-derivedindexStoreName(signature change) and updates the REST implementation accordingly. - Updates
ElasticSearchIndex.clearStoreto derive the index-store name viagetIndexStoreNameand improve the thrown error context. - Adds a unit test covering the REST client
clearStorecontract (verbatim name usage + no delete when absent).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| janusgraph-es/src/test/java/org/janusgraph/diskstorage/es/rest/RestClientClearStoreTest.java | Adds unit tests asserting clearStore uses the provided index-store name verbatim and guards deletion by existence. |
| janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/rest/RestElasticSearchClient.java | Updates clearStore to operate on a single indexStoreName argument and removes duplicated name composition. |
| janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/ElasticSearchIndex.java | Derives the index-store name once via getIndexStoreName and passes it to the client; updates exception message accordingly. |
| janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/ElasticSearchClient.java | Changes the interface signature for clearStore and documents the “pre-derived name” contract. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+104
to
+107
| public void shouldNotLowercaseTheNameItIsGiven() throws IOException { | ||
| //Deriving the name is the caller's job. If this method lowercased as well, a store whose Elasticsearch index | ||
| //genuinely differs in case could not be addressed at all | ||
| final String mixedCase = "Janusgraph_MixedCase"; |
|
|
||
| //Only the existence check is issued, so no deletion followed it | ||
| verify(restClientMock, Mockito.times(1)).performRequest(requestCaptor.capture()); | ||
| assertEquals("HEAD", requestCaptor.getValue().getMethod()); |
| when(restClientMock.performRequest(any())).thenThrow(new IOException()); | ||
| final RestElasticSearchClient clientUnderTest = new RestElasticSearchClient(restClientMock, 0, false, | ||
| 0, Collections.emptySet(), 0, 0, 100_000_000); | ||
| Mockito.reset(restClientMock); |
Comment on lines
+55
to
+56
| //Takes the Elasticsearch index name of the store, already derived by the caller, so that the mapping from a | ||
| //JanusGraph store name to an Elasticsearch index name exists in exactly one place |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4929.
Every read and write path maps a JanusGraph store name to an Elasticsearch index name through
generateIndexStoreName, which lowercases the store.RestElasticSearchClient.clearStorecomposed the same name by hand and did not lowercase it.A mixed index stores its JanusGraph index name as the store name verbatim, and JanusGraph index names are case-sensitive. So for an index named
vertexByName:<indexName>_vertexbynameclearStoretargeted<indexName>_vertexByNameAn Elasticsearch index name is always lowercase, so the second cannot exist. Either the existence check returned false and the call silently did nothing, or Elasticsearch rejected the name and
ManagementSystemreported the misleading "Index removal is not supported for this Backend". Either way the documents remained while the schema was markedDISCARDED, so JanusGraph believed the data was gone.Approach
I took the second option from the issue — removing the duplicated derivation rather than correcting it.
ElasticSearchIndex.clearStorenow derives the name throughgetIndexStoreName, the same call every other path uses, and passes it down. So the mapping exists in exactly one place and cannot drift again.This changes an interface signature:
ElasticSearchClient.clearStore(String indexName, String storeName)becomesclearStore(String indexStoreName).RestElasticSearchClientis the only implementation andElasticSearchIndexthe only caller, both in this module, so nothing else in the tree is affected.IndexProvider.clearStore(String storeName)is a different interface and is untouched.If you would rather not change the signature, the one-line alternative from the issue — lowercasing inside
clearStore— also fixes the reported bug, and I am happy to switch to it. I preferred this version because the duplication is the root cause.Testing
Docker was not available to me, so I could not run the container-backed
ElasticsearchIndexTestlocally and did not want to add an integration test I could not verify.RestClientClearStoreTestunit-tests the client contract with a mockedRestClient: the existence check and the delete both address the exact name given, no delete is issued when the index is absent, and the method does not lowercase what it receives — which is what makes passing the pre-derived name correct.The lowercasing itself stays covered by the existing paths, since
getIndexStoreNameis now the only derivation and every read and write already depends on it.Also worth a look, not fixed here
Two things I noticed next to this code, both from your issue and both left alone deliberately:
generateIndexStoreNamelowercases, two mixed indexes differing only in case (byNameandbyname) map to the same Elasticsearch index and silently share documents.ManagementSystem.checkIndexNameonly enforces uniqueness on the JanusGraph side. That is a validation change, not a rename, so it belongs in its own PR.RestElasticSearchClient.deleteIndexdoes nothing at all when the name is not an alias, which looks unintended but is a separate concern.For all changes:
master)?For code changes: