Skip to content

TSDB: fix the time_series in order collect priority#85526

Merged
csoulios merged 5 commits intoelastic:masterfrom
weizijun:fix-time_series-collect
Apr 12, 2022
Merged

TSDB: fix the time_series in order collect priority#85526
csoulios merged 5 commits intoelastic:masterfrom
weizijun:fix-time_series-collect

Conversation

@weizijun
Copy link
Copy Markdown
Contributor

@weizijun weizijun commented Mar 31, 2022

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
Copy Markdown
Collaborator

elasticmachine commented Mar 31, 2022

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

@romseygeek
Copy link
Copy Markdown
Contributor

romseygeek commented Mar 31, 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?

@weizijun
Copy link
Copy Markdown
Contributor Author

weizijun commented Mar 31, 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?

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
Copy Markdown
Contributor

romseygeek commented Mar 31, 2022

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

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

@weizijun
Copy link
Copy Markdown
Contributor Author

weizijun commented Mar 31, 2022

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

csoulios commented Apr 11, 2022

@elasticmachine test this please

csoulios
csoulios reviewed Apr 11, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

weizijun commented Apr 12, 2022

@weizijun can you please add the changelog file?

done

@weizijun
Copy link
Copy Markdown
Contributor Author

weizijun commented Apr 12, 2022

@elasticmachine update branch

@csoulios
Copy link
Copy Markdown
Contributor

csoulios commented Apr 12, 2022

@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
Copy Markdown
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
Copy Markdown
Contributor Author

weizijun commented Apr 13, 2022

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

1 more reviewer

@csoulios csoulios csoulios approved these changes

Reviewers whose approvals may not affect merge requirements

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