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/01/26 07:53:21 UTC
[PR] IGNITE-21357 Getting the catalog version in which the index created [ignite-3]
tkalkirill opened a new pull request, #3099:
URL: https://github.com/apache/ignite-3/pull/3099
https://issues.apache.org/jira/browse/IGNITE-21357
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467323842
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -102,4 +102,11 @@ public interface CatalogService extends EventProducer<CatalogEvent, CatalogEvent
* @param version Catalog version to wait for.
*/
CompletableFuture<Void> catalogReadyFuture(int version);
+
+ /**
+ * Returns a local snapshot of all versions of the catalog that exist at the time the method is called.
+ *
+ * <p>NOTE: Collection is sorted by {@link Catalog#version() catalog version} in ascending order.</p>
+ */
+ Collection<Catalog> catalogVersionsSnapshot();
Review Comment:
Since this is a sorted collection, then maybe it should be declared as a List?
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
+ Integer catalogVersion = null;
Review Comment:
I'm pretty sure this variable is redundant
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
Review Comment:
Are we going to clean up old catalog versions somehow in the future? If yes, how is this method going to work?
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -2264,6 +2266,19 @@ void testStartBuildingIndexEvent() {
assertThat(fireEventFuture, willCompleteSuccessfully());
}
+ @Test
+ void testCatalogVersionsSnapshot() {
+ createSomeTable(TABLE_NAME);
+ createSomeIndex(TABLE_NAME, INDEX_NAME);
+
+ List<Catalog> expCatalogs = IntStream.rangeClosed(manager.earliestCatalogVersion(), manager.latestCatalogVersion())
+ .mapToObj(manager::catalog)
+ .collect(toList());
+
+ assertThat(expCatalogs, hasSize(greaterThanOrEqualTo(2)));
Review Comment:
Why is this "greaterThan" and not "equals"?
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
Review Comment:
What do you mean by this note? Why is this important?
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467342586
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
Review Comment:
> Maybe you have a better suggestion?
No, because I don't know how it is supposed to work.
> Yes, old versions of the catalog will be deleted. AssertionError will be thrown.
And how is this going to work? Or do you guarantee somehow that Catalog versions will not be removed when you are calling this method?
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467467576
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
Review Comment:
Okay I deleted it.
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467336139
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
Review Comment:
Yes, old versions of the catalog will be deleted. `AssertionError` will be thrown.
For now, I will only use this method to change the index status from `REGISTERED` to `BUILDING`.
Maybe you have a better suggestion?
I talked to the guys about storing the version of the catalog in which the index was created at `CatalogIndexDescriptor`, but they didn’t support me.
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467338075
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -2264,6 +2266,19 @@ void testStartBuildingIndexEvent() {
assertThat(fireEventFuture, willCompleteSuccessfully());
}
+ @Test
+ void testCatalogVersionsSnapshot() {
+ createSomeTable(TABLE_NAME);
+ createSomeIndex(TABLE_NAME, INDEX_NAME);
+
+ List<Catalog> expCatalogs = IntStream.rangeClosed(manager.earliestCatalogVersion(), manager.latestCatalogVersion())
+ .mapToObj(manager::catalog)
+ .collect(toList());
+
+ assertThat(expCatalogs, hasSize(greaterThanOrEqualTo(2)));
Review Comment:
Since there were two catalog changes, I check that the version must be greater than 2.
Why is it not equal, because at the start of the `CatalogManagerImpl` there is immediately an empty version and in the end there are 3 versions.
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467329664
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
Review Comment:
A note to myself, because when working with a catalog I ask myself questions about the safety of working with its versions. Here I will know that I can safely call this method anywhere and not forget about it.
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467467036
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
+ Integer catalogVersion = null;
Review Comment:
I don't like it because it's a matter of taste. I fixed it differently.
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467467748
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
Review Comment:
Add todos
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467346622
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
Review Comment:
I'm not sure that leaving notes for yourself in documentation is a good idea. It is also inconsistent, because all other methods don't specify that they must be called in some event threads
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467330761
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
+ Integer catalogVersion = null;
Review Comment:
What's your proposal?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -102,4 +102,11 @@ public interface CatalogService extends EventProducer<CatalogEvent, CatalogEvent
* @param version Catalog version to wait for.
*/
CompletableFuture<Void> catalogReadyFuture(int version);
+
+ /**
+ * Returns a local snapshot of all versions of the catalog that exist at the time the method is called.
+ *
+ * <p>NOTE: Collection is sorted by {@link Catalog#version() catalog version} in ascending order.</p>
+ */
+ Collection<Catalog> catalogVersionsSnapshot();
Review Comment:
Why not.
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467343965
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
+ Integer catalogVersion = null;
Review Comment:
```
for (Catalog catalog : catalogService.catalogVersionsSnapshot()) {
CatalogIndexDescriptor indexDescriptor = catalog.index(indexId);
if (indexDescriptor != null && indexDescriptor.status() == REGISTERED) {
return catalog.version();
}
}
throw new AssertionError("Catalog version in which the index created was not found: " + indexId);
```
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467361920
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
+ Integer catalogVersion = null;
Review Comment:
Why?
--
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-21357 Getting the catalog version in which the index created [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3099:
URL: https://github.com/apache/ignite-3/pull/3099#discussion_r1467352401
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManagementUtils.java:
##########
@@ -263,4 +265,31 @@ static boolean isPrimaryReplica(ReplicaMeta primaryReplicaMeta, ClusterNode loca
return localNode.id().equals(primaryReplicaMeta.getLeaseholderId())
&& timestamp.compareTo(primaryReplicaMeta.getExpirationTime()) < 0;
}
+
+
+ /**
+ * Returns the catalog version in which the existing index created.
+ *
+ * <p>NOTE: Can be used outside of catalog/metastore events.</p>
+ *
+ * @param catalogService Catalog service.
+ * @param indexId Index ID of interest.
+ */
+ static int catalogVersionOfIndexCreation(CatalogService catalogService, int indexId) {
+ Integer catalogVersion = null;
Review Comment:
Oh, I don't like your style suggestion.
--
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