Skip to content

Commit

Permalink
AWS: Remove unused GlueCatalog.fileIO field (apache#8068)
Browse files Browse the repository at this point in the history
  • Loading branch information
findepi authored Aug 3, 2023
1 parent 42db993 commit da6e611
Show file tree
Hide file tree
Showing 5 changed files with 6 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import org.apache.iceberg.aws.AwsClientFactory;
import org.apache.iceberg.aws.AwsIntegTestUtil;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.aws.s3.S3FileIO;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -74,27 +73,19 @@ public class GlueTestBase {
"s3://" + testBucketName + "/writeFolderStorageLoc");

static final String testBucketPath = "s3://" + testBucketName + "/" + testPathPrefix;
static final S3FileIO fileIO = new S3FileIO(clientFactory::s3);

@BeforeClass
public static void beforeClass() {
glueCatalog = new GlueCatalog();
AwsProperties properties = new AwsProperties();
properties.setS3FileIoDeleteBatchSize(10);
glueCatalog.initialize(
catalogName, testBucketPath, properties, glue, null, fileIO, ImmutableMap.of());
glueCatalog.initialize(catalogName, testBucketPath, properties, glue, null, ImmutableMap.of());

glueCatalogWithSkipNameValidation = new GlueCatalog();
AwsProperties propertiesSkipNameValidation = new AwsProperties();
propertiesSkipNameValidation.setGlueCatalogSkipNameValidation(true);
glueCatalogWithSkipNameValidation.initialize(
catalogName,
testBucketPath,
propertiesSkipNameValidation,
glue,
null,
fileIO,
ImmutableMap.of());
catalogName, testBucketPath, propertiesSkipNameValidation, glue, null, ImmutableMap.of());
}

