Copilot commented on code in PR #3081: URL: https://github.com/apache/hugegraph/pull/3081#discussion_r3683894817
########## hugegraph-store/hg-store-common/src/main/java/org/apache/hugegraph/store/cloud/CloudStorageProviderFactory.java: ########## @@ -0,0 +1,217 @@ +/* + * 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.hugegraph.store.cloud; + +import java.io.IOException; +import java.util.Map; +import java.util.ServiceLoader; +import java.util.concurrent.ConcurrentHashMap; + +import lombok.Getter; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Factory for {@link CloudStorageProvider} instances. + * + * <p>Providers are discovered at class-loading time via {@link ServiceLoader}. + * Any JAR that includes + * {@code META-INF/services/org.apache.hugegraph.store.cloud.CloudStorageProvider} + * is automatically picked up when it is present on the classpath. + * + * <p>Usage: + * <pre> + * CloudStorageConfig cfg = ...; // populated from application.yml + * CloudStorageProvider provider = CloudStorageProviderFactory.initialize(cfg); + * // later: + * CloudStorageProvider active = CloudStorageProviderFactory.getActiveProvider(); + * </pre> + */ +public final class CloudStorageProviderFactory { + + private static final Logger log = LoggerFactory.getLogger(CloudStorageProviderFactory.class); + + /** All discovered providers keyed by {@link CloudStorageProvider#providerName()}. */ + private static final Map<String, CloudStorageProvider> REGISTRY = new ConcurrentHashMap<>(); + + /** The currently active (initialized) provider; null when disabled or not yet initialized. + * -- GETTER -- + * Returns the currently active provider, or + * if cloud storage is + * disabled or + * has not yet been called. + */ + @Getter + private static volatile CloudStorageProvider activeProvider; + + static { + loadProviders(); + } + + private CloudStorageProviderFactory() { + } + + // ----------------------------------------------------------------------- + // Public API + // ----------------------------------------------------------------------- + + /** + * Initializes and activates the cloud storage provider described by {@code config}. + * + * <p>The method is idempotent: if called multiple times, the existing active + * provider is closed before a new one is initialized. + * + * @param config cloud storage configuration + * @return the initialized provider, or {@code null} when + * {@link CloudStorageConfig#isEnabled()} is {@code false} + * @throws IllegalArgumentException if {@code config} is {@code null}, or if no provider + * matching {@code config.getProvider()} + * was found on the classpath + */ + public static synchronized CloudStorageProvider initialize(CloudStorageConfig config) { + if (config == null) { + throw new IllegalArgumentException("cloud storage config must not be null"); + } + + if (!config.isEnabled()) { + // Disabling cloud storage must deactivate any currently active provider: otherwise a + // reconfiguration/context refresh that flips enabled=false would leave the old provider + // (and its live SDK resources) running and still servicing cloud I/O. Close it + // best-effort, drop the reference, and log the deactivation explicitly. + CloudStorageProvider previous = activeProvider; + if (previous != null) { + try { + previous.close(); + } catch (IOException e) { + log.warn("Error closing cloud storage provider '{}' while disabling cloud storage", + previous.providerName(), e); + } + activeProvider = null; + log.info("Cloud storage disabled (cloud.storage.enabled=false) — deactivated and " + + "closed provider '{}'", previous.providerName()); + } else { + log.info("Cloud storage is disabled (cloud.storage.enabled=false)"); + } + return null; + } + + String name = config.getProvider(); + CloudStorageProvider provider = REGISTRY.get(name); + if (provider == null) { + throw new IllegalArgumentException( + "No cloud storage provider found for name '" + name + "'. " + + "Available providers: " + REGISTRY.keySet() + ". " + + "Make sure the provider JAR (e.g. hg-store-cloud-s3) is on the classpath."); + } + + // Clear the active reference BEFORE closing the previous provider or initializing the new + // one. If init(config) throws, the factory must be left in a safe state (activeProvider == + // null) rather than still referencing a now-closed / partially-initialized provider — + // callers would otherwise observe a non-null but unusable provider after a failed + // reconfiguration. activeProvider is only re-set once init succeeds. + CloudStorageProvider previous = activeProvider; + activeProvider = null; + if (previous != null && previous != provider) { + try { + previous.close(); + } catch (IOException e) { + log.warn("Error closing previous cloud storage provider", e); + } + } + + provider.init(config); + activeProvider = provider; + log.info("Cloud storage provider '{}' initialized. bucket={}", + name, config.getProviderProperties().getOrDefault("bucket", "N/A")); + return provider; Review Comment: `config.getProviderProperties()` can be null (the POJO allows it, and tests explicitly set it to null), which makes this log statement throw a NullPointerException after successful initialization. Consider defensively treating null as an empty map (e.g., local `Map<String,String> props = config.getProviderProperties()` with a null fallback) before calling `getOrDefault`. ########## hugegraph-store/hg-store-cloud-s3/pom.xml: ########## @@ -0,0 +1,122 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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. + --> + +<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xmlns="http://maven.apache.org/POM/4.0.0"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 + http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.hugegraph</groupId> + <artifactId>hugegraph-store</artifactId> + <version>${revision}</version> + <relativePath>../pom.xml</relativePath> + </parent> + + <artifactId>hg-store-cloud-s3</artifactId> + + <description> + S3 cloud storage provider plugin for HugeGraph Store. + Add this JAR to the classpath and set cloud.storage.provider=s3 to enable S3 offload. + </description> + + <properties> + <aws.sdk.version>2.33.8</aws.sdk.version> + </properties> + + <dependencies> + <!-- Cloud storage SPI interface --> + <dependency> + <groupId>org.apache.hugegraph</groupId> + <artifactId>hg-store-common</artifactId> + </dependency> + + <!-- + Required in docker/cloud-storage runtime: CloudStorageEventListener hooks into + RocksDB event callback types (FlushJobInfo/TableFileCreationInfo). + Keep this aligned with hg-store-rocksdb to avoid NoClassDefFoundError. + --> + <dependency> + <groupId>org.rocksdb</groupId> + <artifactId>rocksdbjni</artifactId> + <version>7.7.3</version> + <scope>runtime</scope> + </dependency> Review Comment: Pinning `rocksdbjni` to a concrete runtime version inside the S3 plugin risks classpath conflicts with the RocksDB JNI version used by the main store runtime (e.g., multiple `rocksdbjni` versions in the same classloader). To avoid hard-to-debug native/JNI mismatches, consider aligning this version with the main RocksDB module via shared dependencyManagement (or using `provided` scope and relying on the host application's RocksDB JNI). -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
