Skip to content
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

Index template aliases #2739

Closed
wants to merge 1 commit into from
Closed

Conversation

jbrook
Copy link
Contributor

@jbrook jbrook commented Mar 7, 2013

Added support for aliases in index templates.

@jbrook jbrook closed this Mar 7, 2013
@jbrook jbrook deleted the Index-template-aliases branch March 7, 2013 00:54
@jbrook jbrook restored the Index-template-aliases branch March 7, 2013 00:56
@jbrook jbrook reopened this Mar 7, 2013
@dav3860
Copy link

dav3860 commented Apr 3, 2013

That would be great to automatically create specific views/authorizations on index creation.

@jbrook
Copy link
Contributor Author

jbrook commented Apr 3, 2013

It's really handy. I hope someone will merge it one day soon.

@martijnvg
Copy link
Member

@jbrook Looks nice! I'll try to get it in soon!

@dav3860
Copy link

dav3860 commented Apr 4, 2013

jbrook, how do you write the template if you want to add an alias with a term filter ?

@jbrook
Copy link
Contributor Author

jbrook commented Apr 4, 2013

@dav3860 we use something like this:

"aliases": {
      "my_alias_name": {
        "filter" : { "term" : { "@myAttrName" : "someTerm" } }
      },
      "my_alias_name_2": {
        "filter" : { "term" : { "@myAttrName" : "anotherTerm" } }
      }
    }

@dav3860
Copy link

dav3860 commented Apr 4, 2013

@jbrook thank you. I hope it gets merged.

@jbrook
Copy link
Contributor Author

jbrook commented May 28, 2013

@martijnvg , I tested this pull request today with 'master' and '0.90' in my local environment and it still merges without conflicts and tests OK. I would love to have your feedback or see it merged. I think it is very useful functionality to have when using logstash. I am sure it may also be useful to @rashidkpc in his work on Kibana 3 - custom dashboards could be set up to work only with a specific index alias or aliases instead of always having to specify the date pattern. Anyway, enough of the hard sell - I know you are all really busy with getting to 1.0!

@dav3860
Copy link

dav3860 commented May 28, 2013

@jbrook You mention Kibana 3. One of its killer features is that you can set a time stamped indice pattern. Then all the time-based queries are done sequentially to optimize the user experience.
If you automate the creation of the alias, it should create time stamped aliases too, eg : alias1-05.05.2013 -> logstash-05.05.2013, so as to keep the benefit of the sequential queries. We should avoid hiding all the indices behind the same alias. Does your pull request allow to use some kind of regexp to name the alias in the template ?

@jbrook
Copy link
Contributor Author

jbrook commented Jun 4, 2013

@dav3860 - Is that really a killer feature? Elasticsearch handles concurrent queries very efficiently. I guess the sequential querying is an optimisation in the client so that the histogram, etc build up gradually. Couldn't the same effect be achieved with date filters?

I think it's a good feature suggestion but I would like to have the feeling that the original pull request will be pulled before doing more work on it.

@kimchy
Copy link
Member

kimchy commented Sep 4, 2013

I think having the actual index name be allowed to be used inside the aliases make sense, something like ${index} where it will be replaced with the actual index name.

@ghost ghost assigned javanna Sep 5, 2013
@jbrook
Copy link
Contributor Author

jbrook commented Sep 5, 2013

I implemented the suggestion from @kimchy. I played with allowing the ${index} for the creation of any index but I don't think it makes sense because when using the alias APIs directly, the client doesn't need this feature and I think it makes sense to keep this functionality close to where the other index template code is. The result is that this only works for aliases created by index templates. I hope that makes sense.

@kimchy
Copy link
Member

kimchy commented Sep 5, 2013

looks good, can we squash the changes into a single commit? would be simpler to review it then. Quick note, I don't see the aliases being properly serialized (readFrom/writeTo in IndexTemplateMetaData), also just double checking its on both from and to xcontent methods.

The serialization part, it would be nice to have it versioned, so we backport it to 0.90 potentially (see CommonStats with how it works with completion service).

@jbrook
Copy link
Contributor Author

jbrook commented Sep 6, 2013

I fixed the various serialization bits, added some tests for that and squashed the commits. I haven't added the versioning conditions. Which version constant should I use with the onOrAfter check?

@dav3860
Copy link

dav3860 commented Sep 9, 2013

Cool. It will help adding authorization to ES, and Kibana for example. We could let ES generate specific views each time it creates an index.

@jbrook
Copy link
Contributor Author

jbrook commented Sep 10, 2013

@dav3860 - yes. I see how useful your suggestion was now. I had been looking for a way to do auth like this with a proxy we have written. Unfortunately there is an alias bug that is causing problems for me - #2762 - Alias filters cached/type inferenced incorrectly.

@jbrook
Copy link
Contributor Author

jbrook commented Sep 12, 2013

Issue #3677 is also problematic. Aliases have to be specifically listed as a comma separated list rather than using a wild-card if using the multi-index syntax, otherwise the filters are not applied.

@kimchy
Copy link
Member

kimchy commented Sep 16, 2013

@jbrook if we use version based serialization, then we can backport it to 0.90, in which case, use the latest 0.90 release available. At this time its 0.90.4, but 0.90.5 will probably come out soon due to the site plugin installation problem. Need another review cycle on my end, @martijnvg can you look at it as well?

@jbrook
Copy link
Contributor Author

jbrook commented Sep 27, 2013

