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

feat(core)!: implement write returns metadata for s3 #5562

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

meteorgan
Copy link
Contributor

@meteorgan meteorgan commented Jan 19, 2025

Which issue does this PR close?

Part of #5557

Rationale for this change

What changes are included in this PR?

  • implement write returns metadata for Operator and BlockingOperator
  • implement the logic for service S3
  • other services return default Metadata after writing
  • skip return value for language bindings

Are there any user-facing changes?

Yes.

  • write, write_with in Operator returns Result<Metadata> instead of Return<()>
  • Writer.close() returns Return<Metadata> instead of Return<()>
  • write in BlockingOperator returns Return<Metadata> instead of Return<()>
  • FunctionWrite.call() used by BlockingOperator returns Return<Metadata> instead of Return<()>

@meteorgan meteorgan force-pushed the write_returns_metadata_for_s3 branch from 8591861 to 5c98d37 Compare January 19, 2025 10:44
@meteorgan
Copy link
Contributor Author

emmm, another issue related to ceph rados: the Etag returned by CompleteMultipartUpload doesn't follow the standard.
image

@Xuanwo
Copy link
Member

Xuanwo commented Jan 20, 2025

the Etag returned by CompleteMultipartUpload doesn't follow the standard.

Well, well, well...

Let's do our best to permit this since it's not harmful. Technically, we don't allow users to parse the ETag value; instead, they should store it and compare it with values from the same source. I'm guessing the issue lies in part of our code assuming that the ETag must contain "?

@meteorgan
Copy link
Contributor Author

the Etag returned by CompleteMultipartUpload doesn't follow the standard.

Well, well, well...

Let's do our best to permit this since it's not harmful. Technically, we don't allow users to parse the ETag value; instead, they should store it and compare it with values from the same source. I'm guessing the issue lies in part of our code assuming that the ETag must contain "?

No. The issue is that I compare the Etag from stat with the one from write in the tests to ensure we get the correct Etag if it's returned by write. However, the value returned from stat contain " while the value returned from write does not. they aren't consistent with each other, even within the same service !

@Xuanwo
Copy link
Member

Xuanwo commented Jan 20, 2025

No. The issue is that I compare the Etag from stat with the one from write in the tests to ensure we get the correct Etag if it's returned by write. However, the value returned from stat contain " while the value returned from write does not. they aren't consistent with each other, even within the same service !

That's really surprising. Let's disable the Ceph RADOS test and create an issue to track it. I believe such a bug isn't worth our workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants