-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[storage] Remove dependency on archive flag in ES reader #6490
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
Changes from 22 commits
9d1d999
4f5cad5
af2e9ec
904d3fb
6bc1ebf
0ae679d
c0ea9a2
5659221
52f63fd
c573a66
4976f7b
77d47c0
6b97e7f
bdae29b
2bf4a61
6cf08e8
77ca3c3
686cc8d
b6db77a
20f5f5d
b785631
82d049d
cde60af
2a5d877
bbd6c1f
7ddb76b
c527bb0
f4bebe9
baa463b
3b1e0fb
865b6e3
f9bdd02
6688faa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -245,7 +245,7 @@ func TestArchiveDisabled(t *testing.T) { | |
func TestArchiveEnabled(t *testing.T) { | ||
f := NewFactory() | ||
f.primaryConfig = &escfg.Configuration{} | ||
f.archiveConfig = &escfg.Configuration{Enabled: true} | ||
f.archiveConfig = &escfg.Configuration{Enabled: true, UseReadWriteAliases: true} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this change necessary? It goes contrary to default config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro This was mostly for code coverage purposes. I can remove it but then we may need to merge with less than 100% code coverage or add a new test that goes through that code path There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my comment on the archive tests would address cove coverage There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro It didn't because that test doesn't go through |
||
f.newClientFn = (&mockClientBuilder{}).NewClient | ||
err := f.Initialize(metrics.NullFactory, zap.NewNop()) | ||
require.NoError(t, err) | ||
|
Original file line number | Diff line number | Diff line change | |||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -118,7 +118,7 @@ func withSpanReader(t *testing.T, fn func(r *spanReaderTest)) { | ||||||||||||||||
fn(r) | |||||||||||||||||
} | |||||||||||||||||
|
|||||||||||||||||
func withArchiveSpanReader(t *testing.T, readAlias bool, fn func(r *spanReaderTest)) { | |||||||||||||||||
func withArchiveSpanReader(t *testing.T, readAlias bool, readAliasSuffix string, fn func(r *spanReaderTest)) { | |||||||||||||||||
client := &mocks.Client{} | |||||||||||||||||
tracer, exp, closer := tracerProvider(t) | |||||||||||||||||
defer closer() | |||||||||||||||||
|
@@ -134,7 +134,7 @@ func withArchiveSpanReader(t *testing.T, readAlias bool, fn func(r *spanReaderTe | ||||||||||||||||
Tracer: tracer.Tracer("test"), | |||||||||||||||||
MaxSpanAge: 0, | |||||||||||||||||
TagDotReplacement: "@", | |||||||||||||||||
Archive: true, | |||||||||||||||||
ReadAliasSuffix: readAliasSuffix, | |||||||||||||||||
UseReadWriteAliases: readAlias, | |||||||||||||||||
}), | |||||||||||||||||
} | |||||||||||||||||
|
@@ -199,7 +199,6 @@ func TestSpanReaderIndices(t *testing.T) { | ||||||||||||||||
}{ | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
Archive: false, | |||||||||||||||||
SpanIndex: spanIndexOpts, | |||||||||||||||||
ServiceIndex: serviceIndexOpts, | |||||||||||||||||
}, | |||||||||||||||||
|
@@ -213,7 +212,12 @@ func TestSpanReaderIndices(t *testing.T) { | ||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
Archive: false, | |||||||||||||||||
ReadAliasSuffix: "archive", // ignored because ReadWriteAliases is false | |||||||||||||||||
}, | |||||||||||||||||
indices: []string{spanIndexBaseName, serviceIndexBaseName}, | |||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
SpanIndex: spanIndexOpts, | |||||||||||||||||
ServiceIndex: serviceIndexOpts, | |||||||||||||||||
IndexPrefix: "foo:", | |||||||||||||||||
|
@@ -228,27 +232,21 @@ func TestSpanReaderIndices(t *testing.T) { | ||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
Archive: true, | |||||||||||||||||
}, | |||||||||||||||||
indices: []string{spanIndexBaseName + archiveIndexSuffix, serviceIndexBaseName + archiveIndexSuffix}, | |||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
SpanIndex: spanIndexOpts, ServiceIndex: serviceIndexOpts, IndexPrefix: "foo:", Archive: true, | |||||||||||||||||
ReadAliasSuffix: "archive", | |||||||||||||||||
UseReadWriteAliases: true, | |||||||||||||||||
}, | |||||||||||||||||
indices: []string{"foo:" + config.IndexPrefixSeparator + spanIndexBaseName + archiveIndexSuffix, "foo:" + config.IndexPrefixSeparator + serviceIndexBaseName + archiveIndexSuffix}, | |||||||||||||||||
indices: []string{spanIndexBaseName + "archive", serviceIndexBaseName + "archive"}, | |||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
SpanIndex: spanIndexOpts, ServiceIndex: serviceIndexOpts, IndexPrefix: "foo:", Archive: true, UseReadWriteAliases: true, | |||||||||||||||||
SpanIndex: spanIndexOpts, ServiceIndex: serviceIndexOpts, IndexPrefix: "foo:", UseReadWriteAliases: true, ReadAliasSuffix: "archive", | |||||||||||||||||
}, | |||||||||||||||||
indices: []string{"foo:" + config.IndexPrefixSeparator + spanIndexBaseName + archiveReadIndexSuffix, "foo:" + config.IndexPrefixSeparator + serviceIndexBaseName + archiveReadIndexSuffix}, | |||||||||||||||||
indices: []string{"foo:" + config.IndexPrefixSeparator + spanIndexBaseName + "archive", "foo:" + config.IndexPrefixSeparator + serviceIndexBaseName + "archive"}, | |||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
SpanIndex: spanIndexOpts, | |||||||||||||||||
ServiceIndex: serviceIndexOpts, | |||||||||||||||||
Archive: false, | |||||||||||||||||
RemoteReadClusters: []string{"cluster_one", "cluster_two"}, | |||||||||||||||||
}, | |||||||||||||||||
indices: []string{ | |||||||||||||||||
|
@@ -262,20 +260,20 @@ func TestSpanReaderIndices(t *testing.T) { | ||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
Archive: true, RemoteReadClusters: []string{"cluster_one", "cluster_two"}, | |||||||||||||||||
UseReadWriteAliases: true, ReadAliasSuffix: "archive", RemoteReadClusters: []string{"cluster_one", "cluster_two"}, | |||||||||||||||||
}, | |||||||||||||||||
indices: []string{ | |||||||||||||||||
spanIndexBaseName + archiveIndexSuffix, | |||||||||||||||||
"cluster_one:" + spanIndexBaseName + archiveIndexSuffix, | |||||||||||||||||
"cluster_two:" + spanIndexBaseName + archiveIndexSuffix, | |||||||||||||||||
serviceIndexBaseName + archiveIndexSuffix, | |||||||||||||||||
"cluster_one:" + serviceIndexBaseName + archiveIndexSuffix, | |||||||||||||||||
"cluster_two:" + serviceIndexBaseName + archiveIndexSuffix, | |||||||||||||||||
spanIndexBaseName + "archive", | |||||||||||||||||
"cluster_one:" + spanIndexBaseName + "archive", | |||||||||||||||||
"cluster_two:" + spanIndexBaseName + "archive", | |||||||||||||||||
serviceIndexBaseName + "archive", | |||||||||||||||||
"cluster_one:" + serviceIndexBaseName + "archive", | |||||||||||||||||
"cluster_two:" + serviceIndexBaseName + "archive", | |||||||||||||||||
}, | |||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
Archive: false, UseReadWriteAliases: true, RemoteReadClusters: []string{"cluster_one", "cluster_two"}, | |||||||||||||||||
UseReadWriteAliases: true, RemoteReadClusters: []string{"cluster_one", "cluster_two"}, | |||||||||||||||||
}, | |||||||||||||||||
indices: []string{ | |||||||||||||||||
spanIndexBaseName + "read", | |||||||||||||||||
|
@@ -286,19 +284,6 @@ func TestSpanReaderIndices(t *testing.T) { | ||||||||||||||||
"cluster_two:" + serviceIndexBaseName + "read", | |||||||||||||||||
}, | |||||||||||||||||
}, | |||||||||||||||||
{ | |||||||||||||||||
params: SpanReaderParams{ | |||||||||||||||||
Archive: true, UseReadWriteAliases: true, RemoteReadClusters: []string{"cluster_one", "cluster_two"}, | |||||||||||||||||
}, | |||||||||||||||||
indices: []string{ | |||||||||||||||||
spanIndexBaseName + archiveReadIndexSuffix, | |||||||||||||||||
"cluster_one:" + spanIndexBaseName + archiveReadIndexSuffix, | |||||||||||||||||
"cluster_two:" + spanIndexBaseName + archiveReadIndexSuffix, | |||||||||||||||||
serviceIndexBaseName + archiveReadIndexSuffix, | |||||||||||||||||
"cluster_one:" + serviceIndexBaseName + archiveReadIndexSuffix, | |||||||||||||||||
"cluster_two:" + serviceIndexBaseName + archiveReadIndexSuffix, | |||||||||||||||||
}, | |||||||||||||||||
}, | |||||||||||||||||
} | |||||||||||||||||
for _, testCase := range testCases { | |||||||||||||||||
testCase.params.Client = clientFn | |||||||||||||||||
|
@@ -1267,7 +1252,7 @@ func TestSpanReader_GetEmptyIndex(t *testing.T) { | ||||||||||||||||
} | |||||||||||||||||
|
|||||||||||||||||
func TestSpanReader_ArchiveTraces(t *testing.T) { | |||||||||||||||||
withArchiveSpanReader(t, false, func(r *spanReaderTest) { | |||||||||||||||||
withArchiveSpanReader(t, true, "archive", func(r *spanReaderTest) { | |||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by passing the suffix explicitly (which no production code does today) you're overriding the default behavior. It's ok to do this as one of the tests, but we should keep the "default" tests where only Perhaps it makes sense to combine into a table driven test
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro Done. Can we remove the test below this now? |
|||||||||||||||||
mockSearchService(r). | |||||||||||||||||
Return(&elastic.SearchResult{}, nil) | |||||||||||||||||
mockArchiveMultiSearchService(r, "jaeger-span-archive"). | |||||||||||||||||
|
@@ -1283,7 +1268,7 @@ func TestSpanReader_ArchiveTraces(t *testing.T) { | ||||||||||||||||
} | |||||||||||||||||
|
|||||||||||||||||
func TestSpanReader_ArchiveTraces_ReadAlias(t *testing.T) { | |||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why remove this test? It seems it was counting on different index names, so useful to keep as backwards compatibility guard. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro Its because we don't concatenate the two inside the reader anymore. That logic is handled by the factory. If use_aliases is set and the read alias suffix is set, then we just use that suffix instead of concatenating the two. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we model that by altering There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro That would just be a difference of changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, the difference should be in the external configuration (i.e. calling create-archive with different use_aliases settings) to exercise the branching logic in both create-archive functions and the storage implementation, and yet ensure the same use of index names as before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro Brought the test back |
|||||||||||||||||
withArchiveSpanReader(t, true, func(r *spanReaderTest) { | |||||||||||||||||
withArchiveSpanReader(t, true, "archive-read", func(r *spanReaderTest) { | |||||||||||||||||
mockSearchService(r). | |||||||||||||||||
Return(&elastic.SearchResult{}, nil) | |||||||||||||||||
mockArchiveMultiSearchService(r, "jaeger-span-archive-read"). | |||||||||||||||||
|
@@ -1342,7 +1327,7 @@ func TestBuildTraceByIDQuery(t *testing.T) { | ||||||||||||||||
} | |||||||||||||||||
|
|||||||||||||||||
func TestTerminateAfterNotSet(t *testing.T) { | |||||||||||||||||
srcFn := getSourceFn(false, 99) | |||||||||||||||||
srcFn := getSourceFn(99) | |||||||||||||||||
searchSource := srcFn(elastic.NewMatchAllQuery(), 1) | |||||||||||||||||
sp, err := searchSource.Source() | |||||||||||||||||
require.NoError(t, err) | |||||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.