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

Report indexing failures on ft.info - MOD-5364 #3682

Merged
merged 54 commits into from Dec 26, 2023

Conversation

DvirDukhan
Copy link
Contributor

@DvirDukhan DvirDukhan commented Jun 27, 2023

This PR adds index failures reporting on the FT.INFO command.
This PR introduces two new objects that hold statistics to be used in the info command

  1. Index error - an object holding the total number of indexing failures, the last index error, and the last failed key.
  2. Field statistics - an object holding any information we found valuable regarding an index field, including the above Index error.

Those objects implement the following new traits:

  1. Reply - The object is responsible for sending the suitable reply
  2. Deserialize - Reconstruct an object from MRReply - valid for cluster operations only
  3. OpPlusEquals (operator +=) - The object can append/merge additional objects to itself - valid for cluster operation only where aggregations are required.

Implementing those traits makes those objects usable in info command code paths both for single shard and cluster.

Usage:

  1. Field statistics is a new array where we can find relevant statistics for each field. For this PR, it will hold only the Index failures, but we can use it the expose memory consumption and other field-specific data.
  2. Index Error is found both on each entry of the field statistics and as a new nested object in the global info command reply scope. It is to replace the hash_indexing_failures entry. This allows registering more general errors, not necessarily related to a specific field.

This PR also introduces more verbose error reporting for hash and JSON indexing failures, both field-related (pre-process) and document related.

TODO:

  • implementation
  • Dedicated flow tests
  • Fix whatever broken in the test suit after modifications
  • Protocols

@DvirDukhan DvirDukhan marked this pull request as ready for review July 23, 2023 21:04
Copy link
Collaborator

@alonre24 alonre24 left a comment

Choose a reason for hiding this comment

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

Great, few last comments

src/info/index_error.c Outdated Show resolved Hide resolved
tests/pytests/test_geometry_flat.py Outdated Show resolved Hide resolved
src/info/index_error.c Show resolved Hide resolved
alonre24
alonre24 previously approved these changes Dec 25, 2023
@GuyAv46 GuyAv46 added this pull request to the merge queue Dec 25, 2023
@GuyAv46 GuyAv46 removed this pull request from the merge queue due to a manual request Dec 25, 2023
@GuyAv46 GuyAv46 added this pull request to the merge queue Dec 25, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 25, 2023
@GuyAv46 GuyAv46 added this pull request to the merge queue Dec 25, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 26, 2023
@GuyAv46 GuyAv46 added this pull request to the merge queue Dec 26, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 26, 2023
@alonre24 alonre24 added this pull request to the merge queue Dec 26, 2023
Merged via the queue into master with commit a35598e Dec 26, 2023
17 checks passed
@alonre24 alonre24 deleted the dvirdu_index_failure_report branch December 26, 2023 17:59
GuyAv46 added a commit that referenced this pull request Dec 26, 2023
* scaffolding

* per field info + vector test

* numeric

* cosmetics

* wip

* wip

* added exception message for hash and JSON failures

* wip

* wip

* pytest pass for index error

* wip

* tests pass + coordinator pass

* removed warnings

* free resources

* clear field spec info

* added const to reply

* free resources

* cast to remove const

* sanitizer build warning fix

* remove print

* free resources

* clear query errors

* post merge fixes

* minor fixes

* more fixes

* import fix

* some tests changes

* fix incorrect offset

* remove unneeded helpers

* move field stats to the end of the info list

* added a test

* align code

* fix for cluster

* simple review fixes

* added timestamps for indexing

* added timespec to the struct

* improved test

* extend mock API

* review fixes

* fix alignment

* fix misalignment

---------

Co-authored-by: GuyAv46 <guy.avimor@gmail.com>
Co-authored-by: GuyAv46 <47632673+GuyAv46@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Dec 26, 2023
* Report indexing failures on ft.info - MOD-5364 (#3682)

* scaffolding

* per field info + vector test

* numeric

* cosmetics

* wip

* wip

* added exception message for hash and JSON failures

* wip

* wip

* pytest pass for index error

* wip

* tests pass + coordinator pass

* removed warnings

* free resources

* clear field spec info

* added const to reply

* free resources

* cast to remove const

* sanitizer build warning fix

* remove print

* free resources

* clear query errors

* post merge fixes

* minor fixes

* more fixes

* import fix

* some tests changes

* fix incorrect offset

* remove unneeded helpers

* move field stats to the end of the info list

* added a test

* align code

* fix for cluster

* simple review fixes

* added timestamps for indexing

* added timespec to the struct

* improved test

* extend mock API

* review fixes

* fix alignment

* fix misalignment

---------

Co-authored-by: GuyAv46 <guy.avimor@gmail.com>
Co-authored-by: GuyAv46 <47632673+GuyAv46@users.noreply.github.com>

* revert numeric types in ft.info

* revert tests info types

* fix index error tests for old info types

* test fixes for coordinator

---------

Co-authored-by: DvirDukhan <dvir@redis.com>
raz-mon pushed a commit that referenced this pull request Mar 6, 2024
* scaffolding

* per field info + vector test

* numeric

* cosmetics

* wip

* wip

* added exception message for hash and JSON failures

* wip

* wip

* pytest pass for index error

* wip

* tests pass + coordinator pass

* removed warnings

* free resources

* clear field spec info

* added const to reply

* free resources

* cast to remove const

* sanitizer build warning fix

* remove print

* free resources

* clear query errors

* post merge fixes

* minor fixes

* more fixes

* import fix

* some tests changes

* fix incorrect offset

* remove unneeded helpers

* move field stats to the end of the info list

* added a test

* align code

* fix for cluster

* simple review fixes

* added timestamps for indexing

* added timespec to the struct

* improved test

* extend mock API

* review fixes

* fix alignment

* fix misalignment

---------

Co-authored-by: GuyAv46 <guy.avimor@gmail.com>
Co-authored-by: GuyAv46 <47632673+GuyAv46@users.noreply.github.com>
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.

None yet

3 participants