You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/04/28 14:51:26 UTC
[GitHub] [ignite] Erixonich opened a new pull request #9061: IGNITE-14469 Add test that ensures correctness of sequential call index_force_rebuild, some refactoring
Erixonich opened a new pull request #9061:
URL: https://github.com/apache/ignite/pull/9061
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] 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 pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _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.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] tkalkirill commented on a change in pull request #9061: IGNITE-14469 Add test that ensures correctness of sequential call index_force_rebuild, some refactoring
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #9061:
URL: https://github.com/apache/ignite/pull/9061#discussion_r624000421
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java
##########
@@ -4463,6 +4463,7 @@ public void completeProxyInitialize(String name) {
* @param <K> type of keys.
* @param <V> type of values.
* @return Cache instance for given name.
+ * @throws IllegalArgumentException if cache not exists.
Review comment:
If cache not exists.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexForceRebuildTask.java
##########
@@ -68,111 +65,60 @@ protected IndexForceRebuildJob(@Nullable IndexForceRebuildTaskArg arg, boolean d
throws IgniteException
{
//Either cacheNames or cacheGrps must be specified
- assert (arg.cacheNames() == null) != (arg.cacheGrps() == null) :
- "Either cacheNames or cacheGroups must be specified";
-
- // Collect info about indexes being rebuilt.
- Set<IndexRebuildStatusInfoContainer> rebuildIdxCaches =
- ignite.context().cache().publicCaches()
- .stream()
- .filter(c -> !c.indexReadyFuture().isDone())
- .map(this::fromIgniteCache)
- .collect(Collectors.toSet());
-
- Set<String> rebuildIdxCachesNames = rebuildIdxCaches.stream()
- .map(IndexRebuildStatusInfoContainer::cacheName)
- .collect(Collectors.toSet());
+ assert (arg.cacheNames() == null) ^ (arg.cacheGrps() == null) :
+ "Either cacheNames or cacheGroups must be specified.";
- if (arg.cacheNames() != null)
- return rebuildByCacheNames(arg.cacheNames(), rebuildIdxCaches, rebuildIdxCachesNames);
- else if (arg.cacheGrps() != null)
- return rebuildByGroupNames(arg.cacheGrps(), rebuildIdxCaches, rebuildIdxCachesNames);
- else {
- assert false : "Neither cache names nor cache groups specified";
+ if (arg.cacheNames() == null && arg.cacheGrps() == null) {
+ assert false : "Neither cache names nor cache groups specified.";
return null;
}
- }
-
- /**
- * Triggers force rebuild of indexes in caches from {@code cacheNames}.
- *
- * @param cacheNames Set of cache names.
- * @param rebuildIdxCaches Set of infos about cached which have indexes being rebuilt at the moment.
- * @param rebuildIdxCachesNames Set of names of cached which have indexes being rebuilt at the moment.
- * @return {@code IndexForceRebuildTaskRes} object.
- */
- @NotNull private IndexForceRebuildTaskRes rebuildByCacheNames(
- Set<String> cacheNames,
- Set<IndexRebuildStatusInfoContainer> rebuildIdxCaches,
- Set<String> rebuildIdxCachesNames)
- {
- final GridCacheProcessor cacheProcessor = ignite.context().cache();
- // Collect info about not found caches.
- Set<String> notFoundCaches = new HashSet<>(cacheNames);
- notFoundCaches.removeIf(name -> cacheProcessor.cache(name) != null);
-
- Set<GridCacheContext> cacheContexts =
- cacheProcessor.publicCaches()
- .stream()
- .filter(c -> !rebuildIdxCachesNames.contains(c.getName()))
- .filter(c -> cacheNames.contains(c.getName()))
- .map(IgniteCacheProxy::context)
- .collect(Collectors.toSet());
+ Set<GridCacheContext> cachesToRebuild = new HashSet<>();
+ Set<String> notFound = new HashSet<>();
- // Collect info about started index rebuild.
- Set<IndexRebuildStatusInfoContainer> cachesWithStartedRebuild =
- cacheContexts.stream()
- .map(c -> new IndexRebuildStatusInfoContainer(c.config()))
- .collect(Collectors.toSet());
+ final GridCacheProcessor cacheProcessor = ignite.context().cache();
- cacheProcessor.context().database().forceRebuildIndexes(cacheContexts);
+ if (arg.cacheNames() != null) {
+ for (String cacheName : arg.cacheNames()) {
+ IgniteInternalCache<Object, Object> cache = cacheProcessor.cache(cacheName);
Review comment:
<Object, Object> - not needed
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexForceRebuildTask.java
##########
@@ -68,111 +65,60 @@ protected IndexForceRebuildJob(@Nullable IndexForceRebuildTaskArg arg, boolean d
throws IgniteException
{
//Either cacheNames or cacheGrps must be specified
- assert (arg.cacheNames() == null) != (arg.cacheGrps() == null) :
- "Either cacheNames or cacheGroups must be specified";
-
- // Collect info about indexes being rebuilt.
- Set<IndexRebuildStatusInfoContainer> rebuildIdxCaches =
- ignite.context().cache().publicCaches()
- .stream()
- .filter(c -> !c.indexReadyFuture().isDone())
- .map(this::fromIgniteCache)
- .collect(Collectors.toSet());
-
- Set<String> rebuildIdxCachesNames = rebuildIdxCaches.stream()
- .map(IndexRebuildStatusInfoContainer::cacheName)
- .collect(Collectors.toSet());
+ assert (arg.cacheNames() == null) ^ (arg.cacheGrps() == null) :
+ "Either cacheNames or cacheGroups must be specified.";
- if (arg.cacheNames() != null)
- return rebuildByCacheNames(arg.cacheNames(), rebuildIdxCaches, rebuildIdxCachesNames);
- else if (arg.cacheGrps() != null)
- return rebuildByGroupNames(arg.cacheGrps(), rebuildIdxCaches, rebuildIdxCachesNames);
- else {
- assert false : "Neither cache names nor cache groups specified";
+ if (arg.cacheNames() == null && arg.cacheGrps() == null) {
+ assert false : "Neither cache names nor cache groups specified.";
return null;
}
- }
-
- /**
- * Triggers force rebuild of indexes in caches from {@code cacheNames}.
- *
- * @param cacheNames Set of cache names.
- * @param rebuildIdxCaches Set of infos about cached which have indexes being rebuilt at the moment.
- * @param rebuildIdxCachesNames Set of names of cached which have indexes being rebuilt at the moment.
- * @return {@code IndexForceRebuildTaskRes} object.
- */
- @NotNull private IndexForceRebuildTaskRes rebuildByCacheNames(
- Set<String> cacheNames,
- Set<IndexRebuildStatusInfoContainer> rebuildIdxCaches,
- Set<String> rebuildIdxCachesNames)
- {
- final GridCacheProcessor cacheProcessor = ignite.context().cache();
- // Collect info about not found caches.
- Set<String> notFoundCaches = new HashSet<>(cacheNames);
- notFoundCaches.removeIf(name -> cacheProcessor.cache(name) != null);
-
- Set<GridCacheContext> cacheContexts =
- cacheProcessor.publicCaches()
- .stream()
- .filter(c -> !rebuildIdxCachesNames.contains(c.getName()))
- .filter(c -> cacheNames.contains(c.getName()))
- .map(IgniteCacheProxy::context)
- .collect(Collectors.toSet());
+ Set<GridCacheContext> cachesToRebuild = new HashSet<>();
+ Set<String> notFound = new HashSet<>();
- // Collect info about started index rebuild.
- Set<IndexRebuildStatusInfoContainer> cachesWithStartedRebuild =
- cacheContexts.stream()
- .map(c -> new IndexRebuildStatusInfoContainer(c.config()))
- .collect(Collectors.toSet());
+ final GridCacheProcessor cacheProcessor = ignite.context().cache();
- cacheProcessor.context().database().forceRebuildIndexes(cacheContexts);
+ if (arg.cacheNames() != null) {
+ for (String cacheName : arg.cacheNames()) {
+ IgniteInternalCache<Object, Object> cache = cacheProcessor.cache(cacheName);
- return new IndexForceRebuildTaskRes(cachesWithStartedRebuild, rebuildIdxCaches, notFoundCaches);
- }
+ if (cache != null)
+ cachesToRebuild.add(cache.context());
+ else
+ notFound.add(cacheName);
+ }
+ }
+ else { //arg.cacheGrps() != null
Review comment:
//arg.cacheGrps() != null - not needed
##########
File path: modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerIndexForceRebuildTest.java
##########
@@ -476,40 +500,95 @@ public void testCorruptedIndexRebuild() throws Exception {
}
/**
- * Validated control.sh utility output for {@link #testCacheNamesArg()}.
+ * Validates control.sh output when caches by name not found.
+ *
+ * @param outputStr control.sh output.
+ * @param cacheNames cache names to print.
+ */
+ private void validateOutputCacheNamesNotFound(String outputStr, String... cacheNames) {
+ assertContains(
+ log,
+ outputStr,
+ "WARNING: These caches were not found:" + U.nl() + makeStringListWithIndent(cacheNames)
+ );
+ }
+
+ /**
+ * Validates control.sh output when caches by group not found.
+ *
+ * @param outputStr control.sh output.
+ * @param cacheGrps cache groups to print.
+ */
+ private void validateOutputCacheGroupsNotFound(String outputStr, String... cacheGrps) {
+ assertContains(
+ log,
+ outputStr,
+ "WARNING: These cache groups were not found:" + U.nl() + makeStringListWithIndent(cacheGrps)
+ );
+ }
+
+ /**
+ * Makes new-line list with indent.
+ * @param strings list of strings.
+ * @return formated text.
*/
- private void validateTestCacheNamesArgOutput() {
- String outputStr = testOut.toString();
+ private String makeStringListWithIndent(String... strings) {
+ return INDENT + String.join(U.nl() + INDENT, strings);
+ }
- assertTrue(outputStr.contains("WARNING: These caches were not found:\n" +
- " " + CACHE_NAME_NON_EXISTING));
+ /**
+ * Makes formatted text for given caches.
+ *
+ * @param cacheGroputToNames Cache groups mapping to non-existing cache names.
+ * @return text for CLI print output for given caches.
+ */
+ private String makeStringListForCacheGroupsAndNames(Map<String, List<String>> cacheGroputToNames) {
+ StringBuilder sb = new StringBuilder();
Review comment:
You can use org.apache.ignite.internal.util.typedef.internal.SB
##########
File path: modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerIndexForceRebuildTest.java
##########
@@ -476,40 +500,95 @@ public void testCorruptedIndexRebuild() throws Exception {
}
/**
- * Validated control.sh utility output for {@link #testCacheNamesArg()}.
+ * Validates control.sh output when caches by name not found.
+ *
+ * @param outputStr control.sh output.
+ * @param cacheNames cache names to print.
+ */
+ private void validateOutputCacheNamesNotFound(String outputStr, String... cacheNames) {
+ assertContains(
+ log,
+ outputStr,
+ "WARNING: These caches were not found:" + U.nl() + makeStringListWithIndent(cacheNames)
+ );
+ }
+
+ /**
+ * Validates control.sh output when caches by group not found.
+ *
+ * @param outputStr control.sh output.
+ * @param cacheGrps cache groups to print.
+ */
+ private void validateOutputCacheGroupsNotFound(String outputStr, String... cacheGrps) {
+ assertContains(
+ log,
+ outputStr,
+ "WARNING: These cache groups were not found:" + U.nl() + makeStringListWithIndent(cacheGrps)
+ );
+ }
+
+ /**
+ * Makes new-line list with indent.
+ * @param strings list of strings.
+ * @return formated text.
*/
- private void validateTestCacheNamesArgOutput() {
- String outputStr = testOut.toString();
+ private String makeStringListWithIndent(String... strings) {
+ return INDENT + String.join(U.nl() + INDENT, strings);
+ }
- assertTrue(outputStr.contains("WARNING: These caches were not found:\n" +
- " " + CACHE_NAME_NON_EXISTING));
+ /**
+ * Makes formatted text for given caches.
+ *
+ * @param cacheGroputToNames Cache groups mapping to non-existing cache names.
+ * @return text for CLI print output for given caches.
Review comment:
With a capital letter.
##########
File path: modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerIndexForceRebuildTest.java
##########
@@ -476,40 +500,95 @@ public void testCorruptedIndexRebuild() throws Exception {
}
/**
- * Validated control.sh utility output for {@link #testCacheNamesArg()}.
+ * Validates control.sh output when caches by name not found.
+ *
+ * @param outputStr control.sh output.
+ * @param cacheNames cache names to print.
+ */
+ private void validateOutputCacheNamesNotFound(String outputStr, String... cacheNames) {
+ assertContains(
+ log,
+ outputStr,
+ "WARNING: These caches were not found:" + U.nl() + makeStringListWithIndent(cacheNames)
+ );
+ }
+
+ /**
+ * Validates control.sh output when caches by group not found.
+ *
+ * @param outputStr control.sh output.
Review comment:
Capitalized argument names.
##########
File path: modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerIndexForceRebuildTest.java
##########
@@ -476,40 +500,95 @@ public void testCorruptedIndexRebuild() throws Exception {
}
/**
- * Validated control.sh utility output for {@link #testCacheNamesArg()}.
+ * Validates control.sh output when caches by name not found.
+ *
+ * @param outputStr control.sh output.
Review comment:
Capitalized argument names.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexForceRebuildTask.java
##########
@@ -68,111 +65,60 @@ protected IndexForceRebuildJob(@Nullable IndexForceRebuildTaskArg arg, boolean d
throws IgniteException
{
//Either cacheNames or cacheGrps must be specified
- assert (arg.cacheNames() == null) != (arg.cacheGrps() == null) :
- "Either cacheNames or cacheGroups must be specified";
-
- // Collect info about indexes being rebuilt.
- Set<IndexRebuildStatusInfoContainer> rebuildIdxCaches =
- ignite.context().cache().publicCaches()
- .stream()
- .filter(c -> !c.indexReadyFuture().isDone())
- .map(this::fromIgniteCache)
- .collect(Collectors.toSet());
-
- Set<String> rebuildIdxCachesNames = rebuildIdxCaches.stream()
- .map(IndexRebuildStatusInfoContainer::cacheName)
- .collect(Collectors.toSet());
+ assert (arg.cacheNames() == null) ^ (arg.cacheGrps() == null) :
+ "Either cacheNames or cacheGroups must be specified.";
- if (arg.cacheNames() != null)
- return rebuildByCacheNames(arg.cacheNames(), rebuildIdxCaches, rebuildIdxCachesNames);
- else if (arg.cacheGrps() != null)
- return rebuildByGroupNames(arg.cacheGrps(), rebuildIdxCaches, rebuildIdxCachesNames);
- else {
- assert false : "Neither cache names nor cache groups specified";
+ if (arg.cacheNames() == null && arg.cacheGrps() == null) {
+ assert false : "Neither cache names nor cache groups specified.";
Review comment:
Don't do that: **assert false**
##########
File path: modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerIndexForceRebuildTest.java
##########
@@ -657,4 +736,54 @@ private void removeLogListener(IgniteEx ignite, LogListener lsnr) {
return idxRebuildFuts.get(cacheId);
}
+
+ /**
+ * Force rebuilds indices for chosen caches, and waits until rebuild process is complete.
+ *
+ * @param cacheNames Cache names need indices to rebuild.
+ * @param grid Ignite node.
+ * @throws Exception if failed.
+ */
+ private void forceRebuildIndices(Iterable<String> cacheNames, IgniteEx grid) throws Exception {
+ String cacheNamesArg = String.join(",", cacheNames);
+
+ assertEquals(
+ EXIT_CODE_OK,
+ execute(
+ "--cache", "indexes_force_rebuild",
+ "--node-id", grid.localNode().id().toString(),
+ "--cache-names", cacheNamesArg
+ )
+ );
+
+ waitForIndexesRebuild(grid, getTestTimeout(), Collections.emptyList());
+ }
+
+ /**
+ * Checking that a sequence of forced rebuild of indexes is possible
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testSequentialForceRebuildIndexes() throws Exception {
Review comment:
Let the test methods be close to each other, move to line 502.
##########
File path: modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerIndexForceRebuildTest.java
##########
@@ -657,4 +736,54 @@ private void removeLogListener(IgniteEx ignite, LogListener lsnr) {
return idxRebuildFuts.get(cacheId);
}
+
+ /**
+ * Force rebuilds indices for chosen caches, and waits until rebuild process is complete.
+ *
+ * @param cacheNames Cache names need indices to rebuild.
+ * @param grid Ignite node.
+ * @throws Exception if failed.
Review comment:
With a capital letter.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexForceRebuildTask.java
##########
@@ -68,111 +65,60 @@ protected IndexForceRebuildJob(@Nullable IndexForceRebuildTaskArg arg, boolean d
throws IgniteException
{
//Either cacheNames or cacheGrps must be specified
- assert (arg.cacheNames() == null) != (arg.cacheGrps() == null) :
- "Either cacheNames or cacheGroups must be specified";
-
- // Collect info about indexes being rebuilt.
- Set<IndexRebuildStatusInfoContainer> rebuildIdxCaches =
- ignite.context().cache().publicCaches()
- .stream()
- .filter(c -> !c.indexReadyFuture().isDone())
- .map(this::fromIgniteCache)
- .collect(Collectors.toSet());
-
- Set<String> rebuildIdxCachesNames = rebuildIdxCaches.stream()
- .map(IndexRebuildStatusInfoContainer::cacheName)
- .collect(Collectors.toSet());
+ assert (arg.cacheNames() == null) ^ (arg.cacheGrps() == null) :
+ "Either cacheNames or cacheGroups must be specified.";
- if (arg.cacheNames() != null)
- return rebuildByCacheNames(arg.cacheNames(), rebuildIdxCaches, rebuildIdxCachesNames);
- else if (arg.cacheGrps() != null)
- return rebuildByGroupNames(arg.cacheGrps(), rebuildIdxCaches, rebuildIdxCachesNames);
- else {
- assert false : "Neither cache names nor cache groups specified";
+ if (arg.cacheNames() == null && arg.cacheGrps() == null) {
Review comment:
This condition will not work because there is a XOR above: https://en.wikipedia.org/wiki/Exclusive_or#Truth_table
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Erixonich commented on a change in pull request #9061: IGNITE-14469 Add test that ensures correctness of sequential call index_force_rebuild, some refactoring
Posted by GitBox <gi...@apache.org>.
Erixonich commented on a change in pull request #9061:
URL: https://github.com/apache/ignite/pull/9061#discussion_r625660154
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexForceRebuildTask.java
##########
@@ -68,111 +65,60 @@ protected IndexForceRebuildJob(@Nullable IndexForceRebuildTaskArg arg, boolean d
throws IgniteException
{
//Either cacheNames or cacheGrps must be specified
- assert (arg.cacheNames() == null) != (arg.cacheGrps() == null) :
- "Either cacheNames or cacheGroups must be specified";
-
- // Collect info about indexes being rebuilt.
- Set<IndexRebuildStatusInfoContainer> rebuildIdxCaches =
- ignite.context().cache().publicCaches()
- .stream()
- .filter(c -> !c.indexReadyFuture().isDone())
- .map(this::fromIgniteCache)
- .collect(Collectors.toSet());
-
- Set<String> rebuildIdxCachesNames = rebuildIdxCaches.stream()
- .map(IndexRebuildStatusInfoContainer::cacheName)
- .collect(Collectors.toSet());
+ assert (arg.cacheNames() == null) ^ (arg.cacheGrps() == null) :
+ "Either cacheNames or cacheGroups must be specified.";
- if (arg.cacheNames() != null)
- return rebuildByCacheNames(arg.cacheNames(), rebuildIdxCaches, rebuildIdxCachesNames);
- else if (arg.cacheGrps() != null)
- return rebuildByGroupNames(arg.cacheGrps(), rebuildIdxCaches, rebuildIdxCachesNames);
- else {
- assert false : "Neither cache names nor cache groups specified";
+ if (arg.cacheNames() == null && arg.cacheGrps() == null) {
Review comment:
This is should work without -ea launching option.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] asfgit closed pull request #9061: IGNITE-14469 Add test that ensures correctness of sequential call index_force_rebuild, some refactoring
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #9061:
URL: https://github.com/apache/ignite/pull/9061
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org