Samrat002 commented on code in PR #206: URL: https://github.com/apache/flink-connector-aws/pull/206#discussion_r4037631798
########## flink-catalog-aws/flink-catalog-aws-glue/.idea/jarRepositories.xml: ########## @@ -0,0 +1,25 @@ +<?xml version="1.0" encoding="UTF-8"?> Review Comment: why .idea folder exists ? ########## flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/constants/AWSGlueConfigConstants.java: ########## @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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. + */ + +package org.apache.flink.table.catalog.glue.constants; + +import org.apache.flink.annotation.PublicEvolving; + +/** Configuration keys for AWS Glue Data Catalog service usage. */ +@PublicEvolving +public class AWSGlueConfigConstants { + + /** + * Configure an alternative endpoint of the Glue service for GlueCatalog to access. + * + * <p>This could be used to use GlueCatalog with any glue-compatible metastore service that has Review Comment: too much java doc . keep it simple and to the point ########## flink-catalog-aws/flink-catalog-aws-glue/src/test/resources/archunit.properties: ########## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. +# + +# By default we allow removing existing violations, but fail when new violations are added. +freeze.store.default.allowStoreUpdate=true Review Comment: why this file is required ? ########## flink-catalog-aws/flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/GlueCatalogSqlMotoITCase.java: ########## @@ -0,0 +1,254 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ + +package org.apache.flink.table.catalog.glue; + +import org.apache.flink.table.api.EnvironmentSettings; +import org.apache.flink.table.api.TableEnvironment; +import org.apache.flink.test.junit5.MiniClusterExtension; +import org.apache.flink.types.Row; +import org.apache.flink.util.CollectionUtil; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.junit.jupiter.Container; +import org.testcontainers.junit.jupiter.Testcontainers; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.model.Table; + +import java.net.URI; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.tuple; + +/** + * SQL-path integration test for the Glue catalog against a moto Glue emulator. + * + * <p>Unlike {@link GlueCatalogMotoITCase}, which drives {@link GlueCatalog} methods directly, this + * test exercises the full user path: {@code CREATE CATALOG ... WITH ('type'='glue')} discovers + * {@code GlueCatalogFactory} via SPI, the factory builds its own {@code GlueClient}, and all + * catalog operations flow through Flink SQL DDL and the planner down to the Glue wire protocol. + * + * <p>The factory-built client is pointed at moto through the catalog's {@code aws.endpoint} option + * (handled by the shared AWS client-creation path) and system-property credentials, so this test + * also covers the factory's AWS option pass-through. + */ +@Testcontainers +@ExtendWith(MiniClusterExtension.class) +class GlueCatalogSqlMotoITCase { + + private static final int MOTO_PORT = 5000; + + @Container + private static final GenericContainer<?> MOTO = Review Comment: What is moto server ? why sudden dependency added to flink-connector-aws ? ########## flink-catalog-aws/flink-catalog-aws-glue/archunit-violations/stored.rules: ########## @@ -0,0 +1,4 @@ +# +#Thu Aug 11 14:04:41 CEST 2022 Review Comment: why this ile is required ? ########## flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/factory/GlueCatalogFactory.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ + +package org.apache.flink.table.catalog.glue.factory; + +import org.apache.flink.configuration.ConfigOption; +import org.apache.flink.configuration.ConfigOptions; +import org.apache.flink.table.catalog.Catalog; +import org.apache.flink.table.catalog.exceptions.CatalogException; +import org.apache.flink.table.catalog.glue.GlueCatalog; +import org.apache.flink.table.factories.CatalogFactory; + +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +/** Factory for creating GlueCatalog instances. */ +public class GlueCatalogFactory implements CatalogFactory { + + // Define configuration options that users must provide + public static final ConfigOption<String> REGION = + ConfigOptions.key("region") + .stringType() + .noDefaultValue() + .withDescription("AWS region for the Glue catalog"); + + public static final ConfigOption<String> DEFAULT_DATABASE = + ConfigOptions.key("default-database") + .stringType() + .defaultValue("default") + .withDescription("Default database to use in Glue catalog"); + + @Override + public String factoryIdentifier() { + return "glue"; + } + + @Override + public Set<ConfigOption<?>> requiredOptions() { + Set<ConfigOption<?>> options = new HashSet<>(); + options.add(REGION); Review Comment: only region is required field ? ########## flink-catalog-aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/factory/GlueCatalogFactory.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ + +package org.apache.flink.table.catalog.glue.factory; + +import org.apache.flink.configuration.ConfigOption; +import org.apache.flink.configuration.ConfigOptions; +import org.apache.flink.table.catalog.Catalog; +import org.apache.flink.table.catalog.exceptions.CatalogException; +import org.apache.flink.table.catalog.glue.GlueCatalog; +import org.apache.flink.table.factories.CatalogFactory; + +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +/** Factory for creating GlueCatalog instances. */ +public class GlueCatalogFactory implements CatalogFactory { + + // Define configuration options that users must provide + public static final ConfigOption<String> REGION = + ConfigOptions.key("region") + .stringType() + .noDefaultValue() + .withDescription("AWS region for the Glue catalog"); + + public static final ConfigOption<String> DEFAULT_DATABASE = + ConfigOptions.key("default-database") + .stringType() + .defaultValue("default") + .withDescription("Default database to use in Glue catalog"); + + @Override + public String factoryIdentifier() { + return "glue"; + } + + @Override + public Set<ConfigOption<?>> requiredOptions() { + Set<ConfigOption<?>> options = new HashSet<>(); + options.add(REGION); + return options; + } + + @Override + public Set<ConfigOption<?>> optionalOptions() { + Set<ConfigOption<?>> options = new HashSet<>(); + options.add(DEFAULT_DATABASE); + return options; + } + + @Override + public Catalog createCatalog(Context context) { + Map<String, String> config = context.getOptions(); + String name = context.getName(); + String region = config.get(REGION.key()); + String defaultDatabase = + config.getOrDefault(DEFAULT_DATABASE.key(), DEFAULT_DATABASE.defaultValue()); + + if (region == null || region.isEmpty()) { + throw new CatalogException( + "The 'region' property must be specified for the Glue catalog."); + } Review Comment: region is marked as a required field. This condition becomes redundant -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
