Conversation
- Keep deletion of third_party/agfs (deleted by us) - Merge other upstream changes
Port of PR volcengine#1333 from Go version to Rust: - Add disable_batch_delete config option to S3Client - When enabled, use sequential single-object deletes instead of DeleteObjects - This is for S3-compatible services like Alibaba Cloud OSS that require Content-MD5 for DeleteObjects but AWS SDK v2 does not send it by default - Add documentation and config example for OSS
|
openviking seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
Add disable_batch_delete to the s3_plugin_config dict in _generate_plugin_config so that the Python config can properly control the Rust S3FS plugin's behavior.
qin-ctx
left a comment
There was a problem hiding this comment.
Porting #1333's disable_batch_delete behavior into the Rust S3 path looks directionally correct, but this PR also removes the Go/AGFS compatibility layer. I found a couple of blocking gaps in that migration: schema field removals currently break checked-in configs/tests/examples, and cross-config validation still expects storage.agfs.url even though this PR removes that field. I also left one non-blocking suggestion about adding a regression test for the new sequential-delete path.
| description="[Deprecated in favor of `storage.workspace`] RAGFS data storage path. This will be ignored if `storage.workspace` is set.", | ||
| ) | ||
|
|
||
| backend: str = Field( |
There was a problem hiding this comment.
[Bug] (blocking) This schema drop removes mode, port, log_level, retry_times, etc. while the repository still contains configs/tests that rely on them (for example tests/server/test_api_sessions.py, tests/session/test_session_context.py, and tests/test_prompt_manager.py). On this branch those suites now fail during config loading with Unknown config field 'storage.agfs.mode'. If the migration to binding-only RAGFS is intentional, the callers/docs/tests need to be migrated in the same PR; otherwise AGFSConfig still needs a compatibility layer.
| description="Path to AGFS binding shared library. If set, use python binding instead of HTTP client. " | ||
| "Default: third_party/agfs/bin/libagfsbinding.{so,dylib}", | ||
| ) | ||
| timeout: int = Field(default=10, description="RAGFS request timeout (seconds)") |
There was a problem hiding this comment.
[Bug] (blocking) Removing url here is incomplete because openviking_cli.utils.config.open_viking_config.is_valid_openviking_config() still reads config.storage.agfs.url for the vectordb.backend == "http" and agfs.backend == "local" case. A minimal config in that mode now raises AttributeError during validation instead of a user-facing config error. This PR needs to update that cross-config validation at the same time as the schema change.
| "prefix": "", | ||
| "use_ssl": true | ||
| "use_ssl": true, | ||
| "disable_batch_delete": false |
There was a problem hiding this comment.
[Bug] (blocking) This example now advertises disable_batch_delete, but the surrounding agfs block still contains port, log_level, and retry_times above. After the AGFSConfig schema change in this PR those fields are invalid, so copying this example into ov.conf will fail before the new S3 option can even be used.
| .send() | ||
| .await | ||
| .map_err(|e| Error::internal(format!("S3 DeleteObjects error: {}", e)))?; | ||
| if self.disable_batch_delete { |
There was a problem hiding this comment.
[Suggestion] (non-blocking) Please add a regression test that exercises this branch end-to-end. #1333 existed because the old stack silently missed forwarding this flag once already, and this PR rewrites the whole Go path away. Without a test that proves disable_batch_delete=true reaches the sequential-delete path, this is easy to regress again in a future refactor.
remove golang depends
and rewrite #1333