Revert "Enable skip_list for @timestamp field or index sort field #19661
Conversation
asimmahmood1
force-pushed
the
revert_timestamp_skip_list
branch
from
35ef4da to
2976e0c
Compare
asimmahmood1
requested review from
a team,
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
github-project-automation
bot
moved this from In-Review
to In Progress
in Performance Roadmap
asimmahmood1
force-pushed
the
revert_timestamp_skip_list
branch
2 times, most recently
from
da8b0f0 to
b3453b9
Compare
…ensearch-project#19480)" This reverts commit 08ed9ee. Fixes opensearch-project#19660 Signed-off-by: Asim Mahmood <asim.seng@gmail.com>
asimmahmood1
force-pushed
the
revert_timestamp_skip_list
branch
from
b3453b9 to
f7b1ee0
Compare
| } | ||
| if (hasDocValues) { | ||
| if (skiplist || isSkiplistDefaultEnabled(context.indexSettings().getIndexSortConfig(), fieldType().name())) { | ||
| if (skiplist) { |
There was a problem hiding this comment.
Will this result in same error if someone enables skiplist setting on a cluster having indices created with <= 2.19 version?
There was a problem hiding this comment.
I am wondering if we can leverage the dateFieldMapper.indexCreatedVersion property to validate the index version?
There was a problem hiding this comment.
No, skip_list mapping is on creation only, so OS will reject the request to update the mapping.
There was a problem hiding this comment.
Yes I was repling to your 1st question.
I am wondering if we can leverage the dateFieldMapper.indexCreatedVersion property to validate the index version?
That should work for upgrade case. Let me confirm it manually, that'll be easy, then I can come up with bwc test case.
The main miss is that I assumed Lucene will allow new segments to have skip list, even if older segments do not. This because query time there's always a check if skiplist iter is available, for each LeafContext. i.e. that assumption is true at query time but not index/ingestion time. |
There was a problem hiding this comment.
@asimmahmood1 - Can we try to use the dateFieldMapper.indexCreatedVersion property to validate the index version?
Signed-off-by: Ankit Jain <jainankitk@apache.org>
|
❌ Gradle check result for 14e006f: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
So manually tested with 3.2.0 index, upgrade to 3.3.0 with version check and this works. Let me see how to write a bwc test and send another PR. |
|
Closing in favor of #19671 |
github-project-automation
bot
moved this from In Progress
to Done
in Performance Roadmap
7 participants
Description
This reverts commit 08ed9ee.
Proper fix will be come up with mechanism to only enable this on new/empty index, e.g. on rollover, since the check is made by Lucene on existing index.
Plus write bwc tests to make upgrades are successful.
FAQ
Why does it affect upgrade?
Lucence has check if you append to an index, it doesn't allow switching from non-skiplist to skiplist. This is not an issue for new/empty index, but affects those that upgrade.
Is there a work around? Can user explicitly set skip_list to false in mapping?
Date skip_list param is introduced in 3.3, previously its only availabe in Number field mapping. So this isn't an option to set prior to 3.3.
Why was not caught in testing?
The unit tests added as part of feature start from new index, there isn't a test to explicilty test to index doc without skip_list, then with skip_list. Existing backward compability tests (bwc) did not fail either, need to check if there are explicit test for @timestamp field in bwc [AI-1]
The manual tests done were using benchmark workloads like big5 and http_logs, those are create only once. I did test loading an old index, which is not an issue, but not subsequent indexing was done, i.e. create <3.3 index, bulk ingest, upgrade to 3.3, no issue, ingest data <- failure.
The main miss is that I assumed Lucene is allow new segments to have skip list, even if older segments do not. This because query time there's always a check if skiplist iter is available, for each LeafContext. i.e. that assumption is true at query time but not index/ingestion time.
Can user set the skip_list to true afterwards and run into same issue?
No, skip_list value is not updateble (creation only).
Related Issues
Fixes #19660 #19646
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.