Skip to content

TSDB: fix the time_series in order collect priority #85526

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 12, 2022
Merged

TSDB: fix the time_series in order collect priority #85526

merged 5 commits into from
Apr 12, 2022

Conversation

weizijun
Copy link
Contributor

Time series index is sort by timestamp desc. So the collect order will be order by timestamp desc.
When _tsid are the same, the bigger timestamp segment will be pop.

@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.3.0 labels Mar 31, 2022
@romseygeek romseygeek added the :StorageEngine/TSDB You know, for Metrics label Mar 31, 2022
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 31, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@romseygeek
Copy link
Contributor

Good spot. I think we're still experimenting with the order in which things are sorted, so maybe the best thing to do here is to read the index sort from the index reader when we build the searcher and determine whether we're sorting by asc or desc timestamp then?

@weizijun
Copy link
Contributor Author

Good spot. I think we're still experimenting with the order in which things are sorted, so maybe the best thing to do here is to read the index sort from the index reader when we build the searcher and determine whether we're sorting by asc or desc timestamp then?

Yeah, @imotov can look at the demand? The index sort config of time_series index is set internally, is it need to read the index sort config for TimeSeriesIndexSearcher?

@romseygeek
Copy link
Contributor

is it need to read the index sort config for TimeSeriesIndexSearcher?

Yes, you can read it from the IndexReader's metadata.

@weizijun
Copy link
Contributor Author

Yes, you can read it from the IndexReader's metadata.

I can't find sort metadata from IndexReader, but I find the index sort in the SearchContext, which can be passed to the TimeSeriesIndexSearcher constructor.

@weizijun
Copy link
Contributor Author

weizijun commented Apr 1, 2022

Good spot. I think we're still experimenting with the order in which things are sorted, so maybe the best thing to do here is to read the index sort from the index reader when we build the searcher and determine whether we're sorting by asc or desc timestamp then?

@romseygeek I set the time_series sort to a static value, and TimeSeriesIndexSearcher read the value to decide the right order.

@weizijun
Copy link
Contributor Author

weizijun commented Apr 6, 2022

@elasticmachine update branch

@csoulios csoulios mentioned this pull request Apr 11, 2022
Merged
@csoulios csoulios self-assigned this Apr 11, 2022
@csoulios
Copy link
Contributor

@elasticmachine test this please

csoulios
csoulios reviewed Apr 11, 2022
View reviewed changes
Copy link
Contributor

@csoulios csoulios left a comment

Choose a reason for hiding this comment

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

@weizijun can you please add the changelog file?

@weizijun
Copy link
Contributor Author

@weizijun can you please add the changelog file?

done

@weizijun
Copy link
Contributor Author

@elasticmachine update branch

@csoulios
Copy link
Contributor

@elasticmachine test this please

@weizijun weizijun mentioned this pull request Apr 12, 2022
Closed
csoulios
csoulios approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@csoulios csoulios left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@csoulios csoulios merged commit 2548d17 into elastic:master Apr 12, 2022
@weizijun
Copy link
Contributor Author

LGTM. Thank you!

Thanks @csoulios !

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Apr 13, 2022
@weizijun
Merge branch 'upstream/master' into fix-time_series-tisdord-exception
5dea773 
* upstream/master: (40 commits)
  Fix BuildTests serialization (elastic#85827)
  Use urgent priority for node shutdown cluster state update (elastic#85838)
  Remove Task classes from HLRC (elastic#85835)
  Remove unused migration classes (elastic#85834)
  Remove uses of Charset name parsing (elastic#85795)
  Remove legacy versioned logic for DefaultSystemMemoryInfo (elastic#85761)
  Expose proxy settings for GCS repositories (elastic#85785)
  Remove SLM classes from HLRC (elastic#85825)
  TSDB: fix the time_series in order collect priority (elastic#85526)
  Remove ILM classes from HLRC (elastic#85822)
  FastVectorHighlighter should use ValueFetchers to load source data (elastic#85815)
  Iteratively execute synchronous ingest processors (elastic#84250)
  Remove TransformClient from HLRC  (elastic#85787)
  Mute XPackRestIT deprecation/10_basic/Test Deprecations (elastic#85807)
  Unmute Lintian packaging test (elastic#85778)
  Add a highlighter unit test base class (elastic#85719)
  Remove NIO Transport Plugin (elastic#82085)
  [TEST] Remove token methods from HLRC SecurityClient (elastic#85515)
  [Test] Use thread-safe hashSet for result collection (elastic#85653)
  [TEST] Mute BuildTests.testSerialization (elastic#85801)
  ...

# Conflicts:
#	server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csoulios csoulios csoulios approved these changes

Assignees

@csoulios csoulios

Labels
>bug external-contributor Pull request authored by a developer outside the Elasticsearch team :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@weizijun @elasticmachine @romseygeek @csoulios @elasticsearchmachine