rpuch commented on code in PR #3215:
URL: https://github.com/apache/ignite-3/pull/3215#discussion_r1492491643
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -91,14 +95,14 @@ public void close() {
/**
* Collect indexes for {@link PartitionAccess#addWrite(RowId, BinaryRow,
UUID, int, int, int)}.
*
- * <p>Approximate index selection algorithm:</p>
+ * <p>Index selection algorithm:</p>
* <ul>
* <li>If the index in the snapshot catalog version is in status
{@link CatalogIndexStatus#BUILDING},
* {@link CatalogIndexStatus#AVAILABLE} or {@link
CatalogIndexStatus#STOPPING}.</li>
* <li>If the index in status {@link CatalogIndexStatus#REGISTERED}
and it is in this status on the active version of the catalog
* for {@code beginTs}.</li>
- * <li>For read-only indexes, if their {@link
CatalogIndexStatus#STOPPING} status activation time is strictly less than
- * {@code beginTs}.</li>
+ * <li>For a read-only index, if {@code beginTs} is strictly less than
the activation time of status
+ * {@link CatalogIndexStatus#STOPPING}.</li>
Review Comment:
Or of removal, if there was no STOPPING
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -140,56 +148,39 @@ public List<Integer> chooseForAddWrite(int
catalogVersion, int tableId, HybridTi
*/
public List<Integer> chooseForAddWriteCommitted(int catalogVersion, int
tableId, HybridTimestamp commitTs) {
Review Comment:
Could you please clarify in the javadoc that this method is always about
committed versions?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -274,30 +267,58 @@ private long catalogActivationTimestampBusy(int
catalogVersion) {
return catalog.time();
}
- private void recoveryReadOnlyIndexesBusy() {
+ private void recoverReadOnlyIndexesBusy() {
int earliestCatalogVersion = catalogService.earliestCatalogVersion();
int latestCatalogVersion = catalogService.latestCatalogVersion();
- // At the moment, we will only use tables from the latest version (not
dropped), since so far only replicas for them are started
- // on the node.
- int[] tableIds =
catalogService.tables(latestCatalogVersion).stream().mapToInt(CatalogObjectDescriptor::id).toArray();
-
- Map<Integer, ReadOnlyIndexInfo> readOnlyIndexes = new HashMap<>();
+ var readOnlyIndexById = new HashMap<Integer, ReadOnlyIndexInfo>();
+ var previousCatalogVersionTableIds = Set.<Integer>of();
for (int catalogVersion = earliestCatalogVersion; catalogVersion <=
latestCatalogVersion; catalogVersion++) {
- Catalog catalog = catalogService.catalog(catalogVersion);
+ long activationTs = catalogActivationTimestampBusy(catalogVersion);
- assert catalog != null : catalogVersion;
+ catalogService.indexes(catalogVersion).stream()
+ .filter(index -> index.status() == STOPPING)
+ .forEach(index ->
readOnlyIndexById.computeIfAbsent(index.id(), i -> new ReadOnlyIndexInfo(index,
activationTs)));
- for (int tableId : tableIds) {
- for (CatalogIndexDescriptor index : catalog.indexes(tableId)) {
- if (index.status() == STOPPING) {
- readOnlyIndexes.computeIfAbsent(index.id(), indexId ->
new ReadOnlyIndexInfo(tableId, catalog.time(), indexId));
- }
- }
+ Set<Integer> currentCatalogVersionTableIds =
tableIds(catalogVersion);
+
+ int finalCatalogVersion = catalogVersion;
Review Comment:
Please add a comment explaining that here we look for indices that
transitioned directly from AVAILABLE to [deleted] (corresponding to the logical
READ_ONLY state) as such transitions only happen when a table is dropped.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -129,8 +137,8 @@ public List<Integer> chooseForAddWrite(int catalogVersion,
int tableId, HybridTi
* <ul>
* <li>If the index in the snapshot catalog version is in status
{@link CatalogIndexStatus#BUILDING},
* {@link CatalogIndexStatus#AVAILABLE} or {@link
CatalogIndexStatus#STOPPING}.</li>
- * <li>For read-only indexes, if their {@link
CatalogIndexStatus#STOPPING} status activation time is strictly less than
- * {@code beginTs}.</li>
+ * <li>For a read-only index, if {@code commitTs} is strictly less
than the activation time of status
+ * {@link CatalogIndexStatus#STOPPING}.</li>
Review Comment:
Or of removal, if there was no STOPPING :)
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -274,30 +267,58 @@ private long catalogActivationTimestampBusy(int
catalogVersion) {
return catalog.time();
}
- private void recoveryReadOnlyIndexesBusy() {
+ private void recoverReadOnlyIndexesBusy() {
int earliestCatalogVersion = catalogService.earliestCatalogVersion();
int latestCatalogVersion = catalogService.latestCatalogVersion();
- // At the moment, we will only use tables from the latest version (not
dropped), since so far only replicas for them are started
- // on the node.
- int[] tableIds =
catalogService.tables(latestCatalogVersion).stream().mapToInt(CatalogObjectDescriptor::id).toArray();
-
- Map<Integer, ReadOnlyIndexInfo> readOnlyIndexes = new HashMap<>();
+ var readOnlyIndexById = new HashMap<Integer, ReadOnlyIndexInfo>();
+ var previousCatalogVersionTableIds = Set.<Integer>of();
for (int catalogVersion = earliestCatalogVersion; catalogVersion <=
latestCatalogVersion; catalogVersion++) {
- Catalog catalog = catalogService.catalog(catalogVersion);
+ long activationTs = catalogActivationTimestampBusy(catalogVersion);
- assert catalog != null : catalogVersion;
+ catalogService.indexes(catalogVersion).stream()
+ .filter(index -> index.status() == STOPPING)
+ .forEach(index ->
readOnlyIndexById.computeIfAbsent(index.id(), i -> new ReadOnlyIndexInfo(index,
activationTs)));
Review Comment:
This might become incorrect if, in future, Catalog compaction will be
enabled. We might find non-first version in which the index is STOPPING,
because first version where it became STOPPING is already removed. Do we need a
TODO about this?
--
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]
