Skip to content

Conversation

felixbarny
Copy link
Member

This avoids that the tsid and routing is calculated differently on the same index. Doing so would have the following consequences:

  • De-duplication would not work, for example, when a client re-transmits a batch as the id (which is based on the tsid) will be different.
  • When replaying a translog operation, the re-computed tsid and id will differ from the id stored in the translog. This would lead to a failure of the index operation.

This could be seen as a breaking change as operations on backing indices that were allowed before are not allowed after this change. However, this only applies to data streams as index.dimensions will never be set on plain time_series indices. For data streams, you first change the template, then optionally the backing indices, and roll over if changing the backing indices doesn't work.

This avoids that the tsid and routing is calculated differently on the same index.
Doing so would have the following consequences:
* De-duplication would not work, for example, when a client re-transmits a batch as the id (which is based on the tsid) will be different.
* When replaying a translog operation, the re-computed tsid and id will differ  from the id stored in the translog. This would lead to a failure of the index operation.
@felixbarny felixbarny self-assigned this Sep 26, 2025
@felixbarny felixbarny added the :StorageEngine/TSDB You know, for Metrics label Sep 26, 2025
@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v9.2.0 labels Sep 26, 2025
// This can happen if a new dynamic template with time_series_dimension: true is added to an existing index.
additionalSettings.putList(INDEX_DIMENSIONS.getKey(), List.of());
additionalSettings.putList(INDEX_ROUTING_PATH.getKey(), newIndexDimensions);
throw new IllegalArgumentException(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also update the comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I deleted the comments above and thought that the exception message would serve as the new documentation. What aspects would you like the comment to cover that the exception message doesn't already mention?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to the method javadoc, providing an overview of how this works. It'd help readability to include a note there too, imho.

@kkrik-es
Copy link
Contributor

I kinda like this. Let's try to be as restrictive as possible.

@felixbarny
Copy link
Member Author

I like it better, too but didn't make the change earlier due to potential impact on backwards compatibility. Are you worried about that at all?

@kkrik-es
Copy link
Contributor

Not really.. If anything, I'd think it's a best practice to avoid such changes (label => dimension) outside rollovers. More so, we should only have dimensions and metrics at this point, no labels. Maybe worth documenting better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor Pull request authored by a developer outside the Elasticsearch team :StorageEngine/TSDB You know, for Metrics v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants