You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "pingtimeout (via GitHub)" <gi...@apache.org> on 2026/01/23 11:17:57 UTC

[PR] Test libraries for storage operations [polaris]

pingtimeout opened a new pull request, #3513:
URL: https://github.com/apache/polaris/pull/3513

   As part of #3256 , the following test libraries were added from the Nessie project:
   * `objet-storage-mock` - Allows for mocking the object storage layer and simulate any Iceberg table, including tables with extremely large number of files
   * `azurite-testcontainer` - Allows for simulating Azure Storage with testcontainers in Polaris codebase
   * `gcs-testcontainer` -  Allows for simulating GCS with testcontainers in Polaris codebase
   
   There was an explicit request to pull the code into Polaris, instead of depending on that code as a library (https://github.com/apache/polaris/pull/3256#discussion_r2673402956, https://github.com/apache/polaris/pull/3256#pullrequestreview-3640674712).  This is what this PR does.
   
   This PR introduces three test-only libraries in the `tools` directory so that they can be used by #3256.
   
   In case it can be useful, [here is a bash script](https://github.com/user-attachments/files/24819477/generate-nessie-diff-commands.sh) that generates `diff` commands for all files added in this PR against a Nessie folder.  It can be used to verify that the code that comes from Nessie has not been modified, apart from package names, code-copied-to-polaris marker and other minor tweaks.
   
   I have also tested the current PR by integrating it with #3256 and verifying that all the tests from #3256 still pass.
   
   ## Checklist
   
   - [x] ๐Ÿ›ก๏ธ Don't disclose security issues! (contact security@apache.org)
   - [x] ๐Ÿ”— Clearly explained why the changes are needed, or linked related issues: Fixes #
   - [ ] ๐Ÿงช Added/updated tests with good coverage, or manually tested (and explained how)
   - [x] ๐Ÿ’ก Added comments for complex logic
   - [ ] ๐Ÿงพ Updated `CHANGELOG.md` (if needed)
   - [ ] ๐Ÿ“š Updated documentation in `site/content/in-dev/unreleased` (if needed)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3791490921

   i believe we are reinventing the wheel here, why not use something like S3Mock ? 
   https://github.com/adobe/S3Mock, it a well maintained library ~1k stars 
   
   can we do more thorough analysis on why we need this custom code and not use something like libraries like S3Mock 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3800634610

   > Let's use the Nessie test dependency (it doesn't hurt)
   
   +1 to this from my POV. I believe it is the easiest option both in terms of license (Nessie is ALv2) and maintenance (the code is covered by active CI under Project Nessie).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #3513:
URL: https://github.com/apache/polaris/pull/3513#discussion_r2722214380


##########
tools/object-storage-mock/src/main/java/org/apache/polaris/test/objectstoragemock/S3Resource.java:
##########
@@ -0,0 +1,513 @@
+/*
+ * Copyright (C) 2022 Dremio
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+// CODE_COPIED_TO_POLARIS from Project Nessie 0.106.1
+package org.apache.polaris.test.objectstoragemock;
+
+import static com.google.common.net.HttpHeaders.CONTENT_MD5;
+import static jakarta.ws.rs.core.HttpHeaders.CONTENT_ENCODING;
+import static jakarta.ws.rs.core.HttpHeaders.CONTENT_LENGTH;
+import static jakarta.ws.rs.core.HttpHeaders.CONTENT_TYPE;
+import static jakarta.ws.rs.core.HttpHeaders.IF_MATCH;
+import static jakarta.ws.rs.core.HttpHeaders.IF_MODIFIED_SINCE;
+import static jakarta.ws.rs.core.HttpHeaders.IF_NONE_MATCH;
+import static jakarta.ws.rs.core.HttpHeaders.IF_UNMODIFIED_SINCE;
+import static java.time.format.DateTimeFormatter.ISO_OFFSET_DATE_TIME;
+import static java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME;
+import static org.apache.polaris.test.objectstoragemock.s3.S3Constants.CONTINUATION_TOKEN;
+import static org.apache.polaris.test.objectstoragemock.s3.S3Constants.ENCODING_TYPE;
+import static org.apache.polaris.test.objectstoragemock.s3.S3Constants.LIST_TYPE;
+import static org.apache.polaris.test.objectstoragemock.s3.S3Constants.MAX_KEYS;
+import static org.apache.polaris.test.objectstoragemock.s3.S3Constants.RANGE;
+import static org.apache.polaris.test.objectstoragemock.s3.S3Constants.START_AFTER;
+import static org.apache.polaris.test.objectstoragemock.s3.S3Constants.X_AMZ_REQUEST_ID;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.JsonNodeFactory;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import jakarta.annotation.Nullable;
+import jakarta.inject.Inject;
+import jakarta.ws.rs.Consumes;
+import jakarta.ws.rs.DELETE;
+import jakarta.ws.rs.DefaultValue;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.HEAD;
+import jakarta.ws.rs.HeaderParam;
+import jakarta.ws.rs.POST;
+import jakarta.ws.rs.PUT;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.QueryParam;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+import jakarta.ws.rs.core.Response.Status;
+import jakarta.ws.rs.core.StreamingOutput;
+import java.io.InputStream;
+import java.time.Instant;
+import java.time.ZoneId;
+import java.time.ZonedDateTime;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.Spliterator;
+import java.util.function.Function;
+import java.util.stream.Stream;
+import org.apache.polaris.test.objectstoragemock.Bucket.ListElement;
+import org.apache.polaris.test.objectstoragemock.s3.BatchDeleteRequest;
+import org.apache.polaris.test.objectstoragemock.s3.ErrorResponse;
+import org.apache.polaris.test.objectstoragemock.s3.ImmutableBatchDeleteResponse;
+import org.apache.polaris.test.objectstoragemock.s3.ImmutableBucket;
+import org.apache.polaris.test.objectstoragemock.s3.ImmutableBuckets;
+import org.apache.polaris.test.objectstoragemock.s3.ImmutableDeletedS3Object;
+import org.apache.polaris.test.objectstoragemock.s3.ImmutableErrorObj;
+import org.apache.polaris.test.objectstoragemock.s3.ImmutableListAllMyBucketsResult;
+import org.apache.polaris.test.objectstoragemock.s3.ImmutableS3Object;
+import org.apache.polaris.test.objectstoragemock.s3.ListAllMyBucketsResult;
+import org.apache.polaris.test.objectstoragemock.s3.ListBucketResult;
+import org.apache.polaris.test.objectstoragemock.s3.ListBucketResultBase;
+import org.apache.polaris.test.objectstoragemock.s3.ListBucketResultV2;
+import org.apache.polaris.test.objectstoragemock.s3.Owner;
+import org.apache.polaris.test.objectstoragemock.s3.Prefix;
+import org.apache.polaris.test.objectstoragemock.s3.S3ObjectIdentifier;
+import org.apache.polaris.test.objectstoragemock.util.Holder;
+import org.apache.polaris.test.objectstoragemock.util.PrefixSpliterator;
+import org.apache.polaris.test.objectstoragemock.util.StartAfterSpliterator;
+
+@Path("/")
+@Produces(MediaType.APPLICATION_XML)
+@Consumes(MediaType.APPLICATION_XML)
+public class S3Resource {
+  @Inject ObjectStorageMock mockServer;
+
+  private static final Owner TEST_OWNER = Owner.of(42, "nessie-iceberg-s3-mock");

Review Comment:
   may be confusing later, not a big deal for now though



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3791622613

   @singhpk234 : 
   
   > i believe we are reinventing the wheel here, [...]
   
   In short, the technical capabilities of these testing utilities are different from S3Mock, which is why they were developed under Project Nessie in the first place. One such aspect is `InterceptingBucket`, which is quite useful in tests. This functionality is currently used in #3256, where this PR originates.
   
   As @RussellSpitzer noted, it is certainly possible to make adjustments later if developers find more efficient ways to work with and test the code in #3256.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3797894222

   I think @singhpk234 has a good point. Given the size of this change, around 5600 LOC(79 new files), I think it is worth pausing and discussing this on the dev mailing list. Reusing an existing, well maintained library where possible is generally preferable, and we should be careful about reinventing the wheel unless there is a clear technical gap that existing solutions cannot cover.
   
   It would be good to have a broader discussion on dev ML about:
   โ€ข what concrete requirements are not met by existing libraries like `S3Mock`
   โ€ข whether those gaps are fundamental or could be addressed with extension or configuration
   โ€ข the long term maintenance cost of carrying a large custom test stack in Polaris
   
   If we agree that custom code is still the right direction after that discussion, at least we will have alignment and a documented rationale.
   
   Happy to help start a dev list thread if that sounds reasonable.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3791380496

   An easy fix here to trim the size of this PR would be to split it into one per container rather than moving all of them at once. Just a guide for the future.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #3513:
URL: https://github.com/apache/polaris/pull/3513#discussion_r2722174469


##########
tools/azurite-testcontainer/src/main/resources/org/apache/polaris/test/azurite/Dockerfile-azurite-version:
##########
@@ -0,0 +1,23 @@
+#

Review Comment:
   Is this all new? Just noting it has the Apache license and not the Dremio one



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3798237003

   If it's a concern about the size of this PR, I wonder if it's not better to be back on the initial proposal: using nessie test dependecy as a library and create an issue to find alternative.
   I think it would be reasonable because some asked to remove the nessie test dependency in favor of the code copy, and now we have concerns about this code.
   It looks like we do ping pong here.
   As a consensus I propose:
   1. Let's use the Nessie test dependency (it doesn't hurt)
   2. Create an issue to keep in mind to find alternative (using s3mock or others)
   Thoughts ?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3798089176

   I agree for a discussion on the dev mailing list. Happy to drive it if needed. 
   Also this PR is large with code donation (I have to check if it doesn't require SGA regarding the scope and size). 
   I'm not convinced we need everything in there. Maybe worth to cleanup and see what's actually needed. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3513: Test libraries for storage operations
URL: https://github.com/apache/polaris/pull/3513


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3801894042

   Just to clarify my position, I am not against adding dependencies or accepting donations. My concern is more about locking ourselves into a single option too early. I think it would be good to explore a bit more, look at multiple libraries or approaches, and understand what concrete requirements we actually have before converging. Is that allowed by the community?
    
   From that angle, a discussion on the dev mailing list feels reasonable, both to validate the direction and to surface any other concerns the wider community may have.
   
   Happy to proceed once we have that shared context and alignment.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3976122130

   This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3791550363

   I'm fine with adding this for now, since it's just a test dependency, if we want to use S3 mock or something like that in the future it's still available.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Test libraries for storage operations [polaris]

Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #3513:
URL: https://github.com/apache/polaris/pull/3513#issuecomment-3791814494

   Thank you for sharing this @dimas-b ! I believe thats precisely i was looking to get answer for !
   
   I wonder if can't we adjust the tests to work with s3mocks rather than writing whole req / responses even introducing things like [Owner](https://github.com/apache/iceberg/pull/1573/changes#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R254). 
   
   my understanding was that iceberg uses s3 mock [pointer](https://github.com/apache/iceberg/pull/1573/changes#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R254), even trino uses s3 mock [Trino](https://github.com/trinodb/trino/pull/27257) so i was confused on what exactly are we testing thats not possible with s3mock as we see other projects which do similar stuff is capable of. 
   
   I believe we just want to check it in and use it for tests (https://github.com/apache/polaris/pull/3256) without changing we have  and think about it later ? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org