[DO-NOT-MERGE] Change MinIO to RustFS for integration testing#3663
[DO-NOT-MERGE] Change MinIO to RustFS for integration testing#3663MonkeyCanCode wants to merge 3 commits intoapache:mainfrom
Conversation
adutra
left a comment
There was a problem hiding this comment.
Thanks for the PR @MonkeyCanCode , it's really interesting that you were able to migrate so smoothly!
Indeed, I wonder if it wouldn't be better to have both MinIO and RustFS tests, at least for some time.
Also, the need for a fixed port is a minor annoyance, but we can live with that I guess.
| addExposedPort(DEFAULT_PORT); | ||
| withNetworkAliases(randomString("rustfs")); | ||
| withLogConsumer(new Slf4jLogConsumer(LoggerFactory.getLogger(RustfsContainer.class))); | ||
| addFixedExposedPort(S3_API_PORT, S3_API_PORT); |
There was a problem hiding this comment.
Do we really need fixed ports?
There was a problem hiding this comment.
Hmm I see, it's explained here:
https://lists.apache.org/thread/5cvgk6m83hnsoly9gq4bxdchcc69f3l0
That's unfortunate, because it could cause test failures especially for devs (not so much for CI imho).
There was a problem hiding this comment.
Could we have a code comment for this too (for ease of recollection later :) )?
There was a problem hiding this comment.
Yeah. that is the unfortunate pieces I found so far. I will add a comment for this.
| private static final String MINIO_DOMAIN = "MINIO_DOMAIN"; | ||
| private static final String RUSTFS_ACCESS_KEY = "RUSTFS_ACCESS_KEY"; | ||
| private static final String RUSTFS_SECRET_KEY = "RUSTFS_SECRET_KEY"; | ||
| private static final String RUSTFS_SERVER_DOMAINS = "RUSTFS_SERVER_DOMAINS"; |
There was a problem hiding this comment.
nit: why "DOMAINS" in the plural?
There was a problem hiding this comment.
This is because RUSTFS_SERVER_DOMAIN is not valid but RUSTFS_SERVER_DOMAINS instead (https://github.com/rustfs/rustfs/blob/e30781654d26006cda575fa0746775eb3d29b8d3/rustfs/src/config/mod.rs#L73). But I think you are ref to the variable name. I am changing this to RUSTFS_DOMAIN in the new PR as we are using one domain only.
|
Heads up: you need to rebase this PR for it to pass CI. |
+1 to running MinIO tests and RustFS tests in parallel for a while. If all is well in CI we can reduce load by removing MinIO tests later. |
|
We could probably start with adapting these tests to RustFS (by copy initially): |
| - Changed from Poetry to UV for Python package management. | ||
| - Exclude KMS policies when KMS is not being used for S3. | ||
| - Improved default KMS permission handling to better distinguish read-only and read-write access. | ||
| - Replaced MinIO with RustFS for integration testing. |
There was a problem hiding this comment.
nit: TBH, I doubt users are interested in this and developers can view GH PRs 😉
| assertThatThrownBy(() -> spark.sql("INSERT INTO ns1.test_table VALUES (3, 'Charlie')")) | ||
| .hasMessageContaining( | ||
| "software.amazon.awssdk.services.s3.model.S3Exception: Access Denied. (Service: S3, Status Code: 403,"); | ||
| "software.amazon.awssdk.services.s3.model.S3Exception: Access Denied (Service: S3, Status Code: 403,"); |
There was a problem hiding this comment.
Suggestion: use RegEx for account for both cases.
There was a problem hiding this comment.
Sure thing. let me update it in a diff PR.
|
Actually, this PR looks good to me as it is. Perhaps, as discussed above, we could keep |
|
Thanks so much for the quick review @dimas-b and @adutra . I think we all think it is better to have both minio and rustfs test-containers co-existed and copied over the existed s3-compatible storage IT over with the needed changes in this PR. I will keep this one and mark it as DO-NOT-MERGE in case if we have more people who would like to review. In the mean time, I will create a different PR for the new test-containers. |
I am not sure if we want to take this PR as it is changing existed integration testing on S3-compatible storage from MinIO to RustFS instead of creating a new test-containers. But here are the high level changes from this PR:
ML: https://lists.apache.org/thread/8o31ly7cd8ov70opjbtg630qlhrfl5yh
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)