Add remote read clusters option to elasticsearch to enable cross-cluster querying #1892

allen13 · 2019-11-01T13:08:10Z

Which problem is this PR solving?

Enables elasticsearch cross-cluster querying

Resolves #1883

Short description of the changes

The cross cluster query api functions by appending a pre-configured remote cluster to an index e.g. cluster_one:index_name. This change simply applies a prefix for each remote cluster and for each index. You will end up with total_indicies = indicies * remote_clusters. Also added a special case local which will not append a prefix and query the local cluster.

This works with both standard and rollover indices since it mostly comes down to a simple string manipulation.

codecov · 2019-11-01T13:58:59Z

Codecov Report

Merging #1892 into master will decrease coverage by 0.01%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #1892      +/-   ##
==========================================
- Coverage   98.44%   98.42%   -0.02%     
==========================================
  Files         197      197              
  Lines        9648     9675      +27     
==========================================
+ Hits         9498     9523      +25     
- Misses        114      115       +1     
- Partials       36       37       +1

Impacted Files	Coverage Δ
plugin/storage/es/options.go	`99.02% <88.23%> (-0.98%)`	⬇️
plugin/storage/es/factory.go	`100.00% <100.00%> (ø)`
plugin/storage/es/spanstore/reader.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dd338d...6de969d. Read the comment docs.

pavolloffay · 2019-11-04T15:33:58Z

plugin/storage/es/options.go

