Skip to content

Integration tests: improve support for temporary folders #987

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.

Sign up for GitHub

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

Merged
adutra merged 3 commits into from
Feb 25, 2025
Merged

Integration tests: improve support for temporary folders #987

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14ee454
Integration tests: improve support for temporary folders
adutra Feb 11, 2025
af04134
[review] consolidate variables
adutra Feb 12, 2025
0bb1c06
review
adutra Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 20 additions & 28 deletions ...s/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.Response.Status;
import java.io.IOException;
import java.nio.file.Files;
import java.net.URI;
import java.nio.file.Path;
import java.time.Duration;
import java.time.temporal.ChronoUnit;
import java.util.List;
import java.util.Map;
import org.apache.commons.io.FileUtils;
import java.util.Optional;
import org.apache.hadoop.conf.Configuration;
import org.apache.iceberg.BaseTable;
import org.apache.iceberg.PartitionData;
Expand Down Expand Up @@ -87,6 +87,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

Expand All @@ -109,7 +110,6 @@ public class PolarisApplicationIntegrationTest {

public static final String PRINCIPAL_ROLE_ALL = "PRINCIPAL_ROLE:ALL";

private static Path testDir;
private static String realm;

private static RestApi managementApi;
Expand All @@ -118,25 +118,26 @@ public class PolarisApplicationIntegrationTest {
private static ClientCredentials clientCredentials;
private static ClientPrincipal admin;
private static String authToken;
private static URI baseLocation;

private String principalRoleName;
private String internalCatalogName;

@BeforeAll
public static void setup(PolarisApiEndpoints apiEndpoints, ClientPrincipal adminCredentials)
throws IOException {
public static void setup(
PolarisApiEndpoints apiEndpoints, ClientPrincipal adminCredentials, @TempDir Path tempDir) {
endpoints = apiEndpoints;
client = polarisClient(endpoints);
realm = endpoints.realmId();
admin = adminCredentials;
clientCredentials = adminCredentials.credentials();
authToken = client.obtainToken(clientCredentials);

testDir = Path.of("build/test_data/iceberg/" + realm);
FileUtils.deleteQuietly(testDir.toFile());
Files.createDirectories(testDir);

managementApi = client.managementApi(clientCredentials);
baseLocation =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_TEMP_DIR"))
.map(URI::create)
.orElse(tempDir.toUri())
.resolve(realm + "/");
Copy link
Contributor

@dimas-b dimas-b Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may have unexpected behaviour... IIRC s3://bucket/a/b .resolve("realm/") will yield s3://bucket/a/realm/... Did you mean that?

It might be best to do a simple string append and then call URI.normalize().

Also, some valid S3 locations do not pass URI validation checks cf. projectnessie/nessie#8328 .... so in the end I think it might be best to stick with string concat until Polaris gets tooling for dealing with S3 locations that are not proper URIs 😅

Copy link
Contributor Author

@adutra adutra Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may have unexpected behaviour... IIRC s3://bucket/a/b .resolve("realm/") will yield s3://bucket/a/realm/... Did you mean that?

No, of course; the expectation is that INTEGRATION_TEST_TEMP_DIR must end with a trailing slash.

It might be best to do a simple string append and then call URI.normalize().

Yep, let me change that.

Also, some valid S3 locations do not pass URI validation checks [...] I think it might be best to stick with string concat

I remember the pain, but OTOH I dislike writing stringly-typed code. Do you think the issues we had in Nessie would manifest here? Honestly I expect these URIs to be as simple as s3://test-bucket, so they should be parsable. And also, as you said, Polaris is not ready yet to handle those problematic S3 locations.

Copy link
Contributor

@dimas-b dimas-b Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think we can assume that Polaris tests use only well-formed URIs as base locations.

}

@AfterAll
Expand Down Expand Up @@ -443,19 +444,17 @@ public void testIcebergRegisterTableInExternalCatalog() throws IOException {
Catalog.TypeEnum.EXTERNAL,
principalRoleName,
FileStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.FILE)
.setAllowedLocations(List.of("file://" + testDir.toFile().getAbsolutePath()))
.setAllowedLocations(List.of(baseLocation.toString()))
.build(),
"file://" + testDir.toFile().getAbsolutePath());
baseLocation.toString());
try (RESTSessionCatalog sessionCatalog = newSessionCatalog(catalogName);
HadoopFileIO fileIo = new HadoopFileIO(new Configuration())) {
SessionCatalog.SessionContext sessionContext = SessionCatalog.SessionContext.createEmpty();
Namespace ns = Namespace.of("db1");
sessionCatalog.createNamespace(sessionContext, ns);
TableIdentifier tableIdentifier = TableIdentifier.of(ns, "the_table");
String location =
"file://"
+ testDir.toFile().getAbsolutePath()
+ "/testIcebergRegisterTableInExternalCatalog";
baseLocation.resolve("testIcebergRegisterTableInExternalCatalog").toString();
String metadataLocation = location + "/metadata/000001-494949494949494949.metadata.json";

TableMetadata tableMetadata =
Expand Down Expand Up @@ -489,19 +488,16 @@ public void testIcebergUpdateTableInExternalCatalog() throws IOException {
Catalog.TypeEnum.EXTERNAL,
principalRoleName,
FileStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.FILE)
.setAllowedLocations(List.of("file://" + testDir.toFile().getAbsolutePath()))
.setAllowedLocations(List.of(baseLocation.toString()))
.build(),
"file://" + testDir.toFile().getAbsolutePath());
baseLocation.toString());
try (RESTSessionCatalog sessionCatalog = newSessionCatalog(catalogName);
HadoopFileIO fileIo = new HadoopFileIO(new Configuration())) {
SessionCatalog.SessionContext sessionContext = SessionCatalog.SessionContext.createEmpty();
Namespace ns = Namespace.of("db1");
sessionCatalog.createNamespace(sessionContext, ns);
TableIdentifier tableIdentifier = TableIdentifier.of(ns, "the_table");
String location =
"file://"
+ testDir.toFile().getAbsolutePath()
+ "/testIcebergUpdateTableInExternalCatalog";
String location = baseLocation.resolve("testIcebergUpdateTableInExternalCatalog").toString();
String metadataLocation = location + "/metadata/000001-494949494949494949.metadata.json";

Types.NestedField col1 = Types.NestedField.of(1, false, "col1", Types.StringType.get());
Expand Down Expand Up @@ -541,20 +537,16 @@ public void testIcebergDropTableInExternalCatalog() throws IOException {
Catalog.TypeEnum.EXTERNAL,
principalRoleName,
FileStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.FILE)
.setAllowedLocations(List.of("file://" + testDir.toFile().getAbsolutePath()))
.setAllowedLocations(List.of(baseLocation.toString()))
.build(),
"file://" + testDir.toFile().getAbsolutePath());
baseLocation.toString());
try (RESTSessionCatalog sessionCatalog = newSessionCatalog(catalogName);
HadoopFileIO fileIo = new HadoopFileIO(new Configuration())) {
SessionCatalog.SessionContext sessionContext = SessionCatalog.SessionContext.createEmpty();
Namespace ns = Namespace.of("db1");
sessionCatalog.createNamespace(sessionContext, ns);
TableIdentifier tableIdentifier = TableIdentifier.of(ns, "the_table");
String location =
"file://"
+ testDir.toFile().getAbsolutePath()
+ "/"
+ "testIcebergDropTableInExternalCatalog";
String location = baseLocation.resolve("testIcebergDropTableInExternalCatalog").toString();
String metadataLocation = location + "/metadata/000001-494949494949494949.metadata.json";

TableMetadata tableMetadata =
Expand Down
44 changes: 31 additions & 13 deletions ...s/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.reflect.Method;
import java.net.URI;
import java.nio.file.Path;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -92,6 +94,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;

/**
* Import the full core Iceberg catalog tests by hitting the REST service via the RESTCatalog
Expand All @@ -108,9 +111,9 @@ public class PolarisRestCatalogIntegrationTest extends CatalogTests<RESTCatalog>
private static final String TEST_ROLE_ARN =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_ROLE_ARN"))
.orElse("arn:aws:iam::123456789012:role/my-role");
private static final String S3_BUCKET_BASE =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_S3_PATH"))
.orElse("file:///tmp/buckets/my-bucket");

private static URI s3BucketBase;
private static URI externalCatalogBase;

protected static final String VIEW_QUERY = "select * from ns1.layer1_table";
private static String principalRoleName;
Expand All @@ -125,7 +128,7 @@ public class PolarisRestCatalogIntegrationTest extends CatalogTests<RESTCatalog>
private String currentCatalogName;

private final String catalogBaseLocation =
S3_BUCKET_BASE + "/" + System.getenv("USER") + "/path/to/data";
s3BucketBase + "/" + System.getenv("USER") + "/path/to/data";

private static final String[] DEFAULT_CATALOG_PROPERTIES = {
"allow.unstructured.table.location", "true",
Expand All @@ -148,7 +151,8 @@ String[] properties() default {
}

@BeforeAll
static void setup(PolarisApiEndpoints apiEndpoints, ClientCredentials credentials) {
static void setup(
PolarisApiEndpoints apiEndpoints, ClientCredentials credentials, @TempDir Path tempDir) {
adminCredentials = credentials;
endpoints = apiEndpoints;
client = polarisClient(endpoints);
Expand All @@ -157,6 +161,20 @@ static void setup(PolarisApiEndpoints apiEndpoints, ClientCredentials credential
principalRoleName = client.newEntityName("rest-admin");
principalCredentials = managementApi.createPrincipalWithRole(principalName, principalRoleName);
catalogApi = client.catalogApi(principalCredentials);
URI testRootUri =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_TEMP_DIR"))
.map(URI::create)
.orElse(tempDir.toUri());
s3BucketBase =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_S3_PATH"))
Copy link
Contributor Author

@adutra adutra Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could get rid of this environment variable and rely solely on INTEGRATION_TEST_TEMP_DIR, but I wanted this change to remain backwards-compatible.

Copy link
Contributor

@HonahX HonahX Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to keep backward compatibility here? Is this environment variable expected to be used in CI pipelines, or is it mainly for local debugging? If it's mostly for local use, consolidating into a single variable might make things simpler and help avoid confusion. WDYT?

Copy link
Contributor Author

@adutra adutra Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let's consolidate then 👍

.map(URI::create)
.orElse(testRootUri)
.resolve("my-bucket");
externalCatalogBase =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_S3_PATH"))
.map(URI::create)
.orElse(testRootUri)
.resolve("external-catalog");
}

@AfterAll
Expand Down Expand Up @@ -192,7 +210,7 @@ public void before(TestInfo testInfo) {
for (int i = 0; i < properties.length; i += 2) {
catalogPropsBuilder.addProperty(properties[i], properties[i + 1]);
}
if (!S3_BUCKET_BASE.startsWith("file:/")) {
if (!s3BucketBase.getScheme().equals("file")) {
catalogPropsBuilder.addProperty(
CatalogEntity.REPLACE_NEW_LOCATION_PREFIX_WITH_CATALOG_DEFAULT_KEY, "file:");
}
Expand All @@ -202,7 +220,7 @@ public void before(TestInfo testInfo) {
.setName(currentCatalogName)
.setProperties(catalogPropsBuilder.build())
.setStorageConfigInfo(
S3_BUCKET_BASE.startsWith("file:/")
s3BucketBase.getScheme().equals("file")
? new FileStorageConfigInfo(
StorageConfigInfo.StorageTypeEnum.FILE, List.of("file://"))
: awsConfigModel)
Expand Down Expand Up @@ -541,12 +559,12 @@ public void testLoadTableWithAccessDelegationForExternalCatalogWithConfigDisable
TableMetadata.newTableMetadata(
new Schema(List.of(Types.NestedField.of(1, false, "col1", new Types.StringType()))),
PartitionSpec.unpartitioned(),
"file:///tmp/ns1/my_table",
externalCatalogBase + "/ns1/my_table",
Map.of());
try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) {
resolvingFileIO.initialize(Map.of());
resolvingFileIO.setConf(new Configuration());
String fileLocation = "file:///tmp/ns1/my_table/metadata/v1.metadata.json";
String fileLocation = externalCatalogBase + "/ns1/my_table/metadata/v1.metadata.json";
TableMetadataParser.write(tableMetadata, resolvingFileIO.newOutputFile(fileLocation));
restCatalog.registerTable(TableIdentifier.of(ns1, "my_table"), fileLocation);
try {
Expand Down Expand Up @@ -576,12 +594,12 @@ public void testLoadTableWithoutAccessDelegationForExternalCatalogWithConfigDisa
TableMetadata.newTableMetadata(
new Schema(List.of(Types.NestedField.of(1, false, "col1", new Types.StringType()))),
PartitionSpec.unpartitioned(),
"file:///tmp/ns1/my_table",
externalCatalogBase + "/ns1/my_table",
Map.of());
try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) {
resolvingFileIO.initialize(Map.of());
resolvingFileIO.setConf(new Configuration());
String fileLocation = "file:///tmp/ns1/my_table/metadata/v1.metadata.json";
String fileLocation = externalCatalogBase + "/ns1/my_table/metadata/v1.metadata.json";
TableMetadataParser.write(tableMetadata, resolvingFileIO.newOutputFile(fileLocation));
restCatalog.registerTable(TableIdentifier.of(ns1, "my_table"), fileLocation);
try {
Expand Down Expand Up @@ -610,12 +628,12 @@ public void testLoadTableWithAccessDelegationForExternalCatalogWithConfigEnabled
TableMetadata.newTableMetadata(
new Schema(List.of(Types.NestedField.of(1, false, "col1", new Types.StringType()))),
PartitionSpec.unpartitioned(),
"file:///tmp/ns1/my_table",
externalCatalogBase + "/ns1/my_table",
Map.of());
try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) {
resolvingFileIO.initialize(Map.of());
resolvingFileIO.setConf(new Configuration());
String fileLocation = "file:///tmp/ns1/my_table/metadata/v1.metadata.json";
String fileLocation = externalCatalogBase + "/ns1/my_table/metadata/v1.metadata.json";
TableMetadataParser.write(tableMetadata, resolvingFileIO.newOutputFile(fileLocation));
restCatalog.registerTable(TableIdentifier.of(ns1, "my_table"), fileLocation);
try {
Expand Down
13 changes: 11 additions & 2 deletions ...n-tests/src/main/java/org/apache/polaris/service/it/test/PolarisSparkIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@
import jakarta.ws.rs.client.Entity;
import jakarta.ws.rs.core.Response;
import java.io.IOException;
import java.net.URI;
import java.nio.file.Path;
import java.time.Instant;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.apache.iceberg.rest.requests.ImmutableRegisterTableRequest;
import org.apache.iceberg.rest.responses.LoadTableResponse;
import org.apache.polaris.core.admin.model.AwsStorageConfigInfo;
Expand Down Expand Up @@ -82,7 +84,7 @@ public class PolarisSparkIntegrationTest {
private String catalogName;
private String externalCatalogName;

@TempDir public Path warehouseDir;
private URI warehouseDir;

@BeforeAll
public static void setup() throws IOException {
Expand All @@ -95,13 +97,20 @@ public static void cleanup() {
}

@BeforeEach
public void before(PolarisApiEndpoints apiEndpoints, ClientCredentials credentials) {
public void before(
PolarisApiEndpoints apiEndpoints, ClientCredentials credentials, @TempDir Path tempDir) {
endpoints = apiEndpoints;
client = polarisClient(endpoints);
sparkToken = client.obtainToken(credentials);
managementApi = client.managementApi(credentials);
catalogApi = client.catalogApi(credentials);

warehouseDir =
Optional.ofNullable(System.getenv("INTEGRATION_TEST_TEMP_DIR"))
.map(URI::create)
.orElse(tempDir.toUri())
.resolve("spark-warehouse");

catalogName = client.newEntityName("spark_catalog");
externalCatalogName = client.newEntityName("spark_ext_catalog");

Expand Down