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

fix: Folders are also considered for WalkWithObjects to adhere bulk insert failure on using GCS buckets #39352

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

gifi-siby
Copy link
Contributor

Related task: #39134

Previous PR: #39150

@sre-ci-robot sre-ci-robot added the size/S Denotes a PR that changes 10-29 lines. label Jan 16, 2025
Copy link
Contributor

mergify bot commented Jan 16, 2025

@gifi-siby Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco.

@mergify mergify bot added needs-dco DCO is missing in this pull request. kind/bug Issues or changes related a bug labels Jan 16, 2025
@gifi-siby gifi-siby force-pushed the gcs_bulk_insert branch 2 times, most recently from 8644c8f to 88af81a Compare January 16, 2025 11:03
@mergify mergify bot added dco-passed DCO check passed. and removed needs-dco DCO is missing in this pull request. labels Jan 16, 2025
Copy link
Contributor

mergify bot commented Jan 16, 2025

@gifi-siby go-sdk check failed, comment rerun go-sdk can trigger the job again.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.03%. Comparing base (75d7978) to head (7766fb6).
Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
internal/storage/gcp_native_object_storage.go 14.28% 4 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #39352      +/-   ##
==========================================
- Coverage   81.03%   81.03%   -0.01%     
==========================================
  Files        1407     1407              
  Lines      198527   198530       +3     
==========================================
- Hits       160885   160884       -1     
  Misses      31983    31983              
- Partials     5659     5663       +4     
Components Coverage Δ
Client 79.50% <ø> (ø)
Core 69.50% <ø> (ø)
Go 82.98% <14.28%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
internal/storage/gcp_native_object_storage.go 59.22% <14.28%> (-0.88%) ⬇️

... and 32 files with indirect coverage changes

@gifi-siby
Copy link
Contributor Author

@xiaofan-luan commented in @39150 that the implementation of gcp_native_object storage can be wrong because the test result of it is different from minio_object_storage test and suggested to modify gcp_native_object_storage to follow the behaviour of minio storage.

Replied here: #39150#issuecomment-2594735364
GCS treats slashes (/) in object keys as part of a simulated folder structure. For example, when you try to create "abc/e/d" after "abc/d/e", GCS detects a conflict because it interprets the object paths as part of a hierarchy. It sees "abc/d/e" and "abc/e/d" as related and checks for consistency in the way these paths are structured.
MinIO, on the other hand, uses a flat object storage model, where slashes are simply characters in the object name and don't imply any folder structure. Therefore, MinIO allows both "abc/d/e" and "abc/e/d" without any issues.
This difference in behaviour is due to how GCS and MinIO handle object keys – it's not a bug in GCS.

But we can stimulate the behaviour of azure in GCS. So, I'll modify the test scenarios according to azure.

@gifi-siby
Copy link
Contributor Author

rerun go-sdk

Copy link
Contributor

mergify bot commented Jan 17, 2025

@gifi-siby go-sdk check failed, comment rerun go-sdk can trigger the job again.

@xiaofan-luan
Copy link
Collaborator

rerun go-sdk

@mergify mergify bot added the ci-passed label Jan 18, 2025
@xiaofan-luan
Copy link
Collaborator

/lgtm
/approve

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gifi-siby, xiaofan-luan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot merged commit f54e8e9 into milvus-io:master Jan 18, 2025
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved ci-passed dco-passed DCO check passed. kind/bug Issues or changes related a bug lgtm size/S Denotes a PR that changes 10-29 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants