Add TestFileIOFactory helper #2105
Conversation
| @Nonnull Set<PolarisStorageActions> storageActions, | ||
| @Nonnull PolarisResolvedPathWrapper resolvedEntityPath); | ||
|
|
||
| static FileIOFactory wrapExisting(FileIO fileIO) { |
There was a problem hiding this comment.
We used to have a TestFileIOFactory, I wonder if this would be a good time to reintroduce that? The method looks to be used only for tests.
There was a problem hiding this comment.
i would argue this is shorter and more discoverable, also while its true that its only used from tests right now, the logic is still generally correct outside of tests.
if you insist, i can still move it to new TestFileIOFactory (any specific module/package) ?
There was a problem hiding this comment.
I would recommend the org.apache.polaris.service package in testFixtures alongside TestServices.
The reason that I am a bit unsure about putting it outside of test code is that we did in fact design FileIOFactory with a sort of a sort of "wrapper" pattern in mind (see WasbTranslatingFileIOFactory for an example) and I worry that having introducing a method like will have implications on that pattern or on how maintainers perceive that pattern is meant to be implemented.
There was a problem hiding this comment.
just to double check: are you aware the method is static? i.e. it will never become relevant to implementers of the interface afaict
There was a problem hiding this comment.
Yes, I see that it's static (and therefore final). I think this would function well as a static method or a constructor in TestFileIOFactory.
There was a problem hiding this comment.
I worry that having introducing a method like will have implications on that pattern or on how maintainers perceive that pattern is meant to be implemented
my point was that since the method is static this worry is unjustified, the "pattern" is not affected at all by the static method in the interface.
that being said, i have now done as you had requested.
however taking a look at it now, not sure if the cleanup is really worth it as it adds a new class that isnt that easy to discover and its no longer a clear win in terms of lines of code, wdyt?
There was a problem hiding this comment.
It's up to you, I personally still think the cleanup is a good idea as it should make it easier to write new tests. I'm more concerned about lowering the cognitive load needed to set up a new test than about lowering LOC.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
XN137
changed the title
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
* chore(deps): update dependency mypy to >=1.17, <=1.17.0 (apache#2114) * Spark 3.5.6 and Iceberg 1.9.1 (apache#1960) * Spark 3.5.6 and Iceberg 1.9.1 * Cleanup * Add `pathStyleAccess` to AwsStorageConfigInfo (apache#2012) * Add `pathStyleAccess` to AwsStorageConfigInfo This change allows configuring the "path-style" access mode in S3 clients (both in Polaris Servers and Iceberg REST Catalog API clients). This change is applicable both to AWS storage and to non-AWS S3-compatible storage (apache#1530). * Add TestFileIOFactory helper (apache#2105) * Add FileIOFactory.wrapExisting helper * fix(deps): update dependency gradle.plugin.org.jetbrains.gradle.plugin.idea-ext:gradle-idea-ext to v1.2 (apache#2125) * fix(deps): update dependency boto3 to v1.39.7 (apache#2124) * Abstract polaris-runtime-service tests for all persistence implementations (apache#2106) The NoSQL persistence implementation has to run the Iceberg table & view catalog plus the Polaris specific tests as well. Reusing existing tests is beneficial to avoid a lot of code duplcation. This change moves the actual tests to `Abstract*` classes and refactors the existing tests to extend those. The NoSQL persistence work extends the same `Abstract*` classes but runs with different Quarkus test profiles. * Add IMPLICIT authentication support to the CLI (apache#2121) PRs apache#1925 and apache#1912 were merged around the same time. This PR connects the two changes and enables the CLI to accept IMPLICIT authentication type. Since Hadoop federated catalogs rely purely on IMPLICIT authentication, the CLI parsing test has been updated to reflect the same. * feat(helm): Add support for external authentication (apache#2104) * fix(deps): update dependency org.apache.iceberg:iceberg-bom to v1.9.2 (apache#2126) * fix(deps): update quarkus platform and group to v3.24.4 (apache#2128) * fix(deps): update dependency boto3 to v1.39.8 (apache#2129) * fix(deps): update dependency io.smallrye.config:smallrye-config-core to v3.13.3 (apache#2130) * Add newIcebergCatalog helper (apache#2134) creation of `IcebergCatalog` instances was quite redundant as tests mostly use the same parameters most of the time. also remove an unused field in 2 other tests. * Add server and client support for the new generic table `baseLocation` field (apache#2122) * Use Makefile to simplify setup and commands (apache#2027) * Use Makefile to simplify setup and commands * Add targets for minikube state management * Add podman support and spark plugin build * Add version target * Update README.md for Makefile usage and relation to the project * Fix nit * Package polaris client as python package (apache#2049) * Package polaris client as python package * Package polaris client as python package * Change owner to spark when copying files from local into Dockerfile * CI: Address failure from accessing GH API (apache#2132) CI sometimes fails with this failure: ``` * What went wrong: Execution failed for task ':generatePomFileForMavenPublication'. > Unable to process url: https://api.github.com/repos/apache/polaris/contributors?per_page=1000 ``` The sometimes failing request fetches the list of contributors to be published in the "root" POM. Unauthorized GH API requests have an hourly(?) limit of 60 requests per source IP. Authorized requests have a much higher rate limit. We do have a GitHub token available in every CI run, which can be used in GH API requests. This change adds the `Authorization` header for the failing GH API request to leverage the higher rate limit and let CI not fail (that often). * fix(deps): update dependency com.nimbusds:nimbus-jose-jwt to v10.4 (apache#2139) * fix(deps): update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.2.0 (apache#2142) * fix(deps): update dependency software.amazon.awssdk:bom to v2.32.4 (apache#2146) * fix(deps): update dependency org.xerial.snappy:snappy-java to v1.1.10.8 (apache#2138) * fix(deps): update dependency org.junit:junit-bom to v5.13.4 (apache#2147) * fix(deps): update dependency boto3 to v1.39.9 (apache#2137) * fix(deps): update dependency com.fasterxml.jackson:jackson-bom to v2.19.2 (apache#2136) * Python client: add support for endpoint, sts-endpoint, path-style-access (apache#2127) This change adds support for endpoint, sts-endpoint, path-style-access to the Polaris Python client. Amends apache#1913 and apache#2012 * Remove PolarisEntityManager.getCredentialCache (apache#2133) `PolarisEntityManager` itself is not using the `StorageCredentialCache` but just hands it out via `getCredentialCache`. the only caller of `getCredentialCache` is `FileIOUtil.refreshAccessConfig`, which in in turn is only called by `DefaultFileIOFactory` and `IcebergCatalog`. note that in a follow-up we will likely be able to remove `PolarisEntityManager` usage completely from `IcebergCatalog`. additional cleanups: - use `StorageCredentialCache` injection in tests (but we need to invalidate all entries on test start) - remove unused `UserSecretsManagerFactory` from `PolarisCallContextCatalogFactory` * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.22-1.1752676419 (apache#2150) * fix(deps): update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.2.1 (apache#2152) * fix(deps): update dependency boto3 to v1.39.10 (apache#2151) * chore: fix class reference in the javadoc of TableLikeEntity (apache#2157) * fix(deps): update dependency commons-codec:commons-codec to v1.19.0 (apache#2160) * fix(deps): update dependency boto3 to v1.39.11 (apache#2159) * Last merged commit 395459f --------- Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Pooja Nilangekar <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: Yun Zou <[email protected]>
3 participants
also cleanup warnings in
TableCleanupTaskHandlerTest