feat: Check per-node shard status in wait_for_vector_indexing

[bevzzz]

This PR is going to deprecate shard.status and shard.vectorQueueSize fields and introduce per_node_status in its stead. We need to update wait_for_vector_indexing to use this information when waiting for shards to get READY.

[orca-security-eu]

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Infrastructure as Code	0   0   0   0	View in Orca
Passed	SAST	0   0   0   0	View in Orca
Passed	Secrets	0   0   0   0	View in Orca
Passed	Vulnerabilities	0   0   0   0	View in Orca

[codecov-commenter]

Codecov Report

❌ Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.53%. Comparing base (95b5d76) to head (649c6d6).
⚠️ Report is 79 commits behind head on main.

Files with missing lines	Patch %	Lines
integration/test_batch_v4.py	54.54%	5 Missing ⚠️
integration/test_collection_config.py	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2127      +/-   ##
==========================================
+ Coverage   86.64%   88.53%   +1.88%     
==========================================
  Files         300      304       +4     
  Lines       23172    23471     +299     
==========================================
+ Hits        20077    20779     +702     
+ Misses       3095     2692     -403     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tsmith023]

With the change on the server, should this go back to being ShardTypes?

[bevzzz]

Agree, reverted.

[tsmith023]

Ah, I also meant that it should be unconditional on this if statement too. So the L90 replacement would be reintroduced, i.e. the test asserts that it always gets shards[0].status == "READY" back thereby preserving BC. Likewise below

[bevzzz]

Ah, I also meant that it should be unconditional on this if statement too

I don't quite understand why. We know that shards.status is a source of flakiness in tests. Wouldn't we want to avoid relying on it if possible?

thereby preserving BC

This is test code though. What compatibility would we be preserving?

[bevzzz]

Like in wait_for_vector_indexing, we prefer per_node_status.

weaviate-python-client/weaviate/collections/batch/batch_wrapper.py

Lines 99 to 104 in 649c6d6

	all(
	status == "READY"
	for status in cast(dict[str, str], shard["per_node_status"]).values()
	)
	if "per_node_status" in shard
	else cast(str, shard["status"]) == "READY"

and not

 all( 
     status == "READY" 
     for status in cast(dict[str, str], shard.get("per_node_status", {})).values() 
 ) and cast(str, shard["status"]) == "READY"

[bevzzz]

I walked around a bit and realized that all("READY") implies shard.status == "READY".
Sorry for the confusion, will update.

[jfrancoa]

If "per_node_status" is missing from shard because no shard could be retrieved (not sure if this can happen from server side), we would move to the else statement, in which case the status will be also empty because we couldn't retrieve any state and accessing directly shard["status"] will error. Could you double check if this scenario is even possible?

[bevzzz]

@tsmith023 Jose and I discussed the possibility of adding UNAVAILABLE status to represent nodes which the coordinator couldn't reach. Currently, when a node fails to respond it will be missing from the shard's per_node_status and the caller will need to know the what the replication factor is to detect that.

WDYT?