+		nsCfg.remoteReadClusters = opt.primary.remoteReadClusters
+	}
+
+	if nsCfg.remoteReadClusters == "" {


can be omitted and inner block added to line 314

pavolloffay · 2019-11-04T15:43:19Z

plugin/storage/es/spanstore/reader.go

+	return func(indices string, startTime time.Time, endTime time.Time) []string {
+		jaegerIndices := fn(indices, startTime, endTime)
+		if len(remoteReadClusters) > 0 {
+			remoteJaegerIndices := []string{}


We know the size at creation time

Do you want the array to be statically sized?
remoteJaegerIndices := [size]string{}

After removing local just appended to the jaegerIndices to simplify things.

pavolloffay · 2019-11-04T15:44:44Z

plugin/storage/es/spanstore/reader.go

+			for _, remoteCluster := range remoteReadClusters {
+				for _, jaegerIndex := range jaegerIndices {
+
+					if remoteCluster != "local" {


Can we avoid this? We can require not adding it via flags.

Some options:
--es.remote-read-clusters-remote-only
--es.remote-read-clusters-no-local

What do you think?

I might be missing something, but why two flags? The names on the flag will be added to indices. If local is added it will be added. if it is not added it won't be added. I just want to avoid unnecessary checks in the reader.

Ahh, I misunderstood. Are you suggesting we always keep the local un-prefixed index by default? I really struggled to figure out how we should toggle on and off the local cluster reference. I've seen use cases where you have an elastic query node that only has remote clusters and no data local.

Current:
Input: --es.remote-read-clusters=local,cluster_one
Output: /twitter,cluster_one:twitter
Alternate?:
Input: --es.remote-read-clusters=cluster_one
Output: /twitter,cluster_one:twitter

I've seen use cases where you have an elastic query node that only has remote clusters and no data local.

can you provide any references to this use-case?

Then I guess we cannot go with the following as it will always add local data.

Alternate?: Input: --es.remote-read-clusters=cluster_one Output: /twitter,cluster_one:twitter

Thanks for the input! This does make things much simpler! The no local use case can be addressed in the future if anyone ever needs it.

pavolloffay · 2019-11-04T15:45:45Z

plugin/storage/es/spanstore/reader_test.go

-			index: "foo:" + indexPrefixSeparator + spanIndex + archiveReadIndexSuffix},
+			indices: []string{"foo:" + indexPrefixSeparator + spanIndex + archiveReadIndexSuffix}},
+		{params: SpanReaderParams{Client: client, Logger: logger, MetricsFactory: metricsFactory,
+			IndexPrefix: "", Archive: false, RemoteReadClusters: []string{"local", "cluster_one"}},


could you add a test with two remote clusters?

pavolloffay · 2019-11-04T15:49:12Z

@allen13 does this switch work for ES 5, 6 and 7?

allen13 · 2019-11-04T18:14:04Z

@pavolloffay Cross-cluster query support was added in Elasticserch 5.4. https://www.elastic.co/guide/en/elasticsearch/reference/5.5/modules-cross-cluster-search.html

…ter querying Signed-off-by: allen13 <[email protected]>

yurishkuro · 2019-11-11T17:44:24Z

plugin/storage/es/options.go

@@ -278,6 +287,13 @@ func initFromViper(cfg *namespaceConfig, v *viper.Viper) {
 // GetPrimary returns primary configuration.
 func (opt *Options) GetPrimary() *config.Configuration {
 	opt.primary.Servers = strings.Split(opt.primary.servers, ",")
+
+	if opt.primary.remoteReadClusters == "" {
+		opt.primary.RemoteReadClusters = []string{}


this branch is not necessary

yurishkuro · 2019-11-11T17:44:38Z

plugin/storage/es/options.go

@@ -293,6 +309,13 @@ func (opt *Options) Get(namespace string) *config.Configuration {
 		nsCfg.servers = opt.primary.servers
 	}
 	nsCfg.Servers = strings.Split(nsCfg.servers, ",")
+
+	if nsCfg.remoteReadClusters == "" {
+		nsCfg.RemoteReadClusters = []string{}


branch not necessary

yurishkuro · 2019-11-11T17:47:09Z

plugin/storage/es/spanstore/reader.go

+// Elasticsearch cross cluster query api example: GET /twitter,cluster_one:twitter,cluster_two:twitter/_search
+// Add a remote cluster prefix for each cluster and for each index and add it to the list of original indices
+func addRemoteReadClusters(fn timeRangeIndexFn, remoteReadClusters []string) timeRangeIndexFn {
+	return func(indices string, startTime time.Time, endTime time.Time) []string {


shouldn't this be short-circuited to return fn if len(remoteReadClusters) == 0?

yurishkuro · 2019-11-11T17:48:09Z

plugin/storage/es/spanstore/reader_test.go

+			indices: []string{"foo:" + indexPrefixSeparator + spanIndex + archiveReadIndexSuffix}},
+		{params: SpanReaderParams{Client: client, Logger: logger, MetricsFactory: metricsFactory,
+			IndexPrefix: "", Archive: false, RemoteReadClusters: []string{"cluster_one", "cluster_two"}},
+			indices: []string{spanIndex + dateFormat, "cluster_one:" + spanIndex + dateFormat, "cluster_two:" + spanIndex + dateFormat}},


please make this stmt multi-line, one cluster per line

dgrizzanti · 2021-03-11T13:15:31Z

@allen13 @pavolloffay I'd be really interested in seeing this PR merged. Do you know if anyone is actively looking into this? I'd be happy to try and get this ready otherwise if there is still willingness for the feature. Thanks!

jpkrohling · 2021-08-23T08:18:57Z

Closing, as I think the linked PR replaced this one.

allen13 requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners November 1, 2019 13:08

allen13 force-pushed the remote-read-clusters branch from 1e4750e to d4a5fc4 Compare November 1, 2019 13:09

allen13 mentioned this pull request Nov 1, 2019

Elasticsearch Cross Cluster Query #1883

Closed

pavolloffay added the storage/elasticsearch label Nov 4, 2019

pavolloffay reviewed Nov 4, 2019

View reviewed changes

allen13 force-pushed the remote-read-clusters branch from d4a5fc4 to 5d40fa0 Compare November 6, 2019 13:47

Add remote read clusters option to elasticsearch to enable cross-clus…

6de969d

…ter querying Signed-off-by: allen13 <[email protected]>

allen13 force-pushed the remote-read-clusters branch from 5d40fa0 to 6de969d Compare November 6, 2019 13:50

yurishkuro reviewed Nov 11, 2019

View reviewed changes

dgrizzanti mentioned this pull request Mar 11, 2021

[es] Add remote read clusters option for cross-cluster querying #2874

Merged

jpkrohling closed this Aug 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remote read clusters option to elasticsearch to enable cross-cluster querying #1892

Add remote read clusters option to elasticsearch to enable cross-cluster querying #1892

allen13 commented Nov 1, 2019 •

edited by pavolloffay

Loading

codecov bot commented Nov 1, 2019 •

edited

Loading

pavolloffay Nov 4, 2019

allen13 Nov 6, 2019

pavolloffay Nov 4, 2019

allen13 Nov 4, 2019

pavolloffay Nov 5, 2019

allen13 Nov 6, 2019

pavolloffay Nov 4, 2019

allen13 Nov 4, 2019

pavolloffay Nov 4, 2019 •

edited

Loading

allen13 Nov 4, 2019

pavolloffay Nov 5, 2019

allen13 Nov 6, 2019

pavolloffay Nov 4, 2019

allen13 Nov 6, 2019

pavolloffay commented Nov 4, 2019

allen13 commented Nov 4, 2019

yurishkuro Nov 11, 2019

yurishkuro Nov 11, 2019

yurishkuro Nov 11, 2019

yurishkuro Nov 11, 2019

dgrizzanti commented Mar 11, 2021

jpkrohling commented Aug 23, 2021

Add remote read clusters option to elasticsearch to enable cross-cluster querying #1892

Add remote read clusters option to elasticsearch to enable cross-cluster querying #1892

Conversation

allen13 commented Nov 1, 2019 • edited by pavolloffay Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Nov 1, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay Nov 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Nov 4, 2019

allen13 commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrizzanti commented Mar 11, 2021

jpkrohling commented Aug 23, 2021

allen13 commented Nov 1, 2019 •

edited by pavolloffay

Loading

codecov bot commented Nov 1, 2019 •

edited

Loading

pavolloffay Nov 4, 2019 •

edited

Loading