-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add support for ES index aliases / rollover to the dependency store (Resolves #2143) #2144
Conversation
a5ed9f9
to
c7eb8c8
Compare
You have cracked an issue that will require some more work :)
I would recommend submitting a separate PR for the first step initially. |
If I already cracked it and you thankfully already looked a little deeper into the other dependencies / breaking changes I take the liberty to go even another step further: As I suggested in #2143 (comment) how about simply switching to using / providing a CronJob using the ElasticSearch curator (including some basic config for it). It's also a Python based tool, but very flexible and advertised by Elastic themselves to actually be the one stop housekeeper - It can do rules with time and name filters and then apply all sorts of actions, including rollover and alias. It also has a dry-run mode for people to first check what will be done (https://github.com/elastic/curator/blob/master/docs/asciidoc/examples.asciidoc). I mean this is Jaeger tracing and likely you do not want to maintain and provide housekeeping tooling for all sorts of storages.
I shall get started on that shortly. |
See the response in #2143 (comment). If you manage to simplify those scripts we will be happy to accept the patch. |
@pavolloffay I pushed some more commits - thanks for your patience. |
As for the index templates - I just created another PR: #2149 |
#2149 looks good just remove there any other changes that are not related to creating index templates. |
Could you elaborate a bit please @pavolloffay . The PR #2149 is based on this one that is why those commits are in there as well. Should I reorder the changes of the two PRs then? Otherwise after merging this one, the other one should be rebased and then only contain the index template bits. |
#2149 should contain only refactoring to move the index mappings to json file and submitting those mappings as index templates at Jaeger startup. |
@pavolloffay I did rearrange the two PRs now. So first step is the removal of the mappings in #2149 and this PR here then follows with adding the support for index aliases and the Params struct to configure things. I noticed that there is not writer for dependencies initialized, that is why I kept the creation of templates with the reader. |
@pavolloffay with the merge of #2285 I now did a rebase of the PR here to allow using an rollover alias for the dependency store. FTAL |
1f314b1
to
cc2a52d
Compare
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.
Apart from this PR we will need changes to rollover and clean indices scripts.
Any chance of this getting updated to a state where is can be merged into master? |
@pavolloffay does it make sense to pick up on this then? |
Closing due to inactivity. |
@jpkrohling it's unfortunate that this, being such a rather small change, was not pursued. |
Thank you for asking! Sometimes, things fall off the cracks and this might have been one of them. Perhaps it would have helped to ping @pavolloffay more frequently (or even other maintainers). If you are still interested in contributing this feature, let me know and I'll reopen this one! |
I certainly would still like to finish this PR. Maybe going through open PRs from time to time so see what is keeping them from moving would be sensible and a good sign to contributors. Kinda like what you did, just before hitting the close button :-) As said, I gladly rebase the PR and would love to hear your feedback on what needs to be adjusted. |
I think it's fine to ping people from time to time. If it's a feature we don't want for some reason, we'll say so :-) Would you prefer to open a new PR, or should I reopen this one? @pavolloffay, would you be able to take another look at this PR and share what you think it's missing? |
if you don't mind please reopen this one. |
@pavolloffay @jpkrohling could you kindly take a look at this? |
+1 let's move this forward please. :) @frittentheke thanks for your work! |
@frittentheke / @wiardvanrij please add support for dependency index to rollover and index cleaner components. I see some changes in rollover but the
|
@frittentheke possible any update on this or require help? |
Sorry for the delay. I shall look into this soon and push an update. |
No problem at all! It's all on a volunteer after all :) Really appreciate your contributions as this feature will help us in a use case we have! |
7a0da46
to
c1be651
Compare
@vvdaal @wiardvanrij sorry about the huge delay ⌛
@pavolloffay @jpkrohling Could you kindly check out my PR again, as I don't actually see what is not working correctly.
is that not exactly what should happen? |
@frittentheke, the integration tests will need updating to include the newly added |
@frittentheke Thanks for the test details, the rollover results look good to me. I think we might need to support I've basically continued on from your test above. After executing the index cleaner, I expect all indices (
... and this is evident with:
|
4a51446
to
d11cddd
Compare
@albertteoh @pavolloffay @jpkrohling I pushed a new revision adding rollover to the es-index-cleaner and the missing tests. PTAL |
Thanks @frittentheke. It looks like some ES integration tests are failing. You can run these tests locally:
|
d11cddd
to
f3efb63
Compare
@albertteoh sorry for the delay. Tool be a minute more to figure that the optional ilm parts where missing in It's all green on my machine now - let's see what CI says then. |
The I think it may be related to the optional ILM part in By the way, the tests can be executed locally as well with: |
f3efb63
to
d3f037f
Compare
Yep, the fixtures needed fixing (additions). |
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.
LGTM, just a couple of nits.
reader := esDepStore.NewDependencyStore(f.primaryClient, f.logger, f.primaryConfig.GetIndexPrefix(), | ||
f.primaryConfig.GetIndexDateLayoutDependencies(), f.primaryConfig.GetMaxDocCount()) | ||
return reader, nil | ||
return createDependencyReader(f.logger, f.primaryClient, f.primaryConfig) |
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.
nit: I think we can just return esDepStore.NewDependencyStore(...)
.
I can see the intention here is to maintain consistency, though I think CreateSpanReader/Writer
extract the function for reuse with its Archive
equivalent. The DependencyStore
doesn't support archiving AFAIK, so there doesn't seem to be much benefit of extracting the function for the cost of the additional indirection, unless you had other reasons?
What do you think?
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.
While I understand your point and while I could argue there might arise the need to add archiving, I actually just would favor the consistency. As for performance, the compiler will optimize the additional call away and it's not like this is a hot path by any means.
In short, if you insist I will certainly change this, but I believe the code is just more approachable if similar things, as in creating readers for three types of stores, are not handled any differently.
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.
We can leave this as is, I'm not strongly attached to my suggestion. 😄
* Give DependencyStore a params struct like the SpanStore to carry its configuration parameters * Adapt and extend the tests accordingly Signed-off-by: Christian Rohmann <[email protected]>
…es indices Signed-off-by: Christian Rohmann <[email protected]>
d84b0bc
to
a3cb643
Compare
reader := esDepStore.NewDependencyStore(f.primaryClient, f.logger, f.primaryConfig.GetIndexPrefix(), | ||
f.primaryConfig.GetIndexDateLayoutDependencies(), f.primaryConfig.GetMaxDocCount()) | ||
return reader, nil | ||
return createDependencyReader(f.logger, f.primaryClient, f.primaryConfig) |
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.
We can leave this as is, I'm not strongly attached to my suggestion. 😄
Which problem is this PR solving?
Currently the dependency store has no support for index aliases / rollover indices.
Resolves #2143
Short description of the changes
Use the already existing and used config variable
UseReadWriteAliases
to switch between the current behavior and using non-dated "-read" and "-write" index names. This aligns with the behavior that is already in place for spans and services.