You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sashapolo (via GitHub)" <gi...@apache.org> on 2024/01/03 10:11:50 UTC
[PR] IGNITE-21191 Wait for indexes using a node's clock [ignite-3]
sashapolo opened a new pull request, #3005:
URL: https://github.com/apache/ignite-3/pull/3005
https://issues.apache.org/jira/browse/IGNITE-21191
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440333328
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
if you use default 10ms, why need to explicitly set 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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440332697
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
+ 30_000L
Review Comment:
why so long?
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440427417
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
Could you imaging real situatio when index will became available for so long time for our tests? Why we don't use 10 hours here in case it's not a problem?
My point is the following: in case our code have bugs all tests will waiting long time and significantly increasing time of whole build. We should use values that make sense.
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440402959
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
I copy-pasted it from a similar method in this class...
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440420520
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
It would look inconsistent, that the other method that also waits for indices uses a different timing
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440403216
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
+ 30_000L
Review Comment:
see the answer below
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
I copy-pasted it from a similar method in this class... Should I change 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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440404814
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
copy-past - it's bad explanation.
We must do it consciously
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440407611
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
previous approach here looks better
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440412546
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
Review Comment:
indexDescriptor can be null?
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440442228
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
Review Comment:
Sure
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo merged PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440413535
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
Review Comment:
Could you please add some comment to explain why we do it by such way
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440412546
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
Review Comment:
indexDescriptor can be null?
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1441475540
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
Changed to 10 seconds
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440422193
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
Why other method waits so long? As I see you was single reviewer for that change )
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
Why other method waits so long? As I see you was a single reviewer for that change )
--
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-21191 Wait for indexes using a node's clock [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3005:
URL: https://github.com/apache/ignite-3/pull/3005#discussion_r1440423883
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/ClusterPerClassIntegrationTest.java:
##########
@@ -386,38 +384,37 @@ private static List<List<Object>> executeAwaitingIndexes(IgniteImpl node, Functi
CatalogManager catalogManager = node.catalogManager();
// Get existing indexes
- Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion())
- .stream().map(CatalogObjectDescriptor::id)
+ Set<Integer> existing = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ .map(CatalogObjectDescriptor::id)
.collect(Collectors.toSet());
List<List<Object>> result = statement.apply(node);
// Get indexes after a statement and compute the difference
- Set<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
+ List<Integer> difference = catalogManager.indexes(catalogManager.latestCatalogVersion()).stream()
.map(CatalogObjectDescriptor::id)
- .collect(Collectors.toSet());
-
- difference.removeAll(existing);
+ .filter(id -> !existing.contains(id))
+ .collect(Collectors.toList());
if (difference.isEmpty()) {
return result;
}
// If there are new indexes, wait for them to become available.
+ HybridClock clock = node.clock();
try {
- assertTrue(waitForCondition(() -> {
- int latestVersion = catalogManager.latestCatalogVersion();
- int notAvailable = 0;
-
- for (CatalogIndexDescriptor index : catalogManager.indexes(latestVersion)) {
- if (!index.available() && difference.contains(index.id())) {
- notAvailable++;
- }
- }
-
- return notAvailable == 0;
- }, 10_000));
+ assertTrue(waitForCondition(
+ () -> {
+ long now = clock.nowLong();
+
+ return difference.stream()
+ .map(id -> catalogManager.index(id, now))
+ .allMatch(indexDescriptor -> indexDescriptor != null && indexDescriptor.available());
+ },
+ 10,
Review Comment:
Why is that a big deal? I don't think 10 seconds is better than 30 seconds in this context
--
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