Wildcard cluster names for cross cluster search #23985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tim-Brooks
added
:Data Management/Indices APIs
|
This is an initial take at this functionality. But I wanted to create the PR early for early feedback. As listed in this comment there are still some questions to resolve. Additionally, I have not modified the cross cluster qa tests for this functionality yet. However, I plan on doing that once my questions have been resolved. |
|
ping @javanna @s1monw @jasontedor |
|
The build failed because of I added a couple of lines >100. I'll fix that in the next revision or merge in the change back to 140. |
s1monw
suggested changes
Apr 11, 2017
| @@ -79,7 +79,7 @@ | |||
| * {@link RemoteClusterService#REMOTE_CONNECTIONS_PER_CLUSTER} until either all eligible nodes are exhausted or the maximum number of | |||
| * connections per cluster has been reached. | |||
| */ | |||
| final class RemoteClusterConnection extends AbstractComponent implements TransportConnectionListener, Closeable { | |||
| public final class RemoteClusterConnection extends AbstractComponent implements TransportConnectionListener, Closeable { | |||
There was a problem hiding this comment.
can we move this back to be package private?
| if (i >= 0) { | ||
| String remoteClusterName = index.substring(0, i); | ||
| if (isRemoteClusterRegistered(remoteClusterName)) { | ||
| List<String> clusters = clusterNameResolver.resolveClusterNames(remoteClusterNames, remoteClusterName); |
There was a problem hiding this comment.
can we maybe return an emtpy list instead of null, I think we don't necessarily need the null invariant?
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class ClusterNameExpressionResolver extends AbstractComponent { |
|
|
||
| Set<String> matches = matches(remoteClusters, clusterExpression); | ||
| if (matches.isEmpty()) { | ||
| return null; |
There was a problem hiding this comment.
maybe return the empty list?
|
|
||
| public ClusterNameExpressionResolver(Settings settings) { | ||
| super(settings); | ||
| new WildcardExpressionResolver(); |
| } else if (Regex.isSimpleMatchPattern(clusterExpression)) { | ||
| return wildcardResolver.resolve(remoteClusters, clusterExpression); | ||
| } else { | ||
| return null; |
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class ClusterNameExpressionResolver extends AbstractComponent { |
There was a problem hiding this comment.
some basic javadocs for this please?
| new WildcardExpressionResolver(); | ||
| } | ||
|
|
||
| public List<String> resolveClusterNames(Set<String> remoteClusters, String clusterExpression) { |
There was a problem hiding this comment.
also please add some javadocs what this can resolve etc?
s1monw
suggested changes
Apr 11, 2017
| @@ -213,7 +213,7 @@ boolean isRemoteNodeConnected(final String remoteCluster, final DiscoveryNode no | |||
| if (i >= 0) { | |||
| String remoteClusterName = index.substring(0, i); | |||
| List<String> clusters = clusterNameResolver.resolveClusterNames(remoteClusterNames, remoteClusterName); | |||
| if (clusters != null) { | |||
| if (clusters.isEmpty()) { | |||
There was a problem hiding this comment.
hmm this should be cluster.isEmpty() == false?
s1monw
approved these changes
Apr 11, 2017
|
LGTM |
Tim-Brooks
added a commit
that referenced
this pull request
Apr 11, 2017
This is related to #23893. This commit allows users to use wilcards for cluster names when executing a cross cluster search. So instead of defining every cluster such as: GET one:*,two:*,three:*/_search A user could just search: GET *:*/_search As ":" characters are currently allowed in index names, if the text up to the first ":" does not match a defined cluster name, the entire string is treated as an index name.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Apr 13, 2017
* master: (41 commits) Remove awaits fix from evil JNA native tests Correct handling of default and array settings Build: Switch jna dependency to an elastic version (elastic#24081) fix CategoryContextMappingTests compilation bugs testConcurrentGetAndSetOnPrimary - fix a race condition between indexing and updating value map Allow different data types for category in Context suggester (elastic#23491) Restrict build info loading to ES jar, not any jar (elastic#24049) Remove more hidden file leniency from plugins Register error listener in evil logger tests Detect using logging before configuration [DOCS] Added note about Elastic Cloud to improve 'elastic aws' SERP results. Add version constant for 5.5 (elastic#24075) Add unit tests for NestedAggregator (elastic#24054) Add more debugging information to rethrottles Tests: Use random analyzer only on string fields in Match/MultiMatchBuilderTests Cleanup outdated comments for fixing up pom dependencies (elastic#24056) S3 Repository: Eagerly load static settings (elastic#23910) Reject duplicate settings on the command line Wildcard cluster names for cross cluster search (elastic#23985) Update scripts/security docs for sandboxed world (elastic#23977) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is related to #23893. This commit allows users to use wilcards for
cluster names when executing a cross cluster search.
So instead of defining every cluster such as:
GET one:*,two:*,three:*/_searchA user could just search:
GET *:*/_searchAs ":" characters are currently allowed in index names, if the text
up to the first ":" does not match a defined cluster name, the entire
string is treated as an index name.
Closes #23893