Skip to content

Remove redundant code #52

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
merged 2 commits into from
Aug 6, 2024
Merged

Remove redundant code #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70f8ca4
Remove redundant code
ebyhr Aug 1, 2024
09142e7
Merge branch 'main' into ebi/remove-unused-code2
takidau Aug 6, 2024
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
1 change: 0 additions & 1 deletion ...-core/src/main/java/io/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ public EnumMap<PolarisCredentialProperty, String> getSubscopedCreds(
validateAccountAndContainer(location, allowedReadLocations, allowedWriteLocations);

String storageDnsName = location.getStorageAccount() + "." + location.getEndpoint();
String endpoint = "https://" + storageDnsName;
String filePath = location.getFilePath();

BlobSasPermission blobSasPermission = new BlobSasPermission();
Expand Down
10 changes: 0 additions & 10 deletions ...re/src/test/java/io/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,6 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() {
String externalId = "externalId";
String bucket = "bucket";
String warehouseKeyPrefix = "path/to/warehouse";
String firstPath = warehouseKeyPrefix + "/namespace/table";
String secondPath = warehouseKeyPrefix + "/oldnamespace/table";
Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class)))
.thenAnswer(
invocation -> {
Expand Down Expand Up @@ -449,14 +447,6 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() {
return bucketArn + "/" + keyPrefix + "/*";
}

private static @NotNull String s3CnArn(String bucket, String keyPrefix) {
String bucketArn = "arn:aws-cn:s3:::" + bucket;
if (keyPrefix == null) {
return bucketArn;
}
return bucketArn + "/" + keyPrefix + "/*";
}

private static @NotNull String s3Path(
String bucket, String keyPrefix, PolarisStorageConfigurationInfo.StorageType storageType) {
return storageType.getPrefix() + bucket + "/" + keyPrefix;
Expand Down
18 changes: 0 additions & 18 deletions polaris-service/src/main/java/io/polaris/service/auth/OAuthUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,29 +16,11 @@
package io.polaris.service.auth;

import jakarta.ws.rs.core.Response;
import java.nio.charset.StandardCharsets;
import org.apache.commons.codec.binary.Base64;

/** Simple utility class to assist with OAuth operations */
public class OAuthUtils {

public static final String AUTHORIZATION_HEADER = "Authorization";

public static final String SF_HEADER_ACCOUNT_NAME = "Snowflake-Account";
Copy link
Contributor

@takidau takidau Aug 1, 2024

Choose a reason for hiding this comment

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

This looks like a Snowflake specific string that arguably doesn't need to live here any more, but first I'd like to verify nothing we have internally depends on it before removing it. Let me check on that.

Copy link
Contributor

@takidau takidau Aug 1, 2024

Choose a reason for hiding this comment

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

Confirmed, we still use these internally. Let me try to quickly remove our internal dependency on these and then we can move forward here.

Copy link
Contributor

@takidau takidau Aug 6, 2024

Choose a reason for hiding this comment

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

Sorry for the delay. This is all cleaned up on our side now.


public static final String POLARIS_ROLE_PREFIX = "PRINCIPAL_ROLE:";

public static final String SF_ACCOUNT_NAME_HEADER = "sf-account";
public static final String SF_ACCOUNT_URL_HEADER = "sf-account-url";

/**
* @return basic Authorization Header of the form `base64_encode(client_id:client_secret)
*/
public static String getBasicAuthHeader(String clientId, String clientSecret) {
return Base64.encodeBase64String(
(clientId + ":" + clientSecret).getBytes(StandardCharsets.UTF_8));
}

public static Response getResponseFromError(OAuthTokenErrorResponse.Error error) {
return switch (error) {
case unauthorized_client ->
Expand Down
58 changes: 21 additions & 37 deletions polaris-service/src/main/java/io/polaris/service/catalog/BasePolarisCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ public class BasePolarisCatalog extends BaseMetastoreViewCatalog
private static final Logger LOG = LoggerFactory.getLogger(BasePolarisCatalog.class);

private static final Joiner SLASH = Joiner.on("/");
private static final Joiner DOT = Joiner.on(".");

// Config key for whether to allow setting the FILE_IO_IMPL using catalog properties. Should
// only be allowed in dev/test environments.
Expand Down Expand Up @@ -351,8 +350,7 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) {
})
.orElse(Map.of());
PolarisMetaStoreManager.DropEntityResult dropEntityResult =
dropTableLike(
catalogId, PolarisEntitySubType.TABLE, tableIdentifier, storageProperties, purge);
dropTableLike(PolarisEntitySubType.TABLE, tableIdentifier, storageProperties, purge);
if (!dropEntityResult.isSuccess()) {
return false;
}
Expand All @@ -376,7 +374,7 @@ public List<TableIdentifier> listTables(Namespace namespace) {
"Cannot list tables for namespace. Namespace does not exist: %s", namespace);
}

return listTableLike(catalogId, PolarisEntitySubType.TABLE, namespace);
return listTableLike(PolarisEntitySubType.TABLE, namespace);
}

@Override
Expand All @@ -385,7 +383,7 @@ public void renameTable(TableIdentifier from, TableIdentifier to) {
return;
}

renameTableLike(catalogId, PolarisEntitySubType.TABLE, from, to);
renameTableLike(PolarisEntitySubType.TABLE, from, to);
}

@Override
Expand Down Expand Up @@ -700,7 +698,7 @@ public List<TableIdentifier> listViews(Namespace namespace) {
"Cannot list views for namespace. Namespace does not exist: %s", namespace);
}

