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

Deprecate : in cluster and index names #23892

Closed
clintongormley opened this issue Apr 4, 2017 · 19 comments
Closed

Deprecate : in cluster and index names #23892

clintongormley opened this issue Apr 4, 2017 · 19 comments
Assignees
Labels
:Core/Infra/Settings Settings infrastructure and APIs :Data Management/Indices APIs APIs to create and manage indices and templates >deprecation discuss good first issue low hanging fruit

Comments

@clintongormley
Copy link
Contributor

We use : in cross cluster search to separate the cluster name from the index name, eg cluster:index. This can introduce ambiguity when you have an index named cluster:index (although this can be worked around by adding a cluster prefix: my_cluster:cluster:index.

Still, to avoid ambiguity, we should deprecate : in cluster and index names.

@clintongormley clintongormley added :Data Management/Indices APIs APIs to create and manage indices and templates :Core/Infra/Settings Settings infrastructure and APIs help wanted adoptme >deprecation good first issue low hanging fruit labels Apr 4, 2017
@glefloch
Copy link
Contributor

glefloch commented Apr 4, 2017

I'm interested into working on this issue.
By deprecating : in cluster and index names, do you mean logging something at startup (or at index creation time) if the name contains :, or should we throw an exception to prevent adding it?

@jasontedor
Copy link
Member

To be clear, the ambiguity is handled in the following way already today. If you try to search on cluster:index, there is a remote cluster aliased by cluster, and there is an index named cluster:index, then we fail the request and inform the user that they need to change the remote cluster alias to resolve the ambiguity.

@javanna
Copy link
Member

javanna commented Apr 5, 2017

good point @jasontedor . I also wonder if we need to add any validation to the cluster name like in #23911 or rather in cluster aliases used for cross cluster search, that don't necessarily have to match with the remote cluster names.

@javanna
Copy link
Member

javanna commented Apr 5, 2017

the other bit that comes to mind is that the same validation should be applied to both index names and aliases, something that we happened to forget about in the past.

@javanna javanna marked this as a duplicate of #25924 Jul 27, 2017
dakrone added a commit to dakrone/elasticsearch that referenced this issue Aug 16, 2017
We use `:` for cross-cluster search (eg `cluster:index`), therefore, we should
not allow the ambiguity when allowing cluster or index names.

Relates to elastic#23892
@dakrone dakrone removed the help wanted adoptme label Aug 16, 2017
@dakrone dakrone self-assigned this Aug 16, 2017
dakrone added a commit that referenced this issue Aug 17, 2017
We use `:` for cross-cluster search (eg `cluster:index`), therefore, we should
not allow the ambiguity when allowing cluster or index names.

Relates to #23892
dakrone added a commit that referenced this issue Aug 17, 2017
We use `:` for cross-cluster search (eg `cluster:index`), therefore, we should
not allow the ambiguity when allowing cluster or index names.

Relates to #23892
dakrone added a commit that referenced this issue Aug 17, 2017
We use `:` for cross-cluster search (eg `cluster:index`), therefore, we should
not allow the ambiguity when allowing cluster or index names.

Relates to #23892
@dakrone
Copy link
Member

dakrone commented Aug 17, 2017

This is now deprecated in 6.0+ and forbidden in 7.0+

@codefromthecrypt
Copy link

This is likely to cause a lot of ecosystem damage that will be blamed on call sites such as other projects, not this discussion between 5 people. Please reconsider and/or create a plan for all ecosystem effects including your whole stack. How folks will actually deprecate this in practice and keep their searches, cleanup jobs etc working.

See also openzipkin/zipkin#2219

@jasontedor
Copy link
Member

I understand your concern @adriancole; being broken by upstream changes is frustrating for any engineer.

I think we have taken a conservative approach here that I hope addresses your concerns.

We have deprecated this in 6.0.0 which was released nearly a year ago, and will remove support to create new indices in 7.0.0 (a future release whose timeframe we can not indicate right now). That means this carries a long deprecation period over which users and downstream developers can adjust.

And 7.0.0 will still support these existing indices, it's only that new ones will not be able to be created.

It's only in 8.0.0 that support for these indices will be dropped, and that's in accordance with our support policy anyway. Elasticsearch major versions support older indices only from the previous major version.

All this together means it's a long period from the initial deprecation to complete removal.

If you have specific feedback on what we can do better here, we are eager to receive it.

@codefromthecrypt
Copy link

Thanks for writing back. Seems the decision is final and that you are ok as I suppose there was no other means possible? I still would like to be able to link to some document that says what practice people should do to migrate off this. This is work causing and frankly ES has way more employees than we have volunteers to do this sort of migration guide.

Ask 1: Can you (ES) please write a public doc on this with concrete suggestions on how to mitigate this, linking to the discussion that caused this?

Next topic is that the message isn't precise, it says there's no support in 7.0

[2018-10-24T11:32:00,033][WARN ][o.e.d.c.m.MetaDataCreateIndexService] index or alias name [zipkin:span-2018-10-24] containing ':' is deprecated and will not be supported in Elasticsearch 7.0+

Please change that message? Maybe to say

"existing indexes containing ':' are deprecated. Elasticsearch 7 will be able to read them, but not create new ones. Elasticsearch 8+ will drop support entirely"

Ask 2: Can you please make a more precise deprecation message?

@codefromthecrypt
Copy link

One thing very important is that we reviewed our index pattern after the last time we were bit causing a huge work ripple through the ecosystem. In the "mitigation doc" can you please permanently whitelist characters you will not remove later?

@jasontedor
Copy link
Member

Thank you for the feedback.

Seems the decision is final and that you are ok as I suppose there was no other means possible?

Well, we try to maintain a philosophy that every decision can be revisited. With compelling justification, any decision can be iterated on, and ever reversed. There is a lot to overcome here though. 😇

Can you (ES) please write a public doc on this with concrete suggestions on how to mitigate this, linking to the discussion that caused this?

Can you elaborate on what you mean by "to mitigate this"? I want to make sure I understand the request before acting on it.

Can you please make a more precise deprecation message?

I agree that the existing deprecation message is misleading. We will address this. Can you pick this up @dakrone?

@codefromthecrypt
Copy link

I'll write separate replies :) thanks for keeping an open mind.

Well, we try to maintain a philosophy that every decision can be revisited. With compelling justification, any decision can be iterated on, and ever reversed. There is a lot to overcome here though. 😇

So, the exposure here is folks with index patterns such as ':' While I don't know the details here, this seems a query related concern, even mentioning a workaround for the ambiguity. In a green field the best way to remove ambiguity is to eliminate the problem (by reducing things like the character set). This is muddy in the brown field (see what I did just there?)

The justification would then be based on brown fields. Frankly data policy deprecation is hard to even notice. There's no code deprecation! We only found out by accident today. If this happened since 6.0 that must say something (maybe about us!). So I don't know all of the sites of impact, but at least in zipkin we already have several branches of code to read different policy of elasticsearch. This constraint against colons had 2 thumbs up on this issue.. that seems a low justification to overcome :) you tell me? your knowledge of ES ecosystem, how many different types of tech will result in a glitch unless someone changes their index pattern (during migration and after)? Maybe others can speak to how much impact this is?

