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

[dbnode] Fix multi-segment field iterator support of __ prefix fields alphanumerically before __m3ninx_id #4095

Merged
merged 3 commits into from
Apr 4, 2022

Conversation

robskillington
Copy link
Collaborator

@robskillington robskillington commented Apr 4, 2022

What this PR does / why we need it:

The specific bug is with how field iterators are built around existing segments and used for in-memory index segment compaction. There was an initial Next call to each segment's field iterator, causing the iterator to start at e1 instead of e0. This is masked/uncommon because of how the iterator sorts these fields, and how the default field doc.IDReservedFieldName (i.e. "_m3ninx_id") alphanumerically precedes most fields and isn't required for field/term indexing. However, fields that start with a double underscore that precedes the ID reserved field name, e.g. "__m2" becomes susceptible for being skipped.

In practice for Prometheus metrics this has not been an issue since fields with double underscores are meant to be used only for internal use as per guidance:

Label names beginning with __ are reserved for internal use.

https://github.com/prometheus/docs/blob/e7567ffa4079b745bf0b4b5c8ef87401d2330588/content/docs/concepts/data_model.md?plain=1#L34-L36

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

@robskillington robskillington enabled auto-merge (squash) April 4, 2022 15:45
@robskillington robskillington merged commit 68be4dd into master Apr 4, 2022
@robskillington robskillington deleted the multi-segment-iter-next-fix branch April 4, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants