Skip to content

Conversation

Nivesh-01
Copy link
Collaborator

We validate wether the metadata has been propogated accross the cluster properly. We check the consistency accross different nodes in a cluster to see if everything is properly migrated.
This PR also has some changes related to #395 -> this is going to be merged into main and then in to fulltext but would be a blocking change hence added those same changes in this PR so that the testing can be comprehensive.

Nivesh Tuwani added 2 commits September 23, 2025 17:32
Signed-off-by: Nivesh Tuwani <tuwanivu@amazon.com>
I, Nivesh Tuwani <tuwanivu@amazon.com>, hereby add my Signed-off-by to this commit: 8efa09f

Signed-off-by: Nivesh Tuwani <tuwanivu@amazon.com>

Signed-off-by: Nivesh Tuwani <tuwanivu@amazon.com>
@BCathcart
Copy link
Collaborator

I'm going to push back a bit on the amount of tests and whether they all belong in the same file again.

First off, I'm not sure test_basic_text_index_metadata_validation is needed. I think just having test_complex_text_index_metadata_validation would be fine. I see that you're adding other field types in the complex test which I think is good because we don't seem to have a FT.CREATE integration test that already does this for the existing fields. Since we're testing the effects of FT.CREATE, I would consider moving it to the existing test_ft_create.py.

Next it looks like test_index_with_data_metadata_validation has good intentions to test that we can actually use the index cluster-wide. I'm not sure it's actually testing anything right now though. It just asserts that the indexes are in the ready state. I'd approach this more as an end-to-end sanity test where we create the index, ingest docs, and then do queries on them.

test_error_handling_metadata_validation tests invalid requests for FT.INFO and FT.CREATE. I'd check if these aren't already covered by any existing testing. From a quick glance it looks like there might already be coverage for FT.INFO in our integration tests and FT.CREATE in unit tests. If you see gaps in the invalid FT.CREATE command coverage, I'd add additional unit tests to ft_create_test.cc

Signed-off-by: Nivesh Tuwani <tuwanivu@amazon.com>
…os that would be used in later integration tests along with this one

Signed-off-by: Nivesh Tuwani <tuwanivu@amazon.com>
@Nivesh-01 Nivesh-01 force-pushed the clusterMetadata branch 2 times, most recently from bae8288 to b9b0528 Compare October 7, 2025 23:34
Signed-off-by: Nivesh Tuwani <tuwanivu@amazon.com>
Signed-off-by: Nivesh Tuwani <tuwanivu@amazon.com>
Copy link
Collaborator

@BCathcart BCathcart left a comment

Choose a reason for hiding this comment

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

Looks good overall. Let's meet tomorrow afternoon and clean up these last few small things.

node = self.new_client_for_primary(i)
result = node.execute_command("FT._LIST")
results.append(result)
return results
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add get_ft_list() to IndexingTestHelper and then get_ft_list_from_all_nodes and get_ft_info_from_all_nodes aren't really needed

Signed-off-by: Nivesh Tuwani <tuwanivu@amazon.com>
@Nivesh-01 Nivesh-01 merged commit d6b63f7 into valkey-io:fulltext Oct 11, 2025
5 checks passed
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