@codefromthecrypt
Copy link

Can you elaborate on what you mean by "to mitigate this"? I want to make sure I understand the request before acting on it.

Concretely in zipkin we still have to read old indexes.. people will have data at least in dependencies. That or we have to hand over to them a list of chores. For example, change this in curator cleanup jobs, proxy or alias so that there's seamless query when looking back against daily buckets where the index pattern changes. Think about your spark stuff, someone needs to scan against different patterns now.. maybe knowing about regex or wildcards or other mechanisms to address the change from ":" in one day to... errr I don't even want to guess the next character, so you tell me :P

@codefromthecrypt
Copy link

codefromthecrypt commented Oct 25, 2018

I agree that the existing deprecation message is misleading. We will address this

yay! also not sure how but we should think about why at least in our community no-one noticed until today. Is there another way to indicate deprecation? This is probably a separate topic.

@codefromthecrypt
Copy link

codefromthecrypt commented Oct 25, 2018

One last anecdote.. the only reason we have colons was due to dropping of multi-type indexes earlier. This was a result of an earlier very expensive chore, we literally had to write 2 query engines last time! It is a pain because we thought we reviewed the approach carefully to avoid this again.

This is less impactful, as presumably there are still characters permitted. We need to know what they are, and in our case we will change zipkin:span-2017-08-11 to a different delimter like zipkin💩span-2017-08-11. The rippling stuff seems mostly on read side.. but we would have to make a loud release telling people in advance as especially in ES people use different tools to ingest data, not just ours. they also have layered index templates which might miss etc.

