snazy commented on code in PR #2328: URL: https://github.com/apache/polaris/pull/2328#discussion_r2269323475
########## persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/relational/spanner/GoogleCloudSpannerConfiguration.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.polaris.persistence.relational.spanner; + +import java.util.Optional; + +public interface GoogleCloudSpannerConfiguration { + + public Optional<String> projectId(); + + public Optional<String> instanceId(); + + public Optional<String> databaseId(); + + public Optional<String> emulatorHost(); Review Comment: Guess this needs a port as well? ########## persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/relational/spanner/GoogleCloudSpannerDatabaseClientLifecycleManager.java: ########## @@ -0,0 +1,143 @@ +/* + * 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.polaris.persistence.relational.spanner; + +import com.google.cloud.spanner.Database; +import com.google.cloud.spanner.DatabaseAdminClient; +import com.google.cloud.spanner.DatabaseClient; +import com.google.cloud.spanner.DatabaseId; +import com.google.cloud.spanner.Dialect; +import com.google.cloud.spanner.Spanner; +import com.google.cloud.spanner.SpannerException; +import com.google.common.collect.ImmutableList; +import jakarta.annotation.PostConstruct; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.inject.Produces; +import jakarta.inject.Inject; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.Charset; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Consumer; +import java.util.function.Supplier; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.persistence.bootstrap.SchemaOptions; +import org.apache.polaris.persistence.relational.spanner.model.Realm; +import org.apache.polaris.persistence.relational.spanner.util.SpannerUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@ApplicationScoped +public class GoogleCloudSpannerDatabaseClientLifecycleManager { + + private static Logger LOGGER = + LoggerFactory.getLogger(GoogleCloudSpannerDatabaseClientLifecycleManager.class); + + @Inject GoogleCloudSpannerConfiguration spannerConfiguration; + + protected Spanner spanner; + protected DatabaseId databaseId; + + @PostConstruct + protected void init() { + spanner = SpannerUtil.spannerFromConfiguration(spannerConfiguration); + databaseId = SpannerUtil.databaseFromConfiguration(spannerConfiguration); + } + + protected List<String> getSpannerDatabaseDdl(SchemaOptions options) { + final InputStream schemaStream; + if (options.schemaFile() != null) { + try { + schemaStream = new FileInputStream(options.schemaFile()); + } catch (IOException e) { + throw new IllegalArgumentException("Unable to load file " + options.schemaFile(), e); + } + } else { + if (options.schemaVersion() == null || options.schemaVersion() == 1) { + schemaStream = + getClass().getResourceAsStream("/org/apache/polaris/persistence/spanner/schema-v1.sql"); + } else { + throw new IllegalArgumentException("Unknown schema version " + options.schemaVersion()); + } + } + try (schemaStream) { + String schema = new String(schemaStream.readAllBytes(), Charset.forName("UTF-8")); + List<String> lines = new ArrayList<>(); + for (String s : schema.split("\n")) { + s = s.trim(); + if (s.startsWith("--") || s.length() == 0) { + continue; + } + lines.add(s); + } + return List.of(String.join(" ", lines).split(";")); + } catch (IOException e) { + throw new RuntimeException("Unable to retrieve DDL statements", e); + } + } + + @Produces + public Consumer<SchemaOptions> getSchemaInitializer() { Review Comment: Nit: I'd personally introduce a separate interface type that extends `Consumer<X>` (ran into issues with CDI + generics in the past - might no longer be an issue though). ########## persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/relational/spanner/GoogleCloudSpannerConfiguration.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.polaris.persistence.relational.spanner; + +import java.util.Optional; + +public interface GoogleCloudSpannerConfiguration { + + public Optional<String> projectId(); + + public Optional<String> instanceId(); + + public Optional<String> databaseId(); + + public Optional<String> emulatorHost(); Review Comment: BTW: Thanks for adding emulator support for testing right away! ########## persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/relational/spanner/GoogleCloudSpannerConfiguration.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.polaris.persistence.relational.spanner; + +import java.util.Optional; + +public interface GoogleCloudSpannerConfiguration { Review Comment: Doesn't hurt to add the `@ConfigMapping` annotation (from smallrye-config) right away. Might be convenient for testing as well. For testing alone, maybe just add `@PolarisImmutable` (so you don't need any mocking). ########## persistence/google-cloud-spanner/build.gradle.kts: ########## @@ -0,0 +1,42 @@ +/* + * 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. + */ + +plugins { + id("polaris-server") + alias(libs.plugins.jandex) +} + +dependencies { + implementation(project(":polaris-core")) + implementation(libs.slf4j.api) + implementation(libs.guava) + + implementation(platform(libs.google.cloud.libraries.bom)) + implementation("com.google.cloud:google-cloud-spanner") + + compileOnly(libs.jakarta.annotation.api) + compileOnly(libs.jakarta.enterprise.cdi.api) + compileOnly(libs.jakarta.inject.api) + + implementation(libs.smallrye.common.annotation) // @Identifier Review Comment: Nit: ```suggestion compileOnly(libs.smallrye.common.annotation) // @Identifier ``` ########## persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/relational/spanner/GoogleCloudSpannerDatabaseClientLifecycleManager.java: ########## @@ -0,0 +1,143 @@ +/* + * 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.polaris.persistence.relational.spanner; + +import com.google.cloud.spanner.Database; +import com.google.cloud.spanner.DatabaseAdminClient; +import com.google.cloud.spanner.DatabaseClient; +import com.google.cloud.spanner.DatabaseId; +import com.google.cloud.spanner.Dialect; +import com.google.cloud.spanner.Spanner; +import com.google.cloud.spanner.SpannerException; +import com.google.common.collect.ImmutableList; +import jakarta.annotation.PostConstruct; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.inject.Produces; +import jakarta.inject.Inject; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.Charset; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Consumer; +import java.util.function.Supplier; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.persistence.bootstrap.SchemaOptions; +import org.apache.polaris.persistence.relational.spanner.model.Realm; +import org.apache.polaris.persistence.relational.spanner.util.SpannerUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@ApplicationScoped +public class GoogleCloudSpannerDatabaseClientLifecycleManager { + + private static Logger LOGGER = + LoggerFactory.getLogger(GoogleCloudSpannerDatabaseClientLifecycleManager.class); + + @Inject GoogleCloudSpannerConfiguration spannerConfiguration; Review Comment: For testing purposes it might be more convenient to `@Inject` into a constructor (without CDI). An alternative might be using `weld-junit5` to not have to have a "full Quarkus setup" just for testing. ########## persistence/google-cloud-spanner/src/main/java/org/apache/polaris/persistence/relational/spanner/GoogleCloudSpannerConfiguration.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.polaris.persistence.relational.spanner; + +import java.util.Optional; + +public interface GoogleCloudSpannerConfiguration { Review Comment: Do we need options like appProfileId, quotaProjectId here? Wonder whether Spanner has similar retry options as Bigtable? -- 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]
