leonardBang commented on code in PR #22937: URL: https://github.com/apache/flink/pull/22937#discussion_r1257836980
########## flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/CatalogStore.java: ########## @@ -0,0 +1,75 @@ +/* + * 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; + +import org.apache.flink.annotation.PublicEvolving; +import org.apache.flink.table.catalog.exceptions.CatalogException; + +import java.util.Optional; +import java.util.Set; + +/** + * The {@link CatalogStore} is used in {@link CatalogManager} to retrieve, save and remove {@link + * CatalogDescriptor} at the external storage system. + */ +@PublicEvolving +public interface CatalogStore { + + /** + * Store a catalog under the given catalog name. The catalog name must be unique. Review Comment: minor: Stores ########## flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/CatalogStore.java: ########## @@ -0,0 +1,75 @@ +/* + * 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; + +import org.apache.flink.annotation.PublicEvolving; +import org.apache.flink.table.catalog.exceptions.CatalogException; + +import java.util.Optional; +import java.util.Set; + +/** + * The {@link CatalogStore} is used in {@link CatalogManager} to retrieve, save and remove {@link + * CatalogDescriptor} at the external storage system. + */ Review Comment: let's try to define the interface firstly instead of explain it. ```suggestion /** * Represents the storage where persists all {@link Catalog}s. * * <p>All catalogs can be lazy initialized with the {@link CatalogStore}. * * <p>It can be used in {@code CatalogManager} to retrieve, save and remove catalog in {@link * CatalogDescriptor} format at the external storage system. */ ``` ########## flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/CatalogStore.java: ########## @@ -0,0 +1,75 @@ +/* + * 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; + +import org.apache.flink.annotation.PublicEvolving; +import org.apache.flink.table.catalog.exceptions.CatalogException; + +import java.util.Optional; +import java.util.Set; + +/** + * The {@link CatalogStore} is used in {@link CatalogManager} to retrieve, save and remove {@link + * CatalogDescriptor} at the external storage system. + */ +@PublicEvolving +public interface CatalogStore { + + /** + * Store a catalog under the given catalog name. The catalog name must be unique. + * + * @param catalogName the given catalog name under which to store the given catalog + * @param catalog catalog descriptor to store + * @throws CatalogException throw when registration failed + */ + void storeCatalog(String catalogName, CatalogDescriptor catalog) throws CatalogException; + + /** + * Remove a catalog with the given catalog name. The catalog name must be existed. Review Comment: The javadoc` The catalog name must be existed.` is mismatched with parameter `ignoreIfNotExists ` ########## flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/CatalogStoreFactory.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.factories; + +import org.apache.flink.annotation.PublicEvolving; +import org.apache.flink.configuration.ReadableConfig; +import org.apache.flink.table.catalog.CatalogStore; + +import java.util.Map; + +/** + * A factory to create configured catalog store instances based on string-based properties. See also + * {@link Factory} for more information. + */ +@PublicEvolving +public interface CatalogStoreFactory extends Factory { + + /** Creates a {@link CatalogStore} instance from context information. */ + CatalogStore createCatalogStore(Context context); + + /** Initialize the CatalogStoreFactory. */ + void open(Context context); + + /** Close the CatalogStoreFactory. */ + void close(); Review Comment: Why a factory need the Initialization and tear-down methods? Please give more explanation to developers. Maybe we known the reason as the FLIP participates but other developer would confuse a lot. ########## flink-table/flink-table-api-java/src/test/java/org/apache/flink/table/catalog/GenericInMemoryCatalogStoreTest.java: ########## @@ -0,0 +1,45 @@ +/* + * 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; + +import org.apache.flink.configuration.Configuration; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; + +/** Test for {@link GenericInMemoryCatalogStore}. */ +public class GenericInMemoryCatalogStoreTest { + + @Test + void testStoreAndGet() { + CatalogStore catalogStore = new GenericInMemoryCatalogStore(); + catalogStore.open(); + + catalogStore.storeCatalog( + "catalog1", CatalogDescriptor.of("catalog1", new Configuration())); + assertThat(catalogStore.getCatalog("catalog1").isPresent()).isTrue(); + assertThat(catalogStore.contains("catalog1")).isTrue(); + + catalogStore.removeCatalog("catalog1", true); + assertThat(catalogStore.contains("catalog1")).isFalse(); + + catalogStore.close(); Review Comment: Could we use `catalogStore` after it is closed? what's the expected behavior ? -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