Updated and rebased again to support randomized testing etc and bring the request up-to-date with development on master.

@dav3860
Copy link

dav3860 commented Oct 11, 2013

Do you plan to merge it into the next release ?

@ghost ghost assigned javanna Oct 14, 2013
@javanna
Copy link
Member

javanna commented Oct 23, 2013

I'm going to start reviewing it, so that it hopefully makes it to the next release. Will leave inline comments on the code if needed.

}
}

super.readFrom(in);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you set your IDE to never use tabs, always 4 spaces instead?

@@ -207,6 +209,15 @@ public ClusterState execute(ClusterState currentState) throws Exception {
customs.put(type, merged);
}
}
// handle aliases
for (Map.Entry<String, AliasMetaData> aliasEntry : template.aliases().entrySet()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I got what you meant to do here. Is this code just to avoid duplicates, like aliases with same name (which would be handled automatically since you are using a Map? If so I think we should handle this issue in the beginning, when creating the index template. The index templates already registered should be good right?

@javanna
Copy link
Member

javanna commented Oct 23, 2013

Thanks a lot for your work on this @jbrook !!! My review is completed, can you make the suggested changes?
We should include this in the documentation too, which is now included in this same repository. Can you look into that?
Feel free to ping me if you have any question!

@jbrook
Copy link
Contributor Author

jbrook commented Oct 30, 2013

Thanks a lot for the detailed review @javanna My time is rather limited at the moment. I will do my best to look at this soon. I deliberately left the version numbers out of the serialization/de-serialization for the moment because I didn't know which version to use. Perhaps you can take care of that part once I have made the other changes?

@javanna
Copy link
Member

javanna commented Oct 30, 2013

Hey @jbrook take your time and let me know if you need help! Don't worry too much about the serialization for now, we'll fix that when the rest is ready.

@javanna
Copy link
Member

javanna commented Nov 14, 2013

Hi @jbrook did you have the chance to work on this? If your time is still limited I can have a look and apply some changes myself, if you don't mind.

@jbrook
Copy link
Contributor Author

jbrook commented Nov 14, 2013

I've done most of it. Still need to dig in to duplicate aliases, invalid aliases and tests. Hope to find time soon. Could commit what I have - new baby just joined the family :)

Sent from my iPhone

On 14 Nov 2013, at 12:35, Luca Cavanna notifications@github.com wrote:

Hi @jbrook did you have the chance to work on this? If your time is still limited I can have a look and apply some changes myself, if you don't mind.


Reply to this email directly or view it on GitHub.

@javanna
Copy link
Member

javanna commented Nov 14, 2013

Congrats! Thanks for your work, if you don't mind pushing what you have I can take it from there and get this merged soon.

Support {index} token in index template aliases - When updating index creation metadata from a template, check the alias name for an {index} token and replace it with the name of the index to which it is being applied. This makes it easy to apply rolling dated aliases in line with schemes like that used by logstash, e.g. allowing for an alias called myalias-logstash-2013.09.05 where the alias string is "myalias-{index}" and the index name is logstash-2013.09.05.

Not all items in code review are addressed. See comments with 'FIXME: jbrook'
@dav3860
Copy link

dav3860 commented Jan 11, 2014

Thank you @jbrook for the work on this feature. @javanna, do you think it can be merged soon ? We've been waiting for this feature to configure authorization profiles for Kibana 3 !

@javanna
Copy link
Member

javanna commented Jan 28, 2014

We decided to split this task into two sub-tasks:

  • add support for aliases in create index api
  • add support for aliases in index templates, which will make use of the infrastructure added in step 1

We've been working on step 1, have a look at #4920. After that we'll look into merging this PR.

@kakbit
Copy link

kakbit commented Feb 8, 2014

Still waiting...

@javanna
Copy link
Member

javanna commented Feb 19, 2014

Thanks again @jbrook for your work, it took a while but we almost made it ;)

The support for aliases in create index (#4920) got pushed to master and 1.x branch.

I just created a new PR where I pulled your commit, updated it and finalized your work so that we can get this merged. Have a look at #5180 if you are interested. Closing this one as it got replaced by #5180.

@javanna javanna closed this Feb 19, 2014
@jbrook
Copy link
Contributor Author

jbrook commented Feb 19, 2014

That’s great to see! Thanks for looking at it. I actually spent a day last week trying to finish it off but had to abandon to work on other things.

Congratulations on delivering the 1.0 release.

On 19 Feb 2014, at 17:10, Luca Cavanna notifications@github.com wrote:

Thanks again @jbrook for your work, it took a while but we almost made it ;)

The support for aliases in create index (#4920) got pushed to master and 1.x branch.

I just created a new PR where I pulled your commit, updated it and finalized your work so that we can get this merged. Have a look at #5180 if you are interested. Closing this one as it got replaced by #5180.


Reply to this email directly or view it on GitHub.

javanna pushed a commit to javanna/elasticsearch that referenced this pull request Mar 6, 2014
Adapted existing PR (elastic#2739) to updated code (post elastic#4920), added tests and docs (@javanna)

Closes elastic#1825
javanna pushed a commit that referenced this pull request Mar 6, 2014
Adapted existing PR (#2739) to updated code (post #4920), added tests and docs (@javanna)

Closes #1825
javanna pushed a commit that referenced this pull request Mar 6, 2014
Adapted existing PR (#2739) to updated code (post #4920), added tests and docs (@javanna)

Closes #1825
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants