You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "tkalkirill (via GitHub)" <gi...@apache.org> on 2024/07/10 13:58:58 UTC
[PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
tkalkirill opened a new pull request, #4067:
URL: https://github.com/apache/ignite-3/pull/4067
https://issues.apache.org/jira/browse/IGNITE-22682
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
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- 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.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #4067:
URL: https://github.com/apache/ignite-3/pull/4067
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
Posted by "Phillippko (via GitHub)" <gi...@apache.org>.
Phillippko commented on code in PR #4067:
URL: https://github.com/apache/ignite-3/pull/4067#discussion_r1673554590
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -273,137 +261,37 @@ private void onIndexRemoved(RemoveIndexEventParameters parameters) {
if (catalogVersion <= lwmCatalogVersion) {
// There is no need to add a read-only indexes, since the index should be destroyed under the updated low watermark.
- tableVersionByIndexId.remove(indexId);
- } else {
- CatalogIndexDescriptor index = indexBusy(indexId, catalogVersion - 1);
-
- if (index.status() == AVAILABLE) {
- // On drop table event.
- readOnlyIndexes.add(new ReadOnlyIndexInfo(index, catalogActivationTimestampBusy(catalogVersion), catalogVersion));
- } else if (index.status() == STOPPING) {
- readOnlyIndexes.add(
- new ReadOnlyIndexInfo(index, findStoppingActivationTsBusy(indexId, catalogVersion - 1), catalogVersion)
- );
- } else {
- // Index that is dropped before even becoming available.
- tableVersionByIndexId.remove(indexId);
- }
+ return;
}
- });
- });
- }
- private void onIndexCreated(CreateIndexEventParameters parameters) {
- inBusyLock(busyLock, () -> {
- CatalogIndexDescriptor index = parameters.indexDescriptor();
-
- int tableVersion = tableVersionBusy(index, parameters.catalogVersion());
+ IndexMeta indexMeta = indexMetaStorage.indexMeta(indexId);
- tableVersionByIndexId.put(index.id(), tableVersion);
- });
- }
+ assert indexMeta != null : "indexId=" + indexId + ", catalogVersion=" + catalogVersion;
- private long catalogActivationTimestampBusy(int catalogVersion) {
- Catalog catalog = catalogService.catalog(catalogVersion);
-
- assert catalog != null : catalogVersion;
-
- return catalog.time();
- }
-
- // TODO: IGNITE-21771 Deal with catalog compaction
- private void recoverStructuresBusy() {
- int earliestCatalogVersion = catalogService.earliestCatalogVersion();
- int latestCatalogVersion = catalogService.latestCatalogVersion();
- int lwmCatalogVersion = catalogService.activeCatalogVersion(hybridTimestampToLong(lowWatermark.getLowWatermark()));
-
- var tableVersionByIndexId = new HashMap<Integer, Integer>();
- var readOnlyIndexes = new HashSet<ReadOnlyIndexInfo>();
-
- var stoppingActivationTsByIndexId = new HashMap<Integer, Long>();
- var previousCatalogVersionIndexIds = Set.<Integer>of();
-
- for (int catalogVersion = earliestCatalogVersion; catalogVersion <= latestCatalogVersion; catalogVersion++) {
- int finalCatalogVersion = catalogVersion;
-
- var indexIds = new HashSet<Integer>();
-
- catalogService.indexes(finalCatalogVersion).forEach(index -> {
- tableVersionByIndexId.computeIfAbsent(index.id(), i -> tableVersionBusy(index, finalCatalogVersion));
-
- if (index.status() == STOPPING) {
- stoppingActivationTsByIndexId.computeIfAbsent(index.id(), i -> catalogActivationTimestampBusy(finalCatalogVersion));
+ if (indexMeta.status() == MetaIndexStatus.READ_ONLY) {
Review Comment:
I think we could move checking READ_ONLY status to the top, it looks like the simplest condition to check and the most important
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -1246,7 +1246,7 @@ public static CompletableFuture<Void> stopAsync(Supplier<CompletableFuture<Void>
* @param components Array of ignite components to stop.
* @return CompletableFuture that will be completed when all components are stopped.
*/
- public static CompletableFuture<Void> stopAsync(ComponentContext componentContext, IgniteComponent... components) {
+ public static CompletableFuture<Void> stopAsync(ComponentContext componentContext, @Nullable IgniteComponent... components) {
Review Comment:
IMHO nullable is confusing there - it feels like components can be nullable, and I think we don't have to annotate that varargs can be empty
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -413,17 +301,30 @@ private void onLwmChanged(ChangeLowWatermarkEventParameters parameters) {
inBusyLock(busyLock, () -> {
int lwmCatalogVersion = catalogService.activeCatalogVersion(parameters.newLowWatermark().longValue());
- Iterator<ReadOnlyIndexInfo> it = readOnlyIndexes.iterator();
+ readOnlyIndexes.removeIf(readOnlyIndexInfo -> readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion);
+ });
+ }
- while (it.hasNext()) {
- ReadOnlyIndexInfo readOnlyIndexInfo = it.next();
+ private static long activationTs(IndexMeta indexMeta, MetaIndexStatus status) {
+ return indexMeta.statusChange(status).activationTimestamp();
+ }
- if (readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion) {
- it.remove();
+ private static ReadOnlyIndexInfo toReadOnlyIndexInfo(IndexMeta indexMeta) {
+ assert indexMeta.status() == MetaIndexStatus.READ_ONLY : "indexId=" + indexMeta.indexId() + ", status=" + indexMeta.status();
- tableVersionByIndexId.remove(readOnlyIndexInfo.indexId());
- }
- }
- });
+ long activationTs;
+
+ if (indexMeta.statusChanges().containsKey(MetaIndexStatus.STOPPING)) {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.STOPPING);
+ } else {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.READ_ONLY);
Review Comment:
Let's add Nullable to indexMeta.statusChange()? javadoc - "Returns the index status change, {@code null} if absent"
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4067:
URL: https://github.com/apache/ignite-3/pull/4067#discussion_r1673589429
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -273,137 +261,37 @@ private void onIndexRemoved(RemoveIndexEventParameters parameters) {
if (catalogVersion <= lwmCatalogVersion) {
// There is no need to add a read-only indexes, since the index should be destroyed under the updated low watermark.
- tableVersionByIndexId.remove(indexId);
- } else {
- CatalogIndexDescriptor index = indexBusy(indexId, catalogVersion - 1);
-
- if (index.status() == AVAILABLE) {
- // On drop table event.
- readOnlyIndexes.add(new ReadOnlyIndexInfo(index, catalogActivationTimestampBusy(catalogVersion), catalogVersion));
- } else if (index.status() == STOPPING) {
- readOnlyIndexes.add(
- new ReadOnlyIndexInfo(index, findStoppingActivationTsBusy(indexId, catalogVersion - 1), catalogVersion)
- );
- } else {
- // Index that is dropped before even becoming available.
- tableVersionByIndexId.remove(indexId);
- }
+ return;
}
- });
- });
- }
- private void onIndexCreated(CreateIndexEventParameters parameters) {
- inBusyLock(busyLock, () -> {
- CatalogIndexDescriptor index = parameters.indexDescriptor();
-
- int tableVersion = tableVersionBusy(index, parameters.catalogVersion());
+ IndexMeta indexMeta = indexMetaStorage.indexMeta(indexId);
- tableVersionByIndexId.put(index.id(), tableVersion);
- });
- }
+ assert indexMeta != null : "indexId=" + indexId + ", catalogVersion=" + catalogVersion;
- private long catalogActivationTimestampBusy(int catalogVersion) {
- Catalog catalog = catalogService.catalog(catalogVersion);
-
- assert catalog != null : catalogVersion;
-
- return catalog.time();
- }
-
- // TODO: IGNITE-21771 Deal with catalog compaction
- private void recoverStructuresBusy() {
- int earliestCatalogVersion = catalogService.earliestCatalogVersion();
- int latestCatalogVersion = catalogService.latestCatalogVersion();
- int lwmCatalogVersion = catalogService.activeCatalogVersion(hybridTimestampToLong(lowWatermark.getLowWatermark()));
-
- var tableVersionByIndexId = new HashMap<Integer, Integer>();
- var readOnlyIndexes = new HashSet<ReadOnlyIndexInfo>();
-
- var stoppingActivationTsByIndexId = new HashMap<Integer, Long>();
- var previousCatalogVersionIndexIds = Set.<Integer>of();
-
- for (int catalogVersion = earliestCatalogVersion; catalogVersion <= latestCatalogVersion; catalogVersion++) {
- int finalCatalogVersion = catalogVersion;
-
- var indexIds = new HashSet<Integer>();
-
- catalogService.indexes(finalCatalogVersion).forEach(index -> {
- tableVersionByIndexId.computeIfAbsent(index.id(), i -> tableVersionBusy(index, finalCatalogVersion));
-
- if (index.status() == STOPPING) {
- stoppingActivationTsByIndexId.computeIfAbsent(index.id(), i -> catalogActivationTimestampBusy(finalCatalogVersion));
+ if (indexMeta.status() == MetaIndexStatus.READ_ONLY) {
Review Comment:
We cannot, because it is in this area that we are protected from changing the low watermark and we are guaranteed not to lose the `IndexMeta`.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4067:
URL: https://github.com/apache/ignite-3/pull/4067#discussion_r1673586494
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -413,17 +301,30 @@ private void onLwmChanged(ChangeLowWatermarkEventParameters parameters) {
inBusyLock(busyLock, () -> {
int lwmCatalogVersion = catalogService.activeCatalogVersion(parameters.newLowWatermark().longValue());
- Iterator<ReadOnlyIndexInfo> it = readOnlyIndexes.iterator();
+ readOnlyIndexes.removeIf(readOnlyIndexInfo -> readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion);
+ });
+ }
- while (it.hasNext()) {
- ReadOnlyIndexInfo readOnlyIndexInfo = it.next();
+ private static long activationTs(IndexMeta indexMeta, MetaIndexStatus status) {
+ return indexMeta.statusChange(status).activationTimestamp();
+ }
- if (readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion) {
- it.remove();
+ private static ReadOnlyIndexInfo toReadOnlyIndexInfo(IndexMeta indexMeta) {
+ assert indexMeta.status() == MetaIndexStatus.READ_ONLY : "indexId=" + indexMeta.indexId() + ", status=" + indexMeta.status();
- tableVersionByIndexId.remove(readOnlyIndexInfo.indexId());
- }
- }
- });
+ long activationTs;
+
+ if (indexMeta.statusChanges().containsKey(MetaIndexStatus.STOPPING)) {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.STOPPING);
+ } else {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.READ_ONLY);
Review Comment:
No need, there is method **org.apache.ignite.internal.table.distributed.index.IndexMeta#statusChangeNullable** for 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4067:
URL: https://github.com/apache/ignite-3/pull/4067#discussion_r1673586494
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -413,17 +301,30 @@ private void onLwmChanged(ChangeLowWatermarkEventParameters parameters) {
inBusyLock(busyLock, () -> {
int lwmCatalogVersion = catalogService.activeCatalogVersion(parameters.newLowWatermark().longValue());
- Iterator<ReadOnlyIndexInfo> it = readOnlyIndexes.iterator();
+ readOnlyIndexes.removeIf(readOnlyIndexInfo -> readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion);
+ });
+ }
- while (it.hasNext()) {
- ReadOnlyIndexInfo readOnlyIndexInfo = it.next();
+ private static long activationTs(IndexMeta indexMeta, MetaIndexStatus status) {
+ return indexMeta.statusChange(status).activationTimestamp();
+ }
- if (readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion) {
- it.remove();
+ private static ReadOnlyIndexInfo toReadOnlyIndexInfo(IndexMeta indexMeta) {
+ assert indexMeta.status() == MetaIndexStatus.READ_ONLY : "indexId=" + indexMeta.indexId() + ", status=" + indexMeta.status();
- tableVersionByIndexId.remove(readOnlyIndexInfo.indexId());
- }
- }
- });
+ long activationTs;
+
+ if (indexMeta.statusChanges().containsKey(MetaIndexStatus.STOPPING)) {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.STOPPING);
+ } else {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.READ_ONLY);
Review Comment:
No need, there is method `org.apache.ignite.internal.table.distributed.index.IndexMeta#statusChangeNullable` for 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
Posted by "Phillippko (via GitHub)" <gi...@apache.org>.
Phillippko commented on code in PR #4067:
URL: https://github.com/apache/ignite-3/pull/4067#discussion_r1673567591
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -413,17 +301,30 @@ private void onLwmChanged(ChangeLowWatermarkEventParameters parameters) {
inBusyLock(busyLock, () -> {
int lwmCatalogVersion = catalogService.activeCatalogVersion(parameters.newLowWatermark().longValue());
- Iterator<ReadOnlyIndexInfo> it = readOnlyIndexes.iterator();
+ readOnlyIndexes.removeIf(readOnlyIndexInfo -> readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion);
+ });
+ }
- while (it.hasNext()) {
- ReadOnlyIndexInfo readOnlyIndexInfo = it.next();
+ private static long activationTs(IndexMeta indexMeta, MetaIndexStatus status) {
+ return indexMeta.statusChange(status).activationTimestamp();
+ }
- if (readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion) {
- it.remove();
+ private static ReadOnlyIndexInfo toReadOnlyIndexInfo(IndexMeta indexMeta) {
+ assert indexMeta.status() == MetaIndexStatus.READ_ONLY : "indexId=" + indexMeta.indexId() + ", status=" + indexMeta.status();
- tableVersionByIndexId.remove(readOnlyIndexInfo.indexId());
- }
- }
- });
+ long activationTs;
+
+ if (indexMeta.statusChanges().containsKey(MetaIndexStatus.STOPPING)) {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.STOPPING);
+ } else {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.READ_ONLY);
Review Comment:
Let's add Nullable to indexMeta.statusChange()? javadoc - "Returns the index status change, {code null} if absent"
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
Posted by "Phillippko (via GitHub)" <gi...@apache.org>.
Phillippko commented on code in PR #4067:
URL: https://github.com/apache/ignite-3/pull/4067#discussion_r1673526468
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -1246,7 +1246,7 @@ public static CompletableFuture<Void> stopAsync(Supplier<CompletableFuture<Void>
* @param components Array of ignite components to stop.
* @return CompletableFuture that will be completed when all components are stopped.
*/
- public static CompletableFuture<Void> stopAsync(ComponentContext componentContext, IgniteComponent... components) {
+ public static CompletableFuture<Void> stopAsync(ComponentContext componentContext, @Nullable IgniteComponent... components) {
Review Comment:
(invalid) IMHO nullable is confusing there - it feels like components can be nullable, and I think we don't have to annotate that varargs can be empty
UPD: my bad, there is a code style rule avoiding confusion
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22682 Use IndexMeta in FullStateTransferIndexChooser [ignite-3]
Posted by "Phillippko (via GitHub)" <gi...@apache.org>.
Phillippko commented on code in PR #4067:
URL: https://github.com/apache/ignite-3/pull/4067#discussion_r1673590303
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/FullStateTransferIndexChooser.java:
##########
@@ -413,17 +301,30 @@ private void onLwmChanged(ChangeLowWatermarkEventParameters parameters) {
inBusyLock(busyLock, () -> {
int lwmCatalogVersion = catalogService.activeCatalogVersion(parameters.newLowWatermark().longValue());
- Iterator<ReadOnlyIndexInfo> it = readOnlyIndexes.iterator();
+ readOnlyIndexes.removeIf(readOnlyIndexInfo -> readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion);
+ });
+ }
- while (it.hasNext()) {
- ReadOnlyIndexInfo readOnlyIndexInfo = it.next();
+ private static long activationTs(IndexMeta indexMeta, MetaIndexStatus status) {
+ return indexMeta.statusChange(status).activationTimestamp();
+ }
- if (readOnlyIndexInfo.indexRemovalCatalogVersion() <= lwmCatalogVersion) {
- it.remove();
+ private static ReadOnlyIndexInfo toReadOnlyIndexInfo(IndexMeta indexMeta) {
+ assert indexMeta.status() == MetaIndexStatus.READ_ONLY : "indexId=" + indexMeta.indexId() + ", status=" + indexMeta.status();
- tableVersionByIndexId.remove(readOnlyIndexInfo.indexId());
- }
- }
- });
+ long activationTs;
+
+ if (indexMeta.statusChanges().containsKey(MetaIndexStatus.STOPPING)) {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.STOPPING);
+ } else {
+ activationTs = activationTs(indexMeta, MetaIndexStatus.READ_ONLY);
Review Comment:
Didn't notice the exception - then javadoc for statusChange() is incorrect, about null if absent))
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org