return listTableLike(catalogId, PolarisEntitySubType.VIEW, namespace);
return listTableLike(PolarisEntitySubType.VIEW, namespace);
}

@Override
Expand All @@ -710,8 +708,7 @@ protected BasePolarisViewOperations newViewOps(TableIdentifier identifier) {

@Override
public boolean dropView(TableIdentifier identifier) {
return dropTableLike(catalogId, PolarisEntitySubType.VIEW, identifier, Map.of(), true)
.isSuccess();
return dropTableLike(PolarisEntitySubType.VIEW, identifier, Map.of(), true).isSuccess();
}

@Override
Expand All @@ -720,14 +717,14 @@ public void renameView(TableIdentifier from, TableIdentifier to) {
return;
}

renameTableLike(catalogId, PolarisEntitySubType.VIEW, from, to);
renameTableLike(PolarisEntitySubType.VIEW, from, to);
}

@Override
public boolean sendNotification(
TableIdentifier identifier, NotificationRequest notificationRequest) {
return sendNotificationForTableLike(
catalogId, PolarisEntitySubType.TABLE, identifier, notificationRequest);
PolarisEntitySubType.TABLE, identifier, notificationRequest);
}

@Override
Expand Down Expand Up @@ -1060,11 +1057,9 @@ private void validateNoLocationOverlap(

private class BasePolarisCatalogTableBuilder
extends BaseMetastoreViewCatalog.BaseMetastoreViewCatalogTableBuilder {
private final TableIdentifier identifier;

public BasePolarisCatalogTableBuilder(TableIdentifier identifier, Schema schema) {
super(identifier, schema);
this.identifier = identifier;
}

@Override
Expand All @@ -1074,11 +1069,9 @@ public TableBuilder withLocation(String newLocation) {
}

private class BasePolarisCatalogViewBuilder extends BaseMetastoreViewCatalog.BaseViewBuilder {
private final TableIdentifier identifier;

public BasePolarisCatalogViewBuilder(TableIdentifier identifier) {
super(identifier);
this.identifier = identifier;
}

@Override
Expand Down Expand Up @@ -1250,9 +1243,9 @@ public void doCommit(TableMetadata base, TableMetadata metadata) {
tableIdentifier, oldLocation, newLocation, existingLocation);
}
if (null == existingLocation) {
createTableLike(catalogId, tableIdentifier, entity);
createTableLike(tableIdentifier, entity);
} else {
updateTableLike(catalogId, tableIdentifier, entity);
updateTableLike(tableIdentifier, entity);
}
}

Expand Down Expand Up @@ -1462,9 +1455,9 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) {
identifier, oldLocation, newLocation, existingLocation);
}
if (null == existingLocation) {
createTableLike(catalogId, identifier, entity);
createTableLike(identifier, entity);
} else {
updateTableLike(catalogId, identifier, entity);
updateTableLike(identifier, entity);
}
}

Expand Down Expand Up @@ -1525,7 +1518,7 @@ long getCatalogId() {
}

