IGNITE-22470 Switch IndexMetaStorage from Vault to MetaStorage #3917
Conversation
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/index/IndexMeta.java
Outdated
Show resolved
Hide resolved
| * Creates a index meta instance. | ||
| * | ||
| * @param catalogIndexDescriptor Catalog index descriptor based on which the meta will be created. | ||
| * @param catalog Catalog version from which the {@code catalogIndexDescriptor} was taken. |
There was a problem hiding this comment.
| * @param catalog Catalog version from which the {@code catalogIndexDescriptor} was taken. | |
| * @param catalog Catalog from which the {@code catalogIndexDescriptor} was taken. |
There was a problem hiding this comment.
Not just a catalog, but a specific version of it, so I told you about it.
| @@ -154,78 +169,86 @@ Collection<IndexMeta> indexMetasSnapshot() { | |||
| return List.copyOf(indexMetaByIndexId.values()); | |||
| } | |||
|
|
|||
| private void onCatalogIndexCreateEvent(CreateIndexEventParameters parameters) { | |||
| private CompletableFuture<Boolean> onCatalogIndexCreateEvent(CreateIndexEventParameters parameters) { | |||
There was a problem hiding this comment.
| private CompletableFuture<Boolean> onCatalogIndexCreateEvent(CreateIndexEventParameters parameters) { | |
| private CompletableFuture<Boolean> updateMetastoreOnCatalogIndexCreateEvent(CreateIndexEventParameters parameters) { |
Let's change the name (or add doc?) to convey what happens on the events and what does boolean in the future mean
There was a problem hiding this comment.
The name may not be entirely obvious at first glance, but it has a certain pattern, and is found in our project:
on...Event - says that this is a reaction to an event.
Catalog - indicates that this is a CatalogEvent.
IndexCreate - a specific event when an index is created in a catalog.
I don't see any need for documentation since the described name and argument pattern self-documents the method.
| ); | ||
| } | ||
|
|
||
| private CompletableFuture<?> updateAndSaveIndexMetaToMetastore( |
There was a problem hiding this comment.
metaStorageManager.invoke returns CompletableFuture, why we use <?> here?
There was a problem hiding this comment.
Because at the moment I don’t care what the future returns, I will ignore this value; if in the future someone needs the result, he can easily change it.
| return new IndexMeta(catalogIndexDescriptor, catalog); | ||
| }); | ||
| return IndexMeta.of(catalogIndexDescriptor, catalog); | ||
| }).thenApply(unused -> false); |
There was a problem hiding this comment.
Why all futures always complete with false? Can't wrap my head around it
There was a problem hiding this comment.
Please read javadoc of org.apache.ignite.internal.event.EventProducer and org.apache.ignite.internal.event.EventListener for understanding.
https://issues.apache.org/jira/browse/IGNITE-22470
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
The Review Checklist
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Notes