Hope the context helps

@jasontedor
Copy link
Member

It is getting late for me and I want to go to bed so I will reply to the easier one to reply to of your comments, and save the others for another time.

yay! also not sure how but we should think about why at least in our community no-one noticed until today. Is there another way to indicate deprecation? This is probably a separate topic.

This is indeed a challenging problem. I will offer one suggestion here and see what you think of it.

Elasticsearch generates warning headers on HTTP requests when deprecated functionality is used. I encourage all developers building on top of Elasticsearch to fail builds when a client call in a test returns any warning header. Then you can make an active choice:

  • make the necessary code changes to make the warning header go away
  • suppress that warning header from failing the build and open an issue (sometimes you need to do this because getting the build passing is important, and then you'll follow up later)

Our Kibana team will be implementing something like this, so that they too become aware during a development cycle whether or not they are relying on deprecated functionality.

@codefromthecrypt
Copy link

great tip on the warning header! I will incorporate this into our integration tests, as we can safely crash requests there. We can resume the other stuff when fresh. Gnight!

@dakrone
Copy link
Member

dakrone commented Oct 25, 2018

I agree that the existing deprecation message is misleading. We will address this. Can you pick this up @dakrone?

Certainly, I'll open a PR for it shortly

dakrone added a commit to dakrone/elasticsearch that referenced this issue Oct 25, 2018
The message was a little unclear in that we will still support indices that have
a ':' in the name in 7.0, we will only prevent index creation containing a ':'

Relates to elastic#23892 (comment)
@dakrone
Copy link
Member

dakrone commented Oct 25, 2018

I opened #34867 for a better deprecation message.

@codefromthecrypt
Copy link

I know it was late during this discussion, but curious if folks are able to reconsider deprecating this with some time bought between now and version 8. Maybe there's a creative way to address the problem, possibly by looking at the exposure (which seems to be the query api)? This request might have gotten lost in comments about the deprecation message.

Failing that, a practical response to give to folks as still we'll need to prepare for the storm. For example, like many we have communities who want the oldest (for us oldest is 2.x) and others who want the newest. We'll ideally be able to bundle in some sort of carrot with the stick, so we'll look for some feature that helps us and package the message about the index refactoring etc with that ;P tips welcome.

dakrone added a commit that referenced this issue Nov 1, 2018
The message was a little unclear in that we will still support indices that have
a ':' in the name in 7.0, we will only prevent index creation containing a ':'

Relates to #23892 (comment)
dakrone added a commit that referenced this issue Nov 1, 2018
The message was a little unclear in that we will still support indices that have
a ':' in the name in 7.0, we will only prevent index creation containing a ':'

Relates to #23892 (comment)
dakrone added a commit that referenced this issue Nov 1, 2018
The message was a little unclear in that we will still support indices that have
a ':' in the name in 7.0, we will only prevent index creation containing a ':'

Relates to #23892 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Settings Settings infrastructure and APIs :Data Management/Indices APIs APIs to create and manage indices and templates >deprecation discuss good first issue low hanging fruit
Projects
None yet
Development

No branches or pull requests

6 participants