private void renameTableLike(
long catalogId, PolarisEntitySubType subType, TableIdentifier from, TableIdentifier to) {
PolarisEntitySubType subType, TableIdentifier from, TableIdentifier to) {
LOG.debug("Renaming tableLike from {} to {}", from, to);
PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getResolvedPath(from, subType);
if (resolvedEntities == null) {
Expand Down Expand Up @@ -1640,7 +1633,7 @@ private void renameTableLike(
* duplicate the logic to try to resolve parentIds before constructing the proposed entity. This
* method will fill in the parentId if needed upon resolution.
*/
private void createTableLike(long catalogId, TableIdentifier identifier, PolarisEntity entity) {
private void createTableLike(TableIdentifier identifier, PolarisEntity entity) {
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getResolvedPath(identifier.namespace());
if (resolvedParent == null) {
Expand All @@ -1649,14 +1642,11 @@ private void createTableLike(long catalogId, TableIdentifier identifier, Polaris
String.format("Failed to fetch resolved parent for TableIdentifier '%s'", identifier));
}

createTableLike(catalogId, identifier, entity, resolvedParent);
createTableLike(identifier, entity, resolvedParent);
}

private void createTableLike(
long catalogId,
TableIdentifier identifier,
PolarisEntity entity,
PolarisResolvedPathWrapper resolvedParent) {
TableIdentifier identifier, PolarisEntity entity, PolarisResolvedPathWrapper resolvedParent) {
// Make sure the metadata file is valid for our allowed locations.
String metadataLocation = TableLikeEntity.of(entity).getMetadataLocation();
validateLocationForTableLike(identifier, metadataLocation, resolvedParent);
Expand Down Expand Up @@ -1689,7 +1679,7 @@ private static boolean isUnderParentLocation(URI childLocation, URI expectedPare
return !expectedParentLocation.relativize(childLocation).equals(childLocation);
}

private void updateTableLike(long catalogId, TableIdentifier identifier, PolarisEntity entity) {
private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) {
PolarisResolvedPathWrapper resolvedEntities =
resolvedEntityView.getResolvedPath(identifier, entity.getSubType());
if (resolvedEntities == null) {
Expand Down Expand Up @@ -1718,7 +1708,6 @@ private void updateTableLike(long catalogId, TableIdentifier identifier, Polaris
}

private @NotNull PolarisMetaStoreManager.DropEntityResult dropTableLike(
long catalogId,
PolarisEntitySubType subType,
TableIdentifier identifier,
Map<String, String> storageProperties,
Expand All @@ -1744,10 +1733,7 @@ private void updateTableLike(long catalogId, TableIdentifier identifier, Polaris
}

private boolean sendNotificationForTableLike(
long catalogId,
PolarisEntitySubType subType,
TableIdentifier tableIdentifier,
NotificationRequest request) {
PolarisEntitySubType subType, TableIdentifier tableIdentifier, NotificationRequest request) {
LOG.debug("Handling notification request {} for tableIdentifier {}", request, tableIdentifier);
PolarisResolvedPathWrapper resolvedEntities =
resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, subType);
Expand All @@ -1757,8 +1743,7 @@ private boolean sendNotificationForTableLike(
Preconditions.checkNotNull(notificationType, "Expected a valid notification type.");

if (notificationType == NotificationType.DROP) {
return dropTableLike(
catalogId, PolarisEntitySubType.TABLE, tableIdentifier, Map.of(), false /* purge */)
return dropTableLike(PolarisEntitySubType.TABLE, tableIdentifier, Map.of(), false /* purge */)
.isSuccess();
} else if (notificationType == NotificationType.CREATE
|| notificationType == NotificationType.UPDATE) {
Expand Down Expand Up @@ -1817,14 +1802,14 @@ private boolean sendNotificationForTableLike(
"Creating table {} for notification with metadataLocation {}",
tableIdentifier,
newLocation);
createTableLike(catalogId, tableIdentifier, entity, resolvedParent);
createTableLike(tableIdentifier, entity, resolvedParent);
} else {
LOG.debug(
"Updating table {} for notification with metadataLocation {}",
tableIdentifier,
newLocation);

updateTableLike(catalogId, tableIdentifier, entity);
updateTableLike(tableIdentifier, entity);
}
}
return true;
Expand All @@ -1848,8 +1833,7 @@ private void createNonExistingNamespaces(Namespace namespace) {
}
}

private List<TableIdentifier> listTableLike(
long catalogId, PolarisEntitySubType subType, Namespace namespace) {
private List<TableIdentifier> listTableLike(PolarisEntitySubType subType, Namespace namespace) {
PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getResolvedPath(namespace);
if (resolvedEntities == null) {
// Illegal state because the namespace should've already been in the static resolution set.
Expand Down
3 changes: 0 additions & 3 deletions polaris-service/src/main/java/io/polaris/service/catalog/IcebergCatalogAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@
import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest;
import org.apache.iceberg.rest.requests.UpdateTableRequest;
import org.apache.iceberg.rest.responses.ConfigResponse;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* {@link IcebergRestCatalogApiService} implementation that delegates operations to {@link
Expand All @@ -70,7 +68,6 @@
*/
public class IcebergCatalogAdapter
implements IcebergRestCatalogApiService, IcebergRestConfigurationApiService {
private static final Logger LOG = LoggerFactory.getLogger(IcebergCatalogAdapter.class);

private final CallContextCatalogFactory catalogFactory;
private final RealmEntityManagerFactory entityManagerFactory;
Expand Down
4 changes: 2 additions & 2 deletions polaris-service/src/main/java/io/polaris/service/task/ManifestFileCleanupTaskHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ private CompletableFuture<Void> tryDelete(
// file's existence, but then it is deleted before we have a chance to
// send the delete request. In such a case, we <i>should</i> retry
// and find
if (TaskUtils.exists(dataFile.toString(), fileIO)) {
fileIO.deleteFile(dataFile.toString());
if (TaskUtils.exists(dataFile, fileIO)) {
fileIO.deleteFile(dataFile);
} else {
LOGGER
.atInfo()
Expand Down
3 changes: 0 additions & 3 deletions polaris-service/src/main/java/io/polaris/service/task/TableCleanupTaskHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
import io.polaris.core.persistence.PolarisMetaStoreManager;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.apache.iceberg.ManifestFile;
Expand All @@ -48,7 +46,6 @@ public class TableCleanupTaskHandler implements TaskHandler {
private final TaskExecutor taskExecutor;
private final MetaStoreManagerFactory metaStoreManagerFactory;
private final Function<TaskEntity, FileIO> fileIOSupplier;
private final ExecutorService executorService = Executors.newVirtualThreadPerTaskExecutor();

public TableCleanupTaskHandler(
TaskExecutor taskExecutor,
Expand Down
14 changes: 6 additions & 8 deletions polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,12 +642,11 @@ public void testUpdateNotificationCreateTableWithHttpPrefix() {
supportsNotifications(), "Only applicable if notifications are supported");

String catalogName = "catalogForMaliciousDomain";
PolarisEntity catalogEntity =
adminService.createCatalog(
new CatalogEntity.Builder()
.setDefaultBaseLocation("http://maliciousdomain.com")
.setName(catalogName)
.build());
adminService.createCatalog(
new CatalogEntity.Builder()
.setDefaultBaseLocation("http://maliciousdomain.com")
.setName(catalogName)
.build());

CallContext callContext = CallContext.getCurrentContext();
PolarisPassthroughResolutionView passthroughView =
Expand Down Expand Up @@ -1119,8 +1118,7 @@ private TableMetadata createSampleTableMetadata(String tableLocation) {
PartitionSpec partitionSpec =
PartitionSpec.builderFor(schema).identity("intType").withSpecId(1000).build();

return TableMetadata.newTableMetadata(
schema, partitionSpec, tableLocation, ImmutableMap.<String, String>of());
return TableMetadata.newTableMetadata(schema, partitionSpec, tableLocation, ImmutableMap.of());
}

private void createNonExistingNamespaces(Namespace namespace) {
Expand Down
23 changes: 11 additions & 12 deletions polaris-service/src/test/java/io/polaris/service/catalog/BasePolarisCatalogViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,18 +108,17 @@ public void before() {
entityManager,
authenticatedRoot,
new PolarisAuthorizer(new PolarisConfigurationStore() {}));
PolarisEntity catalogEntity =
adminService.createCatalog(
new CatalogEntity.Builder()
.setName(CATALOG_NAME)
.addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "true")
.addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true")
.setDefaultBaseLocation("file://tmp")
.setStorageConfigurationInfo(
new FileStorageConfigInfo(
StorageConfigInfo.StorageTypeEnum.FILE, List.of("file://", "/", "*")),
"file://tmp")
.build());
adminService.createCatalog(
new CatalogEntity.Builder()
.setName(CATALOG_NAME)
.addProperty(PolarisConfiguration.CATALOG_ALLOW_EXTERNAL_TABLE_LOCATION, "true")
.addProperty(PolarisConfiguration.CATALOG_ALLOW_UNSTRUCTURED_TABLE_LOCATION, "true")
.setDefaultBaseLocation("file://tmp")
.setStorageConfigurationInfo(
new FileStorageConfigInfo(
StorageConfigInfo.StorageTypeEnum.FILE, List.of("file://", "/", "*")),
"file://tmp")
.build());

PolarisPassthroughResolutionView passthroughView =
new PolarisPassthroughResolutionView(
Expand Down