@AfterClass
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.apache.iceberg.Table;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.aws.dynamodb.DynamoDbLockManager;
import org.apache.iceberg.aws.s3.S3FileIO;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.util.concurrent.MoreExecutors;
Expand All @@ -55,7 +54,6 @@ public static void beforeClass() {
GlueTestBase.beforeClass();
String testBucketPath = "s3://" + testBucketName + "/" + testPathPrefix;
lockTableName = getRandomName();
S3FileIO fileIO = new S3FileIO(clientFactory::s3);
glueCatalog = new GlueCatalog();
AwsProperties awsProperties = new AwsProperties();
dynamo = clientFactory.dynamo();
Expand All @@ -65,7 +63,6 @@ public static void beforeClass() {
awsProperties,
glue,
new DynamoDbLockManager(dynamo, lockTableName),
fileIO,
ImmutableMap.of());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.apache.iceberg.TableOperations;
import org.apache.iceberg.Transaction;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.aws.s3.S3FileIO;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.AlreadyExistsException;
Expand Down Expand Up @@ -142,7 +141,6 @@ public void testCreateAndLoadTableWithoutWarehouseLocation() {
new AwsProperties(),
glue,
LockManagers.defaultLockManager(),
new S3FileIO(clientFactory::s3),
ImmutableMap.of());
String namespace = createNamespace();
String tableName = getRandomName();
Expand Down Expand Up @@ -384,7 +382,6 @@ public void testCommitTableSkipArchive() {
properties,
glue,
LockManagers.defaultLockManager(),
fileIO,
ImmutableMap.of());
glueCatalog.createTable(TableIdentifier.of(namespace, tableName), schema, partitionSpec);
Table table = glueCatalog.loadTable(TableIdentifier.of(namespace, tableName));
Expand Down Expand Up @@ -597,15 +594,13 @@ public void testRegisterTableAlreadyExists() {
@Test
public void testTableLevelS3Tags() {
String testBucketPath = "s3://" + testBucketName + "/" + testPathPrefix;
S3FileIO fileIO = new S3FileIO(clientFactory::s3);
Map<String, String> properties =
ImmutableMap.of(
AwsProperties.S3_WRITE_TABLE_TAG_ENABLED,
"true",
AwsProperties.S3_WRITE_NAMESPACE_TAG_ENABLED,
"true");
glueCatalog.initialize(
catalogName, testBucketPath, new AwsProperties(properties), glue, null, fileIO);
glueCatalog.initialize(catalogName, testBucketPath, new AwsProperties(properties), glue, null);
String namespace = createNamespace();
String tableName = getRandomName();
createTable(namespace, tableName);
Expand Down
20 changes: 3 additions & 17 deletions aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.hadoop.Configurable;
import org.apache.iceberg.io.CloseableGroup;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -89,7 +88,6 @@ public class GlueCatalog extends BaseMetastoreCatalog
private String catalogName;
private String warehousePath;
private AwsProperties awsProperties;
private FileIO fileIO;
private LockManager lockManager;
private CloseableGroup closeableGroup;
private Map<String, String> catalogProperties;
Expand All @@ -113,7 +111,6 @@ public GlueCatalog() {}
public void initialize(String name, Map<String, String> properties) {
this.catalogProperties = ImmutableMap.copyOf(properties);
AwsClientFactory awsClientFactory;
FileIO catalogFileIO;
if (PropertyUtil.propertyAsBoolean(
properties,
AwsProperties.GLUE_LAKEFORMATION_ENABLED,
Expand All @@ -133,19 +130,16 @@ public void initialize(String name, Map<String, String> properties) {
"Detected LakeFormation enabled for Glue catalog, should use a client factory that extends %s, but found %s",
LakeFormationAwsClientFactory.class.getName(),
factoryImpl);
catalogFileIO = null;
} else {
awsClientFactory = AwsClientFactories.from(properties);
catalogFileIO = GlueTableOperations.initializeFileIO(properties, hadoopConf);
}

initialize(
name,
properties.get(CatalogProperties.WAREHOUSE_LOCATION),
new AwsProperties(properties),
awsClientFactory.glue(),
initializeLockManager(properties),
catalogFileIO);
initializeLockManager(properties));
}

private LockManager initializeLockManager(Map<String, String> properties) {
Expand All @@ -170,32 +164,24 @@ void initialize(
AwsProperties properties,
GlueClient client,
LockManager lock,
FileIO io,
Map<String, String> catalogProps) {
this.catalogProperties = catalogProps;
initialize(name, path, properties, client, lock, io);
initialize(name, path, properties, client, lock);
}

@VisibleForTesting
void initialize(
String name,
String path,
AwsProperties properties,
GlueClient client,
LockManager lock,
FileIO io) {
String name, String path, AwsProperties properties, GlueClient client, LockManager lock) {
this.catalogName = name;
this.awsProperties = properties;
this.warehousePath =
(path != null && path.length() > 0) ? LocationUtil.stripTrailingSlash(path) : null;
this.glue = client;
this.lockManager = lock;
this.fileIO = io;

this.closeableGroup = new CloseableGroup();
closeableGroup.addCloseable(glue);
closeableGroup.addCloseable(lockManager);
closeableGroup.addCloseable(fileIO);
closeableGroup.setSuppressCloseFailure(true);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ public void before() {
new AwsProperties(),
glue,
LockManagers.defaultLockManager(),
null,
ImmutableMap.of());
}

Expand All @@ -94,7 +93,6 @@ public void testConstructorEmptyWarehousePath() {
new AwsProperties(),
glue,
LockManagers.defaultLockManager(),
null,
ImmutableMap.of());
Mockito.doReturn(
GetDatabaseResponse.builder().database(Database.builder().name("db").build()).build())
Expand All @@ -119,7 +117,6 @@ public void testConstructorWarehousePathWithEndSlash() {
new AwsProperties(),
glue,
LockManagers.defaultLockManager(),
null,
ImmutableMap.of());
Mockito.doReturn(
GetDatabaseResponse.builder().database(Database.builder().name("db").build()).build())
Expand Down Expand Up @@ -163,7 +160,6 @@ public void testDefaultWarehouseLocationCustomCatalogId() {
awsProperties,
glue,
LockManagers.defaultLockManager(),
null,
ImmutableMap.of());

Mockito.doReturn(
Expand Down Expand Up @@ -598,7 +594,6 @@ public void testTablePropsDefinedAtCatalogLevel() {
new AwsProperties(),
glue,
LockManagers.defaultLockManager(),
null,
catalogProps);
Map<String, String> properties = glueCatalog.properties();
Assertions.assertThat(properties)
Expand All @@ -620,7 +615,6 @@ public void testValidateIdentifierSkipNameValidation() {
props,
glue,
LockManagers.defaultLockManager(),
null,
ImmutableMap.of());
Assertions.assertThat(glueCatalog.isValidIdentifier(TableIdentifier.parse("db-1.a-1")))
.isEqualTo(true);
Expand All @@ -641,7 +635,6 @@ public void testTableLevelS3TagProperties() {
awsProperties,
glue,
LockManagers.defaultLockManager(),
null,
properties);
GlueTableOperations glueTableOperations =
(GlueTableOperations)
Expand Down

0 comments on commit da6e611

Please sign in to comment.