New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNITE-17002 Control.sh command to schedule index rebuild in Maintenance Mode #10042
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand org.apache.ignite.internal.processors.query.h2.maintenance.RebuildIndexAction is not about planning the rebuilding of indexes, but still about the real rebuilding of indexes, did I understand correctly?
...rol-utility/src/main/java/org/apache/ignite/internal/commandline/cache/CacheCommandList.java
Outdated
Show resolved
Hide resolved
...-utility/src/main/java/org/apache/ignite/internal/commandline/cache/CacheIndexesRebuild.java
Outdated
Show resolved
Hide resolved
...-utility/src/main/java/org/apache/ignite/internal/commandline/cache/CacheIndexesRebuild.java
Outdated
Show resolved
Hide resolved
...-utility/src/main/java/org/apache/ignite/internal/commandline/cache/CacheIndexesRebuild.java
Outdated
Show resolved
Hide resolved
desc, | ||
map, | ||
NODE_ID.argName() + " nodeId", | ||
TARGET + " cacheName1=index1,...indexN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to have two arguments and have them in the following options:
- --cache-names=cache0,cache1[idx0,idx2]
- --group-names=grp0,grp1[idx0,idx2]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need group names? Indexes belong to caches, not groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for groups, you can remove the index refinement, the idea is that you need to give the opportunity to rebuild the indexes for all caches in the group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this, just don't specify any indexes: "(Optional) Specify node for indexes rebuild."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you want to rebuild indexes for all caches in the group? Let's not overcomplicate things
...es/indexing/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexRebuildTask.java
Outdated
Show resolved
Hide resolved
...es/indexing/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexRebuildTask.java
Outdated
Show resolved
Hide resolved
...es/indexing/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexRebuildTask.java
Outdated
Show resolved
Hide resolved
...es/indexing/src/main/java/org/apache/ignite/internal/visor/cache/index/IndexRebuildTask.java
Outdated
Show resolved
Hide resolved
...he/ignite/internal/processors/query/h2/maintenance/MaintenanceRebuildIndexUtilsSelfTest.java
Outdated
Show resolved
Hide resolved
3503442
to
4411355
Compare
.../src/main/java/org/apache/ignite/internal/commandline/cache/CacheScheduleIndexesRebuild.java
Show resolved
Hide resolved
.../src/main/java/org/apache/ignite/internal/commandline/cache/CacheScheduleIndexesRebuild.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/ignite/internal/commandline/cache/CacheScheduleIndexesRebuild.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/ignite/internal/commandline/cache/CacheScheduleIndexesRebuild.java
Show resolved
Hide resolved
/** */ | ||
private Arguments(UUID nodeId, Map<String, Set<String>> cacheToIndexes, Set<String> cacheGroups) { | ||
this.nodeId = nodeId; | ||
this.cacheToIndexes = cacheToIndexes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defensive copies? This is not executed at 1000 rps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, we'll have to make a deep copy in case of cacheToIndexes. Not sure if it's really necessary here...
...pache/ignite/internal/cache/query/index/sorted/maintenance/MaintenanceRebuildIndexUtils.java
Outdated
Show resolved
Hide resolved
...utility/src/test/java/org/apache/ignite/util/GridCommandHandlerScheduleIndexRebuildTest.java
Outdated
Show resolved
Hide resolved
...utility/src/test/java/org/apache/ignite/util/GridCommandHandlerScheduleIndexRebuildTest.java
Show resolved
Hide resolved
* @param cache Cache name. | ||
* @return Indexes of the cache. | ||
*/ | ||
private Set<String> indexes(IgniteEx node, String cache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code is duplicated. Could we extract it to some util class to avoid duplication?
...es/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerIndexingUtils.java
Show resolved
Hide resolved
LGTM |
https://issues.apache.org/jira/browse/IGNITE-17002
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
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summary
whereXXXX
- number of JIRA issue.(see the Maintainers list)
the
green visa
attached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.