Phillippko commented on code in PR #3917:
URL: https://github.com/apache/ignite-3/pull/3917#discussion_r1639280630
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/index/IndexMeta.java:
##########
@@ -42,43 +44,60 @@ public class IndexMeta implements Serializable {
@IgniteToStringInclude
private final Map<MetaIndexStatus, MetaIndexStatusChange> statusChanges;
- /** Constructor. */
- IndexMeta(CatalogIndexDescriptor catalogIndexDescriptor, Catalog catalog) {
- this(
- catalogIndexDescriptor.id(),
- catalogIndexDescriptor.tableId(),
- catalogIndexDescriptor.name(),
- MetaIndexStatus.convert(catalogIndexDescriptor.status()),
- Map.of(
-
MetaIndexStatus.convert(catalogIndexDescriptor.status()),
- new MetaIndexStatusChange(catalog.version(),
catalog.time())
- )
- );
- }
-
/**
* Constructor.
*
+ * @param catalogVersion Catalog version in which the current meta was
created.
* @param indexId Index ID.
* @param tableId Table ID to which the index belongs.
* @param indexName Index name.
* @param currentStatus Current status of the index
* @param statusChanges <b>Immutable</b> map of index statuses with change
info (for example catalog version) in which they appeared.
*/
private IndexMeta(
+ int catalogVersion,
int indexId,
int tableId,
String indexName,
MetaIndexStatus currentStatus,
Map<MetaIndexStatus, MetaIndexStatusChange> statusChanges
) {
+ this.catalogVersion = catalogVersion;
this.indexId = indexId;
this.tableId = tableId;
this.indexName = indexName;
this.currentStatus = currentStatus;
this.statusChanges = unmodifiableMap(statusChanges);
}
+ /**
+ * Creates a index meta instance.
+ *
+ * @param catalogIndexDescriptor Catalog index descriptor based on which
the meta will be created.
Review Comment:
```suggestion
* @param catalogIndexDescriptor Catalog index descriptor to create meta
from.
```
Or something like "Catalog index descriptor to base the meta on". Not
insisting
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/index/IndexMetaStorage.java:
##########
@@ -154,78 +169,86 @@ Collection<IndexMeta> indexMetasSnapshot() {
return List.copyOf(indexMetaByIndexId.values());
}
- private void onCatalogIndexCreateEvent(CreateIndexEventParameters
parameters) {
+ private CompletableFuture<Boolean>
onCatalogIndexCreateEvent(CreateIndexEventParameters parameters) {
CatalogIndexDescriptor catalogIndexDescriptor =
parameters.indexDescriptor();
Catalog catalog = catalog(parameters.catalogVersion());
- updateAndSaveIndexMetaToVault(catalogIndexDescriptor.id(), indexMeta
-> {
- assert indexMeta == null : catalogIndexDescriptor.id();
+ return updateAndSaveIndexMetaToMetastore(catalogIndexDescriptor.id(),
indexMeta -> {
+ assert indexMeta == null : "indexId=" +
catalogIndexDescriptor.id() + "catalogVersion=" + catalog.version();
- return new IndexMeta(catalogIndexDescriptor, catalog);
- });
+ return IndexMeta.of(catalogIndexDescriptor, catalog);
+ }).thenApply(unused -> false);
Review Comment:
Why all futures always complete with false? Can't wrap my head around it
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/index/IndexMeta.java:
##########
@@ -42,43 +44,60 @@ public class IndexMeta implements Serializable {
@IgniteToStringInclude
private final Map<MetaIndexStatus, MetaIndexStatusChange> statusChanges;
- /** Constructor. */
- IndexMeta(CatalogIndexDescriptor catalogIndexDescriptor, Catalog catalog) {
- this(
- catalogIndexDescriptor.id(),
- catalogIndexDescriptor.tableId(),
- catalogIndexDescriptor.name(),
- MetaIndexStatus.convert(catalogIndexDescriptor.status()),
- Map.of(
-
MetaIndexStatus.convert(catalogIndexDescriptor.status()),
- new MetaIndexStatusChange(catalog.version(),
catalog.time())
- )
- );
- }
-
/**
* Constructor.
*
+ * @param catalogVersion Catalog version in which the current meta was
created.
* @param indexId Index ID.
* @param tableId Table ID to which the index belongs.
* @param indexName Index name.
* @param currentStatus Current status of the index
* @param statusChanges <b>Immutable</b> map of index statuses with change
info (for example catalog version) in which they appeared.
*/
private IndexMeta(
+ int catalogVersion,
int indexId,
int tableId,
String indexName,
MetaIndexStatus currentStatus,
Map<MetaIndexStatus, MetaIndexStatusChange> statusChanges
) {
+ this.catalogVersion = catalogVersion;
this.indexId = indexId;
this.tableId = tableId;
this.indexName = indexName;
this.currentStatus = currentStatus;
this.statusChanges = unmodifiableMap(statusChanges);
}
+ /**
+ * 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.
Review Comment:
```suggestion
* @param catalog Catalog from which the {@code catalogIndexDescriptor}
was taken.
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/index/IndexMetaStorage.java:
##########
@@ -154,78 +169,86 @@ Collection<IndexMeta> indexMetasSnapshot() {
return List.copyOf(indexMetaByIndexId.values());
}
- private void onCatalogIndexCreateEvent(CreateIndexEventParameters
parameters) {
+ private CompletableFuture<Boolean>
onCatalogIndexCreateEvent(CreateIndexEventParameters parameters) {
Review Comment:
```suggestion
private CompletableFuture<Boolean>
updateMetastoreOnCatalogIndexCreateEvent(CreateIndexEventParameters parameters)
{
```
Let's change the name (or add doc?) to convey what happens on the event**s**
and what does boolean in the future mean
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/index/IndexMetaStorage.java:
##########
@@ -378,13 +405,49 @@ private static IndexMeta setNewStatus(IndexMeta
indexMeta, MetaIndexStatus newSt
return indexMeta.status(newStatus, catalog.version(), catalog.time());
}
- private static ByteArray indexMetaVaultKey(IndexMeta indexMeta) {
- return ByteArray.fromString(INDEX_META_KEY_PREFIX +
indexMeta.indexId());
- }
-
private static boolean shouldBeRemoved(IndexMeta indexMeta, int
lwmCatalogVersion) {
MetaIndexStatus status = indexMeta.status();
return (status == READ_ONLY || status == REMOVED) &&
indexMeta.statusChanges().get(status).catalogVersion() <= lwmCatalogVersion;
}
+
+ private static ByteArray indexMetaVersionKey(IndexMeta indexMeta) {
+ return ByteArray.fromString(INDEX_META_VERSION_KEY_PREFIX +
indexMeta.indexId());
+ }
+
+ private static ByteArray indexMetaValueKey(IndexMeta indexMeta) {
+ return ByteArray.fromString(INDEX_META_VALUE_KEY_PREFIX +
indexMeta.indexId());
+ }
+
+ private CompletableFuture<?> saveToMetastore(IndexMeta newMeta) {
+ ByteArray versionKey = indexMetaVersionKey(newMeta);
+
+ return metaStorageManager.invoke(
+ value(versionKey).lt(intToBytes(newMeta.catalogVersion())),
+ List.of(
+ put(versionKey, intToBytes(newMeta.catalogVersion())),
+ put(indexMetaValueKey(newMeta), toBytes(newMeta))
+ ),
+ List.of(noop())
+ );
+ }
+
+ private CompletableFuture<?> removeFromMetastore(IndexMeta indexMeta) {
+ ByteArray versionKey = indexMetaVersionKey(indexMeta);
+
+ return metaStorageManager.invoke(
+ exists(versionKey),
+ List.of(remove(versionKey),
remove(indexMetaValueKey(indexMeta))),
+ List.of(noop())
+ );
+ }
+
+ private CompletableFuture<?> updateAndSaveIndexMetaToMetastore(
Review Comment:
metaStorageManager.invoke returns CompletableFuture<Boolean>, why we use <?>
here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
