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.
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
[storage] Remove dependency on archive flag in ES reader #6490
[storage] Remove dependency on archive flag in ES reader #6490
Changes from 8 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
There are no files selected for viewing
Check warning on line 224 in plugin/storage/es/factory.go
Codecov / codecov/patch
plugin/storage/es/factory.go#L223-L224
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.
@yurishkuro Not ideal to have to do this but this is roughly what we need to maintain backwards compatibility.
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'm not sure if we want to expose a configuration option to override the default
read
andwrite
alias but that could be one way to streamline this.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.
-read
suffix?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.
Its added by the reader and the writer but we need to handle the special case where the archive storage also has use_aliases set to true
Yeah it does - its done by the reader and the writer. I made some changes to make it an else-if change. The current behavior is that if a suffix is passed in, we use that. If a suffix isn't passed in but use_aliases is set, then we use the default read/write aliases.
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.
that's why I don't follow - if reader/writer automatically add
-read/-write
suffix, why do you need to add it manually above?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 add it above just to address the case where
use_aliases
wasn't initially set for the archive storage. Since we hardcode it to true for the archive storage now, we'll always set the index to bearchive-read
andarchive-write
in the reader/writer. But that would break backwards compatibility because previously it would just bearchive
for both the read and write alias for the archive storage.Check warning on line 240 in plugin/storage/es/factory.go
Codecov / codecov/patch
plugin/storage/es/factory.go#L239-L240