feat: expose compaction lifecycle observability metadata - #668
sre-ci-robot merged 2 commits into
Conversation
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
|
Tick the box to add this pull request to the merge queue (same as
|
| string db_name = 2; | ||
| string collection_name = 3; | ||
| // Resolved by the server-side proxy; when set it takes precedence over compactionID. | ||
| int64 collection_id = 4; |
There was a problem hiding this comment.
[review] collection_id precedence is confusing (item 1)
A request field that is "resolved by the server-side proxy" and takes precedence over compactionID is an odd API surface, and it is also redundant with collection_name/db_name. Prefer clarifying the semantics — either drop collection_id and resolve by name server-side, or document exactly how the three selectors (compactionID / collection_name / collection_id) interact. Currently the rules are only implied by comments.
There was a problem hiding this comment.
Clarified in 9c4eea8: collection_name selects all retained automatic and manual tasks and overrides compactionID; Proxy resolves (db_name, collection_name), returning resolution errors without falling back to the job selector. collection_id is internal Proxy-to-DataCoord metadata: Proxy discards client-supplied IDs and DataCoord uses the resolved nonzero ID, otherwise compactionID. Comments were checked against both handlers, Go bindings regenerated, and go test -p 6 -timeout 90s ./... passed in go-api.
| int64 collection_id = 5; | ||
| int64 partition_id = 6; | ||
| string channel = 7; | ||
| string type = 8; |
There was a problem hiding this comment.
[review] state / type as undocumented free-form strings (item 2)
This is an observability API — clients must parse these values. Please at minimum enumerate the allowed string values in comments; better, reuse/extend common.CompactionState (which currently lacks Failed/Timeout states, common.proto:402) so this response chain does not carry two different state representations (GetCompactionPlansResponse.state uses the enum, while CompactionMergeInfo.state is a string).
There was a problem hiding this comment.
Addressed the state portion in c5e8593: CompactionMergeInfo.state now uses a public per-task common.CompactionTaskState enum, while the response-level common.CompactionState remains the aggregate state; go test -p 6 -timeout 90s ./... passed in go-api. The separate type-string concern is unchanged in this state-only update.
There was a problem hiding this comment.
Updated in 4989b8d: CompactionMergeInfo.type now uses the public common.CompactionType enum, so both per-task type and state are typed. CompactionType preserves the internal enum's numeric assignments, including reserved value 1. Go bindings were regenerated and go test -p 6 -timeout 90s ./... passed in go-api. This update covers the public proto contract; DataCoord and SDK adaptations will follow in their respective PRs.
c5e8593 to
9c4eea8
Compare
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
9c4eea8 to
4989b8d
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: congqixia, MrPresent-Han The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Upgrade the complete proto baseline, including compaction lifecycle metadata from milvus-io/milvus-proto#668, and regenerate Python bindings with grpcio-tools 1.66.2 so milvus-io#3791 can contain only SDK implementation and tests after rebasing. Signed-off-by: MrPresent-Han <chun.han@gmail.com>
issue: #3790 related pr: #3791 ## Scope Upgrade PyMilvus's complete protocol baseline separately from the SDK API implementation in #3791. - Advance the proto submodule from `0fb0d5bcf2e2ff4dff30683e5aa0f8991a20d011` directly to `ae7fea6ab2f4e958f2feef0f0edb9a0d23fa7e0c`. - Include the merged compaction protocol changes from milvus-io/milvus-proto#668: public task-state/type enums, retained segment lifecycle filtering and lineage, collection task selectors, failure reasons, and complete task targets. - Include the other upstream protocol updates between these commits for UUID/nested arrays, RLS, snapshot export, import metadata, and shard routing. - Regenerate the Python messages, typing stubs, and service bindings with the repository generator and pinned `grpcio-tools==1.66.2`. - Change only the proto submodule and its nine generated files; no handwritten SDK logic, tests, generators, or dependency constraints are changed. ## Relationship to #3791 This PR now contains the full protocol bump, including compaction metadata; it no longer stops at the pre-compaction revision. Merge this PR first, then rebase #3791 onto master so that #3791 contains only six SDK implementation files and five unit-test files. The entire `pymilvus/grpc_gen` tree, including the proto submodule pointer, matches #3791 exactly, so all ten protocol-related entries disappear from that PR's diff after the rebase, reducing 21 changed files to 11 without changing SDK behavior. ## Validation - The complete protocol tree matches #3791 byte-for-byte. - No handwritten code or tests differ from master in this PR. - Repeating generation produces no diff, and `git diff --check` passes. - Complete `tests/unit` run on the full `ae7fea6a` baseline: **4,718 passed, 3 skipped, 1 failed** in 54.42 seconds. - The only failure is `tests/unit/orm/test_types.py::TestInferDtypeBydata::test_numpy_like_with_dtype_returns_float_vector` (`FLOAT` inferred versus expected `FLOAT_VECTOR`), which was independently reproduced on unchanged `trunk/master` at `a4b3b38bbd8baa4c1047dccb7647a3bc47defcdb` in the same environment during the baseline preparation; no unrelated SDK or test fix is included. - This is protocol-generation and unit-test verification, not Milvus server E2E verification. ```bash PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=. \ python -m pytest -q --disable-warnings --tb=short --show-capture=no \ -o log_cli=false -p no:cacheprovider -p pytest_asyncio.plugin \ -p pytest_timeout --timeout=45 tests/unit ``` Signed-off-by: MrPresent-Han <chun.han@gmail.com> Co-authored-by: MrPresent-Han <chun.han@gmail.com>
3 participants
issue: #667
What